Skip to content

Conversation

@quval
Copy link
Contributor

@quval quval commented Feb 25, 2021

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.

@google-cla
Copy link

google-cla bot commented Feb 25, 2021

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no label Feb 25, 2021
@google-cla
Copy link

google-cla bot commented Feb 25, 2021

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 @googlebot I consent. in this pull request.

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 cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@katre
Copy link
Collaborator

katre commented Feb 26, 2021

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?

@quval
Copy link
Contributor Author

quval commented Feb 26, 2021

Sure, will do (in fact, I believe this PR only includes those changes, but I originally based it on #12719).

@jin jin added the team-Configurability platforms, toolchains, cquery, select(), config transitions label Mar 1, 2021
quval added 3 commits March 1, 2021 22:17
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.
@quval quval force-pushed the test-on-target-platform branch from 5736c61 to da8952d Compare March 1, 2021 20:21
@google-cla google-cla bot added cla: yes and removed cla: no labels Mar 1, 2021
@quval quval marked this pull request as ready for review March 1, 2021 20:23
@quval
Copy link
Contributor Author

quval commented Mar 3, 2021

@katre: rebased and double-checked everything's in order. Thanks!

@katre
Copy link
Collaborator

katre commented Mar 3, 2021

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

@quval
Copy link
Contributor Author

quval commented Mar 3, 2021

Great, will do. I've been wondering where would tests actually go. Thank you!

@quval
Copy link
Contributor Author

quval commented Mar 3, 2021

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.

Copy link
Collaborator

@katre katre left a 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"
Copy link
Collaborator

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.

Copy link
Contributor Author

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(
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@quval
Copy link
Contributor Author

quval commented Mar 4, 2021

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.

@katre
Copy link
Collaborator

katre commented Mar 5, 2021

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?

@bazel-io bazel-io closed this in d067669 Mar 5, 2021
@quval quval deleted the test-on-target-platform branch March 6, 2021 17:01
quval added a commit to quval/bazel that referenced this pull request Mar 6, 2021
philwo pushed a commit that referenced this pull request Mar 15, 2021
Closes #13167.

PiperOrigin-RevId: 361885312
philwo pushed a commit that referenced this pull request Mar 15, 2021
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
philwo pushed a commit that referenced this pull request Mar 15, 2021
Closes #13167.

PiperOrigin-RevId: 361885312
philwo added a commit that referenced this pull request May 20, 2021
This reverts commit 5f94563.

Signed-off-by: Philipp Wollermann <[email protected]>
philwo added a commit that referenced this pull request May 20, 2021
This reverts commit 5f94563.

Signed-off-by: Philipp Wollermann <[email protected]>
bazel-io pushed a commit that referenced this pull request May 21, 2021
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