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.

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

Add support for forwarding symbol graph #734

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

luispadron
Copy link
Collaborator

rules_swift has support for symbol_graph in SwiftInfo: bazelbuild/rules_swift#838

The framework target should forward that as well

rules/framework.bzl Outdated Show resolved Hide resolved
@luispadron luispadron marked this pull request as ready for review July 6, 2023 00:34
if SwiftInfo in dep and dep[SwiftInfo].direct_modules:
module = dep[SwiftInfo].direct_modules[0]
if module.swift and module.swift.symbol_graph:
symbol_graph = module.swift.symbol_graph
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that the symbol graph shouldn't be part of inputs and outputs / adjacent to the other components? The existing compiler artifacts are collected onto these struct(s) / follow this pattern and would suggest to keep the .symbolgraph similar to how we process .swiftmodule output

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The symbol graphs are added as extra outputs. They don't seem to be available in the rule last I checked

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a way to access extra outputs from the rule? What you suggest is what I tried originally but the symbol graph wasn't in the list the rule gets.

If there's a way to query the extra output groups though that would work

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated this to use dep.output_groups, in there it will have a swift_symbol_graph if the --features=swift.emit_symbol_graph is provided (which the .docc rule transitions on)

@@ -528,6 +538,7 @@ def _copy_swiftmodule(ctx, framework_files):
swiftdoc = outputs.swiftdoc,
swiftmodule = outputs.swiftmodule,
swiftinterface = outputs.swiftinterface,
symbol_graph = outputs.symbol_graph,
Copy link
Contributor

Choose a reason for hiding this comment

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

Changes LGTM 👍 .

Seems like plumbing in the .symbolgraph here good enough to get it correctly propagating end to end for you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep this worked in testing, it correctly forwards the libraries symbol graphs (if they were requested)

@luispadron luispadron merged commit e8e6a0f into master Jul 10, 2023
@luispadron luispadron deleted the lpadron/swift-symnol-graph branch July 10, 2023 23:15
@thiagohmcruz
Copy link
Contributor

Looks like this is not backwards compatible:

Traceback (most recent call last):
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 1042, column 40, in _apple_framework_packaging_impl
                swift_info = _get_merged_swift_info(ctx, framework_files, transitive_deps)
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 566, column 57, in _get_merged_swift_info
                swift_info_fields["modules"] = _copy_swiftmodule(ctx, framework_files)
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 537, column 52, in _copy_swiftmodule
                swift_module = swift_common.create_swift_module(
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_swift/swift/internal/providers.bzl", line 357, column 5, in create_swift_module
                def create_swift_module(
Error: create_swift_module() got unexpected keyword argument: symbol_graph

Was that intentional? We can bump rules_swift internally but curious if there's any suggestion on how to bump rules_ios with this change. I can also look into how to make this optional in rules_ios if that's what we want.

@thiagohmcruz
Copy link
Contributor

Looks like this is not backwards compatible:

Traceback (most recent call last):
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 1042, column 40, in _apple_framework_packaging_impl
                swift_info = _get_merged_swift_info(ctx, framework_files, transitive_deps)
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 566, column 57, in _get_merged_swift_info
                swift_info_fields["modules"] = _copy_swiftmodule(ctx, framework_files)
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_ios/rules/framework.bzl", line 537, column 52, in _copy_swiftmodule
                swift_module = swift_common.create_swift_module(
        File "/private/var/tmp/_bazel_thiago/90ec49cd0a280be0e29f918d4d51c649/external/build_bazel_rules_swift/swift/internal/providers.bzl", line 357, column 5, in create_swift_module
                def create_swift_module(
Error: create_swift_module() got unexpected keyword argument: symbol_graph

Was that intentional? We can bump rules_swift internally but curious if there's any suggestion on how to bump rules_ios with this change. I can also look into how to make this optional in rules_ios if that's what we want.

Nvm, we're internally using an old version of rules_swift overriding the version rules_ios uses so that's on us and I'll take a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants