Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for symbol graph extraction #772

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add support for symbol graph extraction #772

wants to merge 4 commits into from

Conversation

keith
Copy link
Member

@keith keith commented Mar 3, 2022 •
edited by brentleyjones

keith referenced this pull request Mar 3, 2022
@allevato @swiple-rules-gardener
Add support for symbol graph extraction.
d41cdb9 
This change adds the following APIs:

*   An aspect, `swift_symbol_graph_aspect`, which can traverse a dependency graph and extract symbol graphs from Swift modules that it finds. The results are propagated in a `SwiftSymbolGraphInfo` provider.
*   A rule, `swift_extract_symbol_graph`, which applies the above aspect to one or more targets and combines the results into a single directory.
*   A low-level Starlark API, `swift_common.extract_symbol_graph`, which creates the action to invoke `swift-symbolgraph-extract` on a single `.swiftmodule`.

PiperOrigin-RevId: 430517714
@keith
Copy link
Member Author

keith commented Mar 3, 2022

blocked on infra changes that are blocked on aspect_hints

@lyft-lint-bot
Copy link

Lyft integration job started: https://buildkite.com/lyft/rules-swift/builds/248 (must be Lyft employee to view)

keith referenced this pull request Mar 3, 2022
@allevato @swiple-rules-gardener
Update the swift_test rule to perform test discovery if a target is…
39dfb23 
… not using bundled tests (i.e., not Objective-C-runtime-based XCTest) and has no `srcs`.

PiperOrigin-RevId: 430526469
keith referenced this pull request Mar 3, 2022
@allevato @swiple-rules-gardener
Add a test discovery tool that uses SymbolKit to detect XCTestCase …
47a02f1 
…subclasses and their methods from symbol graphs and generate the necessary runner.

This is similar to the logic used by Swift Package Manager to do test discovery, except that SPM uses the index store. We use symbol graphs since JSON is much easier to process than index store records and they can be easily extracted from modules after they've been built.

Note to open-source rules maintainers: You will need to provide BUILD definitions for swift-argument-parser and swift-docc-symbolkit in order to populate these dependencies.

PiperOrigin-RevId: 430518889
keith referenced this pull request Mar 9, 2022
@allevato @swiple-rules-gardener
Affix the class name to distinguish the __allTests arrays generated…
3e90cda 
… in extensions.

This prevents a collision when one discovered test class subclasses another. SwiftPM at HEAD currently doesn't do this (it used to), leading to bugs like SR-15955. This seems like a regression, so I'm fixing it in our tool.

PiperOrigin-RevId: 433481652
@luispadron
Copy link
Contributor

@keith is this still blocked? Im trying to cherry-pick 6cae838 which has some dependencies on this for swift_test

@keith
Copy link
Member Author

keith commented Nov 17, 2023

yea unclear. it's probably quicker if you're worried about it to handle the merge conflicts with cherry picking that commit as I did for the other ones related to compiler plugins

@brentleyjones
Copy link
Collaborator

I'm updating this PR

@brentleyjones brentleyjones force-pushed the ks/add-support-for-symbol-graph-extraction branch from 9e46353 to 9de9e02 Compare April 5, 2024 20:56
@brentleyjones brentleyjones changed the title Add support for symbol graph extraction. Add support for symbol graph extraction Apr 5, 2024
@brentleyjones
Copy link
Collaborator

@jpsim This adds a different way of extracting symbol graphs. It also changes the name of the directory for derived_files.symbol_graph_directory (to match what was used upstream), and I'm not sure if that could cause a clash if your feature is also enabled while using the new rule/aspect. So, two questions:

  1. Does this new stuff replace the need for your existing stuff?
  2. If not, is there anything that should be changed here, to ensure that they work well together (including potential doc changes)?

@brentleyjones brentleyjones marked this pull request as ready for review April 5, 2024 21:02
@brentleyjones brentleyjones force-pushed the ks/add-support-for-symbol-graph-extraction branch from 9de9e02 to 671296e Compare April 5, 2024 21:03
brentleyjones
brentleyjones reviewed Apr 5, 2024
View reviewed changes
swift/repositories.bzl
Comment on lines +119 to +127
_maybe(
http_archive,
name = "com_github_apple_swift_docc_symbolkit",
urls = ["https://github.com/apple/swift-docc-symbolkit/archive/refs/tags/swift-5.10-RELEASE.tar.gz"],
sha256 = "de1d4b6940468ddb53b89df7aa1a81323b9712775b0e33e8254fa0f6f7469a97",
strip_prefix = "swift-docc-symbolkit-swift-5.10-RELEASE",
build_file = "@build_bazel_rules_swift//third_party:com_github_apple_swift_docc_symbolkit/BUILD.overlay",
)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like adding another non-bzlmod dependency. I assume we should add this to BCR like com_github_apple_swift_argument_parser?

@allevato @brentleyjones
Add support for symbol graph extraction.
8e04ab2 
This change adds the following APIs:

*   An aspect, `swift_symbol_graph_aspect`, which can traverse a dependency graph and extract symbol graphs from Swift modules that it finds. The results are propagated in a `SwiftSymbolGraphInfo` provider.
*   A rule, `swift_extract_symbol_graph`, which applies the above aspect to one or more targets and combines the results into a single directory.
*   A low-level Starlark API, `swift_common.extract_symbol_graph`, which creates the action to invoke `swift-symbolgraph-extract` on a single `.swiftmodule`.

PiperOrigin-RevId: 430517714
(cherry picked from commit d41cdb9)
@allevato @brentleyjones
Add a test discovery tool that uses SymbolKit to detect XCTestCase …
e426217 
…subclasses and their methods from symbol graphs and generate the necessary runner.

This is similar to the logic used by Swift Package Manager to do test discovery, except that SPM uses the index store. We use symbol graphs since JSON is much easier to process than index store records and they can be easily extracted from modules after they've been built.

Note to open-source rules maintainers: You will need to provide BUILD definitions for swift-argument-parser and swift-docc-symbolkit in order to populate these dependencies.

PiperOrigin-RevId: 430518889
(cherry picked from commit 47a02f1)
@allevato @brentleyjones
Update the swift_test rule to perform test discovery if a target is…
69d69c0 
… not using bundled tests (i.e., not Objective-C-runtime-based XCTest) and has no `srcs`.

PiperOrigin-RevId: 430526469
(cherry picked from commit 39dfb23)
@allevato @brentleyjones
Affix the class name to distinguish the __allTests arrays generated…
4d91e55 
… in extensions.

This prevents a collision when one discovered test class subclasses another. SwiftPM at HEAD currently doesn't do this (it used to), leading to bugs like SR-15955. This seems like a regression, so I'm fixing it in our tool.

PiperOrigin-RevId: 433481652
(cherry picked from commit 3e90cda)
@brentleyjones
Copy link
Collaborator

Should non-Xcode toolchains support swift_action_names.SYMBOL_GRAPH_EXTRACT?

@brentleyjones brentleyjones force-pushed the ks/add-support-for-symbol-graph-extraction branch from 671296e to 4d91e55 Compare April 5, 2024 21:13
@brentleyjones
Copy link
Collaborator

I got confirmation that the Linux toolchain isn't being upstreamed anymore. So there are parts we need to do here for that.

@luispadron
Copy link
Contributor

This symbol graph rule also seems to better support extending the symbol graph (e.g. with minimum access level attr). Theres more flags related to symbol graph generation we could add too, such as the one required to support: bazelbuild/rules_apple#2445.

I have a work-in-progress PR that adds -emit-extension-block-symbols via a swift.emit_symbol_graph_extension_blocks feature but using an attr in this new rule makes more sense. Do we think the change to the existing symbol graph code is worth making or should we wait for this new rule? Do we think this will land soon

@brentleyjones
Copy link
Collaborator

I would like to get @jpsim's thoughts on this.

@jpsim
Copy link
Contributor

jpsim commented Apr 23, 2024

I'm no longer involved with any projects that would use this or the previous symbol extraction for DocC. If this is what's upstreamed and sounds like more useful than we have today, then I'm all for cherry-picking it in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brentleyjones brentleyjones brentleyjones left review comments

@BalestraPatrick BalestraPatrick Awaiting requested review from BalestraPatrick BalestraPatrick is a code owner

@luispadron luispadron Awaiting requested review from luispadron luispadron is a code owner

@mattrobmattrob mattrobmattrob Awaiting requested review from mattrobmattrob mattrobmattrob is a code owner

@thii thii Awaiting requested review from thii thii is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@keith @lyft-lint-bot @luispadron @brentleyjones @jpsim @allevato