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

Relanding of Add swift.experimental_rules_swift_package_manager flag back to master branch #922

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

gyfelton
Copy link
Contributor

@gyfelton gyfelton commented Sep 18, 2024

already approved at #917

Now it's about landing it in master and since it is guarded by a flag, it's no op unless you enable this feature

@gyfelton gyfelton changed the title Relanding of #917 back to main Relanding of Add swift.experimental_rules_swift_package_manager flag back to master branch Sep 18, 2024
@gyfelton gyfelton force-pushed the thiago/rules_swift_package_manager-1 branch from 72ba0cf to d704f72 Compare September 18, 2024 19:13
Comment on lines +275 to +277
if enable_rules_swift_package_manager and SwiftInfo in dep:
if dep.label.workspace_name.startswith("swiftpkg"):
for spm_dep in dep[SwiftInfo].transitive_modules.to_list():
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont really understand why we need to specifically check for rules_spm here, can you help me understand?

rules_spm creates swift_library_group targets which have deps attr, and all the things in deps should already have SwiftInfo so couldn't you just iterate through the deps here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We had other internal targets going through this conditional internally, the intent was to explicitly filter for only the SPM ones to minimize the chances of unintentionally breaking things for other consumers due to this being WIP-code under an experimental flag for now.

That said, if there's a more optimal way to collect the same info def open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess I just don't understand what this is doing and why we should do it for rules_spm stuff but not rules_swift

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not trying to block, feel free to merge if this fixes something for y'all. Trying to understand because we also use rules_spm and haven't needed this

@gyfelton gyfelton enabled auto-merge (squash) September 18, 2024 21:27
@gyfelton gyfelton merged commit b5efa1f into master Sep 18, 2024
23 checks passed
@gyfelton gyfelton deleted the thiago/rules_swift_package_manager-1 branch September 18, 2024 23:47
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.

3 participants