-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Support execution constraints per exec group #13110
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
Conversation
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
I am rolling forward the test exec change separately (#13119) so it can be separated from this. Once that is in place, can you rebase and update the commits to make the CLA bot happy? |
|
Sure, will do (in fact, I believe this PR only includes those changes, but I originally based it on #12719). |
Previously, exec groups were rejected on platforms. This change allows setting arbitrary exec groups on platforms, and applying only the relevant ones to actions after platform resolution. Also fixes a bug where the default execution platform for the "test" execution group did not take into account execution constraints set on targets.
…r specific groups take precedence.
5736c61 to
da8952d
Compare
|
@katre: rebased and double-checked everything's in order. Thanks! |
|
Thanks! I've taken a quick look at this and it seems okay, but I want to think about it some more. One thing that would really help would be if you could add a test case to https://cs.opensource.google/bazel/bazel/+/master:src/test/shell/bazel/toolchain_test.sh: this will a) test the code, and b) show how it's intended to be used. It would also be useful to add tests to https://cs.opensource.google/bazel/bazel/+/master:src/test/java/com/google/devtools/build/lib/analysis/StarlarkExecGroupTest.java |
|
Great, will do. I've been wondering where would tests actually go. Thank you! |
|
Added a couple of test cases. I think that just about covers it, but do let me know if you think any more could be helpful. |
katre
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few comments and then this will be ready to merge. Thanks for working with me to get this in shape!
| grep "platform_key" out.txt || fail "Did not find the platform key" | ||
| grep "default_value" out.txt || fail "Did not find the default value" | ||
| grep "test_value" out.txt && fail "Used the test-action value when not testing" | ||
| (! grep "test_value" out.txt) || fail "Used the test-action value when not testing" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this syntax existed. I see the benefits, but it's very different than all the other tests, so can you please change this back to the previous style, and use the same style in your new tests? I think consistency is better in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I couldn't figure out why, but one of the tests failed with that style (and I missed the test case with expected failure). I ended up removing it, as there is a positive test anyway.
| private static ImmutableMap<String, ImmutableMap<String, String>> parseExecProperties( | ||
| Map<String, String> rawExecProperties, Set<String> execGroups) | ||
| throws InvalidExecGroupException { | ||
| private static Map<String, Map<String, String>> parseExecGroups( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, I hate Map> so much, but I see this predates your change so leave it. I will probably send a followup shortly to change this to a Table.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. I didn't know about Table (never wrote Java at Google). I would be happy to fix this - please let me know if you'd like me to do this as part of this PR - but you probably have more context.
|
Thanks for the review! It's been a while since I made this change, and initially I intended it as a sketch - glad it can be merged. |
|
I'm merging this today. Thanks for your patience! The internal reviewer did have one request: can you send a separate PR to update the docs for exec groups (at https://github.com/bazelbuild/bazel/blob/master/site/docs/exec-groups.md) to mention the new functionality? |
Closes #13167. PiperOrigin-RevId: 361885312
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected.
With this change, the following becomes possible:
```
cc_test(
name = "my_test",
...,
exec_properties = {
"test.key": "value",
},
)
```
This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible:
```
platform(
name = "test_platform",
constraint_values = [":test_constraint"],
exec_properties = {
"test.key": "value",
},
)
cc_test(
name = "my_test",
...,
exec_compatible_with = [":test_constraint"],
)
```
This achieves the same in a more succinct way.
For related discussion, see PR #12719 by @ulfjack.
Closes #13110.
PiperOrigin-RevId: 361167318
Closes #13167. PiperOrigin-RevId: 361885312
This reverts commit 5f94563. Signed-off-by: Philipp Wollermann <[email protected]>
This reverts commit 5f94563. Signed-off-by: Philipp Wollermann <[email protected]>
Baseline: 37a429a Cherry picks: + a689d67: Use getRunfilesPath for run_under executable path generation. getRootRelativePath doesn't return a valid runfiles path for external source files anymore after the recent external source root change. Also, it won't work for external labels either once the --nolegacy_external_runfiles becomes default. This fixes issue #12545. + d90ec67: Fix NPE when coveragerunner is not set on the toolchain. + 8555789: Fix the classic query package-loading cutoff optimization with external workspaces. + 57672ac: Update turbine + bef4bbb: Update turbine + d113d74: Update turbine + 1489f0f: Support Scala3 .tasty files + 0d2d95c: Update to java_tools javac11 release 10.5 (#12647) + a9419f3: Fix common prefix for instrumentation filter + 84fadcf: Fix builds for filegroup targets with incompatible dependencies + e43825d: Revert "Remove --incompatible_blacklisted_protos_requires_proto_info" + 082d58d: Transform roots along with paths during output deletion. + e8835c1: AttributeContainer.Large now handles more than 127 attributes. + e1e8734: Add an env attribute to all test and binary rule classes + a87d7ed: Take no action to prefetch empty artifacts. + 3e969ff: Fix a couple of bugs with Incompatible Target Skipping + e667082: Pass --host_action_env to host options hostActionEnvironment attribute + 07400c0: Add --{no,}autodetect_server_javabase. + c833660: Only treat "env" and "env_inherit" attrs specially for native rules + 6a60b30: Fix coverage support when using default_java_toolchain. (#12801) + 4158a6f: Revert JacocoCoverage target to remote_java_tools_java_import and add a new target for remore_java_tools_filegroup. (#12813) + f6d30cf: Add windows_msvc back to conditions in bazel_tools. + 6b33bdb: Release 4.0.0 (2021-01-21) + 8811e27: Fix error message from getPrerequisites to not print internal details. + 27e15ad: Clean up ConfiguredTargetValueAccessor and ConfiguredTargetAccessor + e87feb8: Move getConfigConditions into ConfiguredTarget. + 34d9823: Change ConfiguredTargetQuery to use KeyedConfiguredTarget as a value. + 079bb7d: Clean up old dependencies that are unused since 34d98234324da83e93ba0d 5ef5702880d5ac7c5c. + e03cb63: Update bazelbuild/platforms to a current release. - Roll forward 0a4533420a3de467fd211d 7f925cf88e0cd5b76a with kythe fix. + 2eb1bf5: Update docs and tests to use the @platforms//:incompatible constraint + c71697c: Clarify test_suite behaviour in the Platforms docs + dfb70ea: Enable toolchain resolution for filegroup targets. + 24d0864: PlatformProviderUtils should ignore targets that don't have the needed + ba60c0b: ijar: fix manifest sections handling + 58bb42a: Revert "Switch to -fdebug-compilation-dir" + 57672ac: Update turbine + bef4bbb: Update turbine + d113d74: Update turbine + ad241fb: Allow cquery to filter out incompatible targets + 1782f0a: Patch grpc to fix cares selecting the wrong source when building for darwin_arm64 cpu. + 8f7bc2f: [1/3] Bump grpc to 1.33.1 to fix corruption when downloading CAS blobs + 848a517: [2/3] Bump grpc to 1.33.1 to fix corruption when downloading CAS blobs + 9b30172: [3/3] Bump grpc to 1.33.1 to fix corruption when downloading CAS blobs + 1e258d2: Allow exec groups to inherit from the rule or other exec groups. + d067669: Support execution constraints per exec group + f1e0d34: Clean up RuleContext to use a Table instead of a Map of Maps. + 8186fbb: Documentation for
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected.
With this change, the following becomes possible:
This will apply
{"key": "value"}for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible:This achieves the same in a more succinct way.
For related discussion, see PR #12719 by @ulfjack.