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

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Add support for symbol graph extraction #772

merged 5 commits into from
Jun 19, 2024

Conversation

keith
Copy link
Member

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

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
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.

@luispadron luispadron mentioned this pull request Jun 12, 2024
Closed
@brentleyjones brentleyjones force-pushed the ks/add-support-for-symbol-graph-extraction branch 2 times, most recently from 8a649b2 to 5f8e3bc Compare June 19, 2024 00:38
luispadron added a commit that referenced this pull request Jun 19, 2024
@luispadron
Remove emit symbol graph API
d7d96c8 
Removes the now old `emit_symbol_grah` feature in favor of the new `swift_symbol_graph_extract` rule which can be used to collec the symbol graph of Swift targets.

Depends on: #772
@luispadron luispadron mentioned this pull request Jun 19, 2024
Merged
luispadron
luispadron approved these changes Jun 19, 2024
View reviewed changes
Copy link
Contributor

@luispadron luispadron left a comment

Choose a reason for hiding this comment

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

CI should pass on a rebase now

@luispadron luispadron requested a review from brentleyjones June 19, 2024 18:36
allevato and others added 5 commits June 19, 2024 15:08
@allevato @brentleyjones
Add support for symbol graph extraction.
c3998db 
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)

Signed-off-by: Brentley Jones <github@brentleyjones.com>
@allevato @brentleyjones
Add a test discovery tool that uses SymbolKit to detect XCTestCase …
1e11915 
…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…
8079ec2 
… not using bundled tests (i.e., not Objective-C-runtime-based XCTest) and has no `srcs`.

PiperOrigin-RevId: 430526469
(cherry picked from commit 39dfb23)

Signed-off-by: Brentley Jones <github@brentleyjones.com>
@allevato @brentleyjones
Affix the class name to distinguish the __allTests arrays generated…
5005c20 
… 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)
@luispadron @brentleyjones
Add symbol graph extraction support to swift_toolchain for Linux
4f132c9
@brentleyjones brentleyjones force-pushed the ks/add-support-for-symbol-graph-extraction branch from 5f8e3bc to 4f132c9 Compare June 19, 2024 20:08
@brentleyjones brentleyjones enabled auto-merge (rebase) June 19, 2024 20:08
@brentleyjones brentleyjones merged commit 6e48feb into master Jun 19, 2024
14 checks passed
@brentleyjones brentleyjones deleted the ks/add-support-for-symbol-graph-extraction branch June 19, 2024 20:13
luispadron added a commit that referenced this pull request Jun 19, 2024
@luispadron
Remove emit symbol graph API
e035463 
Removes the now old `emit_symbol_grah` feature in favor of the new `swift_symbol_graph_extract` rule which can be used to collec the symbol graph of Swift targets.

Depends on: #772
Rebase
luispadron added a commit that referenced this pull request Jun 19, 2024
@luispadron
Remove emit symbol graph API
569202f 
Removes the now old `emit_symbol_grah` feature in favor of the new `swift_symbol_graph_extract` rule which can be used to collec the symbol graph of Swift targets.

Depends on: #772
Rebase
luispadron added a commit that referenced this pull request Jun 19, 2024
@luispadron
Remove emit symbol graph API
ad7216b 
Removes the now old `emit_symbol_grah` feature in favor of the new `swift_symbol_graph_extract` rule which can be used to collec the symbol graph of Swift targets.

Depends on: #772
Rebase
luispadron added a commit that referenced this pull request Jun 19, 2024
@luispadron
Remove emit symbol graph API
7c3a781 
Removes the now old `emit_symbol_grah` feature in favor of the new `swift_symbol_graph_extract` rule which can be used to collec the symbol graph of Swift targets.

Depends on: #772
Rebase
brentleyjones referenced this pull request Jun 21, 2024
@thomasvl @swiple-rules-gardener
Internal change
300921b 
PiperOrigin-RevId: 549019748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luispadron luispadron luispadron approved these changes

@BalestraPatrick BalestraPatrick Awaiting requested review from BalestraPatrick BalestraPatrick 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

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

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

Successfully merging this pull request may close these issues.
6 participants
@keith @lyft-lint-bot @luispadron @brentleyjones @jpsim @allevato