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

"test" exec_group is not present on starlark defined rules #13986

Closed
djmarcin opened this issue Sep 13, 2021 · 5 comments
Closed

"test" exec_group is not present on starlark defined rules #13986

djmarcin opened this issue Sep 13, 2021 · 5 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug

Comments

@djmarcin
Copy link

Description of the problem / feature request:

The "test" exec_group added in 0b3a5aa does not work for starlark defined rules. It appears to only work for native rules.

Feature requests: what underlying problem are you trying to solve with this feature?

All TestRunner actions should have the "test" exec_group

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

An example repo where I have been playing with remote exec & exec_groups is https://github.com/djmarcin/bazel-remote-exec, but the easiest way to reproduce the bug is to attempt to set the "test" exec group on a rust_test rule directly and observe the following error:

ERROR: Analysis of target '//:hello_rs_test' failed; build aborted: Tried to set properties for non-existent exec groups: test.

What operating system are you running Bazel on?

Linux, macOS

What's the output of bazel info release?

release 4.2.1
or
release 5.0.0-pre.20210831.2

If bazel info release returns "development version" or "(@non-git)", tell us how you built Bazel.

n/a

What's the output of git remote get-url origin ; git rev-parse master ; git rev-parse HEAD ?

n/a

Have you found anything relevant by searching the web?

The bazel documentation seems to indicate that this ought to work https://docs.bazel.build/versions/main/exec-groups.html#execution-groups-for-native-rules

Any other information, logs, or outputs that you want to share?

Issue filed as a result of discussion at #13110 (comment)
@katre

@djmarcin
Copy link
Author

In addition to the analysis error, the execution log from --execution_log_json_file= indicates that setting the exec_properties on the platform does not propagate to these actions unless the rules_rust rust_test is augmented to declare the "test" exec group explicitly, as mentioned on the bazel slack here: https://bazelbuild.slack.com/archives/CA31HN1T3/p1631384272260600

@katre
Copy link
Member

katre commented Sep 13, 2021

Thanks, this is definitely an oversight in the implementation.

@katre katre added P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug labels Sep 13, 2021
@quval
Copy link
Contributor

quval commented Sep 21, 2021

Since I've started to draft a patch for this back when @djmarcin first posted a comment on #13110, I thought I might send it as a PR to make sure this makes the cut into Bazel 5. Hope this is all right.

@katre
Copy link
Member

katre commented Sep 22, 2021

Thanks very much! I've been overwhelmed with other work and hadn't been able to tackle this, but your approach is exactly what I would have done.

@quval
Copy link
Contributor

quval commented Sep 22, 2021

Glad this helps! Thank you for the prompt feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Configurability platforms, toolchains, cquery, select(), config transitions type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants