-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Allow actions to opt into path mapping via execution requirements #19717
Conversation
cb9dc94
to
3407cec
Compare
@fmeum I just realized the test wasn't included in the last merge. The patching tool failed and I had to manually resolve the differences and missed the test. Sorry about that. I saw a few different cls, could you please share here their priorities and the order I should be reviewing them in? THanks! |
src/main/java/com/google/devtools/build/lib/analysis/actions/PathMappers.java
Outdated
Show resolved
Hide resolved
OutputPathsMode outputPathsMode, String mnemonic, Map<String, String> executionInfo) { | ||
if (outputPathsMode == OutputPathsMode.STRIP | ||
&& (SUPPORTED_MNEMONICS.contains(mnemonic) | ||
|| executionInfo.containsKey(ExecutionRequirements.SUPPORTS_PATH_MAPPING))) { |
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.
@fmeum Just thinking out loud here, I wonder if we should only allow the opt-in behavior for Bazel only because within google, we probably don't want users to flip path mapping on and off as it would cause forking (mapped and unmapped paths) and potentially harm caching? @gregestren WDYT?
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.
Sure, I could see that make sense. I will leave that up to you, happy to add this here.
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.
If doing so isn't too much work, can we please add the check then?
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 thought I could use isThisBazel
, but that is only available in tests. I don't see an easy and clean way to tell apart Bazel from Blaze. I could potentially look at the execpath of the output artifact of the action, but that seems hacky. Are you aware of a better way to implement the check?
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 thought there was something similar to JavaClassPathMode.BAZEL
but i don't think there is. I can check with others tomorrow. This shouldn't block this pr though. I will work on the import and work on the next one. Thanks!
3407cec
to
fb56b64
Compare
This provides a simple and backwards compatible way for actions to experiment with path mapping: * rules authors can add `supports-path-mapping` to the `execution_requirements` of a Starlark action * end users can use tags and `--modify_execution_info` to opt individual targets or actions in and out of path mapping. Also includes StrippingPathMapperTest, which wasn't included in the merge of bazelbuild#18098, with an additional test for a Starlark rule that opts in via the mechanisms described above.
Thanks for clarifying. We do need all of these pr merge for path mappings to work in bazel right? |
Yes, that's correct. #19723 isn't required for just Java support though, only for good Starlark support. |
Drive-by comment; apologies if this makes no sense. Now that we're adding an execution requirement, should we consider explicitly tagging the Java actions with it instead of making a decision based on the action mnemonic? Mnemonics are not unique and this opens us up to future cleanup work (i.e., someone reuses a mnemonic, gets path mapping behavior, and then we can't take it back because it's a breaking change). |
This makes a lot of sense and is a very logical cleanup step, it's just something that a Googler will have to do as I can't test the change and probably don't have visibility into all affected rules. |
src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java
Show resolved
Hide resolved
src/test/java/com/google/devtools/build/lib/analysis/actions/StrippingPathMapperTest.java
Outdated
Show resolved
Hide resolved
fb56b64
to
40a94e2
Compare
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.
updated commit looks good. Internal tests are passing
This provides a simple and backwards compatible way for actions to experiment with path mapping:
supports-path-mapping
to theexecution_requirements
of a Starlark action--modify_execution_info
to opt individual targets or actions in and out of path mapping.Also includes
StrippingPathMapperTest
, which wasn't included in the merge of #18098, with an additional test for a Starlark rule that opts in via the mechanisms described above.Closes #19526
Work towards #6526