Fix apple_library rule not working in sandbox when do SwiftCompile in mixed sources
#894
Conversation
gyfelton
changed the title
SwiftCompile in mixed sources
| @@ -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] | |||
There was a problem hiding this comment.
Doesnt this already get added for the extended modulemap below? Or is that overridden 👀
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
@gyfelton Could you test deleting line 965 to see if it still works as expected?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
So the extended modulemap has two major diff:
- location: the added module map has umbrella header next to it, but the extended one is at root
- 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?
There was a problem hiding this comment.
strike that, that is needed or we got -swift.h file not found error in our repo's compilation
There was a problem hiding this comment.
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.
gyfelton
force-pushed
the
gyfelton/fix-library-sandboxed
branch
from
4ca032a to
2a88b70
Compare
| @@ -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, | |||
There was a problem hiding this comment.
We should move the TODO to line 967.
There was a problem hiding this comment.
yup removing in my next PR trying to get CI sandbox run
What changed: 1. Add sandbox on/off to testing matrix, so we have 2 bazel version * VFS on/off * sandbox on off = 8 tests in total ATM 2. Fix redefinition of `AppErrorCode` enum error in sandbox mode by removing the extra import in bridging header. The tests still works and can catch renaming of the enum for example. Why it was working in non sandbox mode is unknown 3. Add `sandbox` config to include all settings needed to pass CI. specifically allow a small set of actions to run locally. Why this change: To add CI coverage to sandbox mode to ensure no regression on it, now that we have fixed issues with sandbox mode in #894 Discussion: CI duration might be a concern, we can exclude certain combo in the test matrix if we want, it's possible with `exclude` keyword
The normal modulemap was added in #894 and the extended modulemap is only for the objective-c part of the library so we shouldn't add it for the swiftc_inputs
To repro, run
bazel clean; bazel build //tests/ios/frameworks/mixed-source/only-source:SwiftLibrary -s --sandbox_debug --strategy=SwiftCompile=sandboxedand got<unknown>:0: error: underlying Objective-C module 'SwiftLibrary' not founderrorusing
bazel aquery //tests/ios/frameworks/mixed-source/only-source:SwiftLibrary_swiftto look at input list, realizedbazel-out/darwin_arm64-dbg/bin/tests/ios/frameworks/mixed-source/only-source/SwiftLibrary-modulemap/SwiftLibrary.modulemapentry is not there even though that's sth generated by the module_map generatorSo adding this fixed the issue because sandbox can now pick up this file
Next step: setup CI job here that run everything in sandbox mode, try to fix anything that is broken in another PR