Fix rules_swift 2 compatibility issues #903
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
2 times, most recently
from
31b7fd7
to
238599d
Compare
|
locally it built!(From red -> green). I did notice this warning though: |
Yeah noticed the warning too, doesnt seem related to these changes or even rules_swift 2.x since i see them in |
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
from
238599d
to
86d25aa
Compare
luispadron
commented
Aug 26, 2024
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
4 times, most recently
from
4e6f12e
to
69d3cff
Compare
oh i did not realize that! |
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
from
5721fb1
to
b35b3d7
Compare
luispadron
changed the title
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
2 times, most recently
from
c7ded82
to
8bae0a7
Compare
gyfelton
approved these changes
Aug 28, 2024
|
we should add about what this fixes (what's the original error) |
luispadron
force-pushed
the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
from
8bae0a7
to
ceceeeb
Compare
luispadron
changed the title
thiagohmcruz
approved these changes
Aug 28, 2024
congt
approved these changes
Aug 28, 2024
…_swift 2.1.1 (#906) One of the CI error is: ``` error: generate-pch command failed with exit code 1 (use -v to see invocation) tests/ios/unit-test/test-imports-app/TestImports-App-Bridging-Header.h:1:9: error: 'TestImports-App/Header2.h' file not found #import <TestImports-App/Header2.h> ^ 1 error generated. <unknown>:0: error: failed to emit precompiled header 'bazel-out/ios-sim_arm64-min12.0-applebin_ios-ios_sim_arm64-dbg-ST-c2aefc9133a8/bin/_pch_output_dir/TestImports-App-Bridging-Header-swift_28IU2BV1DK30K-clang_1FNSWCOAS4SUQ.pch' for bridging header 'tests/ios/unit-test/test-imports-app/TestImports-App-Bridging-Header.h' ``` This is because this header was suppose to be an input to Swift compile via hmap creation rule. But in this breaking change bazelbuild/rules_swift@d68b214 rules_swift stopped collecting compilation context from `CcInfo` if the target is Swift code (line 1555 of `swift/internal/compiling.bzl`). Instead it depends on reading the same info from `clang_module.compilation_context` which is available from `SwiftInfo` provider (SwiftInfo -> modules -> clang) This PR deals with header maps side where we propagate compilation context via this `clang` module Next step is to do the same for other places where we need to pass the compilation context CI for this PR should no longer produce this error (but will still fail on some other tests)
luispadron
changed the title
…s_swift 2.1.1 (#907) same idea as #906 this time is for framework rule. The CI error trying to fix in this PR is: ``` # Execution platform: @local_config_platform//:host error: generate-pch command failed with exit code 1 (use -v to see invocation) tests/ios/unit-test/test-imports-app/TestImports-App-Bridging-Header.h:1:9: error: 'TestImports-App/Header2.h' file not found #import <TestImports-App/Header2.h> ^ 1 error generated. <unknown>:0: error: failed to emit precompiled header 'bazel-out/ios-sim_arm64-min12.0-applebin_ios-ios_sim_arm64-dbg-ST-c2aefc9133a8/bin/_pch_output_dir/TestImports-App-Bridging-Header-swift_28IU2BV1DK30K-clang_1FNSWCOAS4SUQ.pch' for bridging header 'tests/ios/unit-test/test-imports-app/TestImports-App-Bridging-Header.h' 2 errors generated. error: fatalError ``` This time this header is brought in by the framework rule The idea here is to create the `clang_module` based of the same compilation context as CcInfo and construct the final swift info based on it. With this no need to differentiate based on if VFS turned on or not
|
All CI should be green now and we can merge when it does, there will be other follow up work but will land them later |
luispadron
deleted the
luis/add-clang-attr-to-merged-swift-info-for-framework
branch
gyfelton
added a commit
that referenced
this pull request
Sep 5, 2024
What changed and why: 1. added test case that broke with main, under sandbox mode and bazel 7 and `arm64_simulator_use_device_deps` feature turned on : To repro locally, run `bazel build //tests/ios/unit-test/test-imports-app:TestImports-App --config=ios --features apple.arm64_simulator_use_device_deps` and it fails. But success if change `.bazelversion` to 6.4.0 The error is "unable to find header `basic.h`" which is the same issue with what our own repo has. Also this only break Objc side not swift side (probably because CcInfo is more used by objc_library?) 2. To fix above: use the compilation_context generated originally. The original fix #873 is missing fields inside `compilation_context` such as `headers`. So might as well use the original CcInfo collected, and only recreate the linking context. BTW i believe the original PR aims to fix this kind of error in bazel 7: ``` ld: building for 'iOS-simulator', but linking in object file (/path/to/someframework.framework[arm64][2] built for 'iOS' ``` Which is the error we got if trying to just use the original CcInfo. 3. Update the test matrix to have sandbox mode for the tests for `arm64_simulator_use_device_deps` feature Tests done: Without the change from #903 some checks should still fail but the ones using `arm64_simulator_use_device_deps` should be green
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes fixes for the following issues:
Fixes the following issue with non-vfs on Bazel 7+:
Fixes the following issue in Bazel 6.5.0 with both vfs/non-vfs:
Fixes the following CI jobs that are failing in
master:Bazel 6.5.0 / Xcode 15.2 / VFS false / Sandbox true / Latest rules trueBazel 6.5.0 / Xcode 15.2 / VFS false / Sandbox false / Latest rules trueBazel 6.5.0 / Xcode 15.2 / VFS true / Sandbox true / Latest rules trueBazel 6.5.0 / Xcode 15.2 / VFS true / Sandbox false / Latest rules trueBazel 7.1.0 / Xcode 15.2 / VFS false / Sandbox true / Latest rules trueBazel 7.1.0 / Xcode 15.2 / VFS false / Sandbox false / Latest rules true