feat: Enable some path-mapping support in common rules#1217
feat: Enable some path-mapping support in common rules#1217jbedard merged 1 commit intobazel-contrib:mainfrom
Conversation
dzbarsky
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dzbarsky
force-pushed
the
zbarsky/path-mapping
branch
from
f254157 to
b18b770
Compare
|
dzbarsky
force-pushed
the
zbarsky/path-mapping
branch
from
b18b770 to
5b5ba57
Compare
| # Sandboxing for this action is wasteful since there is a 1:1 mapping of input file/directory to | ||
| # output file/directory so little room for non-hermetic inputs to sneak in to the execution. | ||
| "no-sandbox": "1", | ||
| "supports-path-mapping": "1", |
There was a problem hiding this comment.
With sandboxing disabled path mapping will only work with remote execution. Is skipping the sandbox for these actions really worth it? With so few inputs there should be very little overhead.
There was a problem hiding this comment.
Oh. That's a good point. I don't think disabling sandbox is better than avoiding rerunning these actions when changing config...
This upgrade includes a Windows compatibility fix for batch file line endings (bazel-contrib/bazel-lib#1222), which resolves a cmd parsing bug where GOTO/CALL to labels fails when batch files use non CRLF-only line endings and a label crosses a 512-byte boundary: ```diff $ bazel test //deps/openssl3:module_consistency_test [...] ==================== Test output for //deps/openssl3:module_consistency_test: -The system cannot find the batch label specified - compare_files +compare_files +FAIL: files "C:\[...]\bazel-out\[...]\deps\openssl3\openssl3.MODULE.bazel.new" and "C:\[workspace]\deps\openssl3\openssl3.MODULE.bazel" differ. + + +@@//deps/openssl3:openssl3.MODULE.bazel is out of date. To update this file, run: + + bazel run //deps/openssl3:module_consistency +To see differences run: + + diff "C:\[...]\bazel-out\[...]\deps\openssl3\openssl3.MODULE.bazel.new" "C:\[workspace]\deps\openssl3\openssl3.MODULE.bazel" + ================================================================================ ``` Because this now surfaces we'd been generating `*.MODULE.bazel` with OS-specific newlines, the present change fixes it by always issuing UNIX-style newlines: ```sh $ bazel test //deps/openssl3:module_consistency_test [...] //deps/openssl3:module_consistency_test PASSED in 0.4s ``` `bazel_lib` 3.1.1 comes with further improvements, like: - bazel-contrib/bazel-lib#1217 - bazel-contrib/bazel-lib#1220 - bazel-contrib/bazel-lib#1232
### What does this PR do? This upgrade includes a Windows compatibility fix for batch file line endings (bazel-contrib/bazel-lib#1222), which resolves a cmd parsing bug where GOTO/CALL to labels fails when batch files use non CRLF-only line endings and a label crosses a 512-byte boundary: ```diff > bazel test //deps/openssl3:module_consistency_test [...] ==================== Test output for //deps/openssl3:module_consistency_test: -The system cannot find the batch label specified - compare_files +compare_files +FAIL: files "C:\[...]\bazel-out\[...]\deps\openssl3\openssl3.MODULE.bazel.new" and "C:\[workspace]\deps\openssl3\openssl3.MODULE.bazel" differ. + + +@@//deps/openssl3:openssl3.MODULE.bazel is out of date. To update this file, run: + + bazel run //deps/openssl3:module_consistency +To see differences run: + + diff "C:\[...]\bazel-out\[...]\deps\openssl3\openssl3.MODULE.bazel.new" "C:\[workspace]\deps\openssl3\openssl3.MODULE.bazel" + ================================================================================ ``` Because this now surfaces we'd been generating `*.MODULE.bazel` with OS-specific newlines, the present change fixes it by always issuing UNIX-style newlines: ``` > bazel test //deps/openssl3:module_consistency_test [...] //deps/openssl3:module_consistency_test PASSED in 0.4s ``` ### Motivation Keep up to date and better support colleagues working on Windows. ### Describe how you validated your changes Locally for the time being because we still have very limited test coverage due to #44455 being disputed. ### Additional Notes `bazel_lib` 3.1.0/3.1.1 comes with further improvements, like: - bazel-contrib/bazel-lib#1217 - bazel-contrib/bazel-lib#1220 - bazel-contrib/bazel-lib#1232 Co-authored-by: regis.desgroppes <[email protected]>
#1217 unconditionally enabled support for path mapping for rules that may use location expansion or hardcode paths in other ways. Instead, only enable it when it is clearly safe to do so. Verified manually by running `bazel test //... --experimental_output_paths=strip`. Work towards cerisier/toolchains_llvm_bootstrapped#334
3 participants
No description provided.