Pass through apple_split_cpu #196
Conversation
This transtion when used in conjunction with rules_apple's transition causes `apple_split_cpu` to be 100% removed. This PR propagates it to ensure that `swift_library`'s have the same configuration from the CLI. This is the last known instance of something that looks like bazelbuild/bazel#12171 that I could find Testing - it's not clear how to exactly test this. The analysis test mentioned in (#188) will have some benefit but this is mainly an issue with CLI invocations. For now, please test on the CLI ``` bazel clean bazel build -s tests/ios/app:AppWithSelectableCopts tests/ios/app:SwiftLib --apple_platform_type=ios find bazel-out/ -name \*.swiftmodule ```
jerrymarino
requested review from
segiddins and
amberdixon
and removed request for
segiddins
Doing an analysis test with the apple_platform_type set in the build config should allow testing here, and I think we should try to add more formal test cases to ensure we don't regress going forward |
|
I've been researching ways to test this while coming up with a test for #188. The main delta here is that this one is only happening on the CLI and analysis test as is has that ability to accept a |
This test asserts that transitive dependencies have identical outputs for different transition paths. In particular, a rules_apple ios_application and an a apple_framework that share a swift_library, :SwiftLib_swift. This test ensures that the actions in both builds have functionally equal transitions applied by normalizing their output directories into a set. For instance these tests will fail if there is any delta and requires both: - adding apple_common.multi_arch_split to apple_framework.deps - #188 - the transition yields the same result when used w/rules_apple - #196 Note: The gist of Bazel's configuration resolver is that it will apply relevant transitions to keys that are used by a given action. e.g. ios_multi_cpus. src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java In order to get the same configuration for a rule, a given transition has to produce the same values for dependent keys for all possible combinations of edges in a given build.
|
cc @amberdixon @segiddins would you mind giving this one a review? |
| @@ -280,3 +281,5 @@ ios_application( | |||
| ":SwiftLib", | |||
| ], | |||
| ) | |||
|
|
|||
| make_tests() | |||
There was a problem hiding this comment.
is it that we can add more tests under this method in future?
There was a problem hiding this comment.
Mainly a target can't be instantiated from a .bzl file, and I went off the convention of putting the analysis test definitions adjacent to the impl in the .bzl file to keep the BUILD file minimal. Also a nice place to put more instances of _identical_outputs_test e.g. for app VS test transitive dep consistency.
| ) | ||
|
|
||
| def make_tests(): | ||
| # This test asserts that transitive dependencies have identical outputs for |
There was a problem hiding this comment.
To @gyfelton's question, also curious to know how we'll add more tests here. Should make_tests hold all of them? If that's the case (nit) should the test description be moved into _identical_outputs_test ?
There was a problem hiding this comment.
I added a: #196 (comment). If you follow the pattern I setup you can add a test for apps / test bundles like:
make_tests():
.....
_identical_outputs_test(
name = "test_TestHostEquivilance",
target_under_test = ":TestAppWithSelectableCopts",
# These inputs *must* be passed seperately, in order to
# have different transitions applied by Skyframe.
deps = [":TestAppWithSelectableCopts", ":AppWithSelectableCopts"],
)
but _identical_outputs_test cannot be extended as it uses Bazel's analyiss test analysistest.make
This transtion when used in conjunction with rules_apple's transition
causes
apple_split_cputo be 100% removed. This PR propagates it toensure that
swift_library's have the same configuration from the CLI.This is the last known instance of something that looks like
bazelbuild/bazel#12171 that I could find
Testing - it's not clear how to exactly test this. The analysis test
mentioned in (#188) will have some benefit but this is mainly an issue
with CLI invocations.
For now, please test on the CLI