-
Notifications
You must be signed in to change notification settings - Fork 4.4k
Add execution_requirements to ctx.actions.write
#28335
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
Since Bazel's only `FileWriteStrategy` doesn't execute a spawn, this can currently only affect the path mapping behavior of the action when supplying `Args` as content. This will make it possible to opt C++ rules into path mapping without any command line flags beyond `--experimental_output_paths=strip` in future PRs. Work towards bazelbuild#27732 Work towards bazelbuild#27591
|
@hvadehra I would have assigned Ivo for review in the past - who would be the right person for rules API changes nowadays? |
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.
Code Review
This pull request adds an execution_requirements parameter to ctx.actions.write. This allows passing execution information, which for now is mainly used to enable path mapping for actions that write parameter files (Args content). The changes to the Starlark API and the action factory implementation look correct and are accompanied by a new test. I've pointed out a potential issue where ParameterFileWriteAction doesn't persist all execution requirements, which could be surprising for users inspecting the action.
bazel/site/en/contribute/maintainers-guide.md Lines 189 to 190 in cc1e0a6
|
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.
Nice!
|
@bazel-io fork 9.0.0 |
| doc = | ||
| "Information for scheduling the action. See " | ||
| + "<a href=\"${link common-definitions#common.tags}\">tags</a> " | ||
| + "for useful keys."), |
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 realize that this is our existing standard doc for execution_requirements; however, it is a bit confusing to me, given that tags in https://bazel.build/reference/be/common-definitions#common.tags is described as list of string, whereas here we have a dict. Do we have any better documentation for execution_requirements that we could point to? Or that we could write?
Since Bazel's only `FileWriteStrategy` doesn't execute a spawn, this can currently only affect the path mapping behavior of the action when supplying `Args` as content. This will make it possible to opt C++ rules into path mapping without any command line flags beyond `--experimental_output_paths=strip` in future PRs. Work towards bazelbuild#27732 Work towards bazelbuild#27591 Closes bazelbuild#28335. PiperOrigin-RevId: 859069420 Change-Id: I73db26fb1f37303f49931ab19cd12575536607fb
Since Bazel's only
FileWriteStrategydoesn't execute a spawn, this can currently only affect the path mapping behavior of the action when supplyingArgsas content. This will make it possible to opt C++ rules into path mapping without any command line flags beyond--experimental_output_paths=stripin future PRs.Work towards #27732
Work towards #27591