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.

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

[cache][multi-arch] use apple_common.multi_arch_split #188

Merged
merged 11 commits into from
Jan 14, 2021

Conversation

jerrymarino
Copy link
Contributor

This PR fixes #184

Today, there's redundant builds of a swift_library with rules_ios framework
bundle and rules_apple bundles like a an app or test.

Given:

apple_framework(name = "fw", ..)
ios_application(name="app", deps=["fw"])

When running the build, there will be 2 swiftmodules built ( 1 per the app, and 1 per the framework )

bazel build app fw

Consequently, it builds 2x of all the transitive swift libraries and also
causes issues with cache hits.

The root cause is that multi platform support isn't implemented in rules apple.
To partition up actions, we give the deps the transition
apple_common.multi_arch_split. This implicates a different configuration

Note that there is more changes to handle ios_multi_cpus
end to end #186

Testing - it doesn't seem possible to test this with an analysis test
with skylib given the way Bazel transtively computes the output
directory, e.g. for a target_under_test. When #186 is done adding a
CLI test with ios_multi_cpus will assert that it can build in this
way.

Copy link
Collaborator

@amberdixon amberdixon left a comment

Choose a reason for hiding this comment

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

The approach makes sense to me, but left an open question about the new test introduced.

tests/ios/app/BUILD.bazel Outdated Show resolved Hide resolved
Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

Testing - it doesn't seem possible to test this with an analysis test
with skylib given the way Bazel transtively computes the output
directory, e.g. for a target_under_test. When #186 is done adding a
CLI test with ios_multi_cpus will assert that it can build in this
way.

I think we can use analysis tests to force into whatever config we want, and take multiple targets as deps and check that the subdir of bazel-out that their outputs go into matches?

rules/framework.bzl Show resolved Hide resolved
tests/ios/app/BUILD.bazel Outdated Show resolved Hide resolved
rules/framework.bzl Show resolved Hide resolved
tests/ios/app/BUILD.bazel Outdated Show resolved Hide resolved
@jerrymarino
Copy link
Contributor Author

I think we can use analysis tests to force into whatever config we want, and take multiple targets as deps and check that the subdir of bazel-out that their outputs go into matches?

Do you have any suggestion on how to use an analysis test to force a config? I couldn't figure out a way to do this without a decently complex aspect, which might be more trouble than its worth. Anyhow, there's a few other issues I've gotta work out with the CI on this still.

@segiddins
Copy link
Member

@jerrymarino https://github.com/bazelbuild/bazel-skylib/blob/master/docs/unittest_doc.md#analysistest.make-config_settings should allow changing config settings used for analysis of the target under test

rules/BUILD.bazel Outdated Show resolved Hide resolved
rules/framework.bzl Outdated Show resolved Hide resolved
tests/ios/unit-test/test-imports-app/BUILD.bazel Outdated Show resolved Hide resolved
This PR fixes #184

Today, there's redundant builds of a `swift_library` with `rules_ios` framework
bundle and `rules_apple` bundles like a an app or test.

Given:
```
apple_framework(name = "fw", ..)
ios_application(name="app", deps=["fw"])
```

When running the build, there will be 2 swiftmodules built ( 1 per the app, and 1 per the framework )
```
bazel build app fw
```
Consequently, it builds 2x of all the transitive swift libraries and also
causes issues with cache hits.

The root cause is that multi platform support isn't implemented in rules apple.
To partition up actions, we give the deps the transition
`apple_common.multi_arch_split`. This implicates a different configuration

Note that there is more changes to handle `ios_multi_cpus`
end to end #186

Testing - it doesn't seem possible to test this with an analysis test
with skylib given the way Bazel transtively computes the output
directory, e.g. for a `target_under_test`. When #186 is done adding a
CLI test with `ios_multi_cpus` will assert that it can build in this
way.
@jerrymarino jerrymarino force-pushed the jmarino/multi_arch_pass_one branch from e239c24 to 8b6216d Compare January 14, 2021 01:59
@segiddins
Copy link
Member

@jerrymarino it looks like this no longer has anything to do with apple_common.multi_arch_split, did you force-push the wrong branch?

@jerrymarino jerrymarino force-pushed the jmarino/multi_arch_pass_one branch from 8b6216d to abe0586 Compare January 14, 2021 17:57
@jerrymarino
Copy link
Contributor Author

@segiddins thanks for digging into the bit on analysis test config - this is great. Once I've fixed the misc edges on this I suspect I can come up with an aspect to surface out all swift_library outputs and make assertions on the output root in the test. Likely would be a followup to keep this one somewhat straight forward.

@jerrymarino jerrymarino merged commit 1b32a5c into master Jan 14, 2021
@jerrymarino jerrymarino deleted the jmarino/multi_arch_pass_one branch January 14, 2021 19:52
jerrymarino added a commit that referenced this pull request Jan 15, 2021
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 added a commit that referenced this pull request Jan 16, 2021
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.
jerrymarino added a commit that referenced this pull request Jan 19, 2021
* Pass through apple_split_cpu

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
```

* Analysis test for bazelbuild/bazel#12171

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Redundant swift_library builds when using apple_framework and ios_application
5 participants