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

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

Merged
merged 1 commit into from
Sep 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions rules/features.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,9 @@ feature_names = struct(

# When set disable passing the `-import-underlying-module` copt to `swift_library` targets
swift_disable_import_underlying_module = "swift.swift_disable_import_underlying_module",

# Allows consumers to depend on targets from https://github.com/cgrindel/rules_swift_package_manager.
#
# Note: This is a work in progress, and the flag will remain experimental until full support is ready.
experimental_rules_swift_package_manager = "swift.experimental_rules_swift_package_manager",
)
20 changes: 20 additions & 0 deletions rules/framework.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,27 @@ def _get_virtual_framework_info(ctx, framework_files, compilation_context_fields

# We need to map all the deps here - for both swift headers and others
fw_dep_vfsoverlays = []

# Whether or not to collect SwiftInfo providers from rules_swift_package_manager targets
enable_rules_swift_package_manager = feature_names.experimental_rules_swift_package_manager in ctx.features

for dep in transitive_deps + deps:
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():
Comment on lines +275 to +277
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

if spm_dep.swift:
spm_dep_vfs = make_vfsoverlay(
ctx,
hdrs = [],
module_map = [],
swiftmodules = [spm_dep.swift.swiftmodule, spm_dep.swift.swiftdoc],
private_hdrs = [],
has_swift = True,
merge_vfsoverlays = [],
framework_name = spm_dep.name,
)
fw_dep_vfsoverlays.append(spm_dep_vfs.vfs_info)

# Collect transitive headers. For now, this needs to include all of the
# transitive headers
if CcInfo in dep:
Expand Down