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

Fix apple_library rule not working in sandbox when do SwiftCompile in mixed sources #894

Merged
merged 1 commit into from
Aug 19, 2024
Merged

Fix apple_library rule not working in sandbox when do SwiftCompile in mixed sources #894

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: 4 additions & 1 deletion rules/library.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -931,6 +931,7 @@ def apple_library(

if has_swift_sources:
additional_swift_copts += ["-Xcc", "-I."]
swiftc_inputs = other_inputs + objc_hdrs + objc_private_hdrs
if module_map:
# Frameworks find the modulemap file via the framework vfs overlay
if not namespace_is_module_name:
Expand All @@ -940,8 +941,8 @@ def apple_library(
"@build_bazel_rules_ios//:swift_disable_import_underlying_module": [],
"//conditions:default": ["-import-underlying-module"] if not feature_names.swift_disable_import_underlying_module in features else [],
})
swiftc_inputs += [module_map]
Copy link
Collaborator

@luispadron luispadron Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Doesnt this already get added for the extended modulemap below? Or is that overridden 👀

Copy link
Contributor

@congt congt Aug 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This module_map is passed to framework_vfs_overlay_name_swift, while the extended modulemap below is different and passed to framework_vfs_overlay_name and objc_libname. I think it's unnecessary to add the extended modulemap to swiftc_inputs.

(BTW, the code is really hard to reason. We need to clean it up)

Copy link
Contributor

Choose a reason for hiding this comment

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

@gyfelton Could you test deleting line 965 to see if it still works as expected?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah agreed on it being hard to reason about. I'm happy to merge whatever has the least amount of changes, thinking about it more im not sure extended module map should be an input so let's try deleting it.

It might be best to create a sandbox test ci job and merge these together

Copy link
Contributor Author

@gyfelton gyfelton Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So the extended modulemap has two major diff:

  1. location: the added module map has umbrella header next to it, but the extended one is at root
Screenshot 2024-08-19 at 9 43 48 AM
  1. content:
    extended one has the following:
framework module SwiftLibrary {
    umbrella header "SwiftLibrary-umbrella.h"

    export *
    module * { export * }
}

module SwiftLibrary.Swift {
    header "SwiftLibrary-Swift.h"
    requires objc
}

The SwiftLibrary.Swift module is the net addition to the original module map

deleting the adding of extended module map works for my case but i am not sure about the implication of removing it.
I will add a TODO here for now and if we all agree on the future plan, i can come back to this esp. during the time when i need to have rules_ios to fully pass CI in sandbox. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

strike that, that is needed or we got -swift.h file not found error in our repo's compilation

Copy link
Contributor

Choose a reason for hiding this comment

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

that is needed or we got -swift.h file not found error in our repo's compilation

Yes, we still need the extended modulemap for framework_vfs_overlay_name and objc_libname, but I don't think we need to add it to swiftc_inputs on line 967 because the swift compilation shouldn't need the umbrella header or -Swift.h.


swiftc_inputs = other_inputs + objc_hdrs + objc_private_hdrs
if swift_objc_bridging_header:
if swift_objc_bridging_header not in objc_hdrs:
swiftc_inputs.append(swift_objc_bridging_header)
Expand All @@ -952,6 +953,8 @@ def apple_library(
generated_swift_header_name = module_name + "-Swift.h"

if module_map:
# TODO: now that we always add module_map as a swiftc_input,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should move the TODO to line 967.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup removing in my next PR trying to get CI sandbox run

# we should consider removing this one if it's not needed
extend_modulemap(
name = module_map + ".extended." + name,
destination = "%s.extended.modulemap" % name,
Expand Down