Use verbatim paths for process::Command if necessary#92519
Conversation
|
(rust-highfive has picked a reviewer for you, use r? to override) |
|
☔ The latest upstream changes (presumably #92580) made this pull request unmergeable. Please resolve the merge conflicts. |
c95249c to
2bfd6f8
Compare
|
Seems reasonable to me. @rfcbot merge |
|
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
@joshtriplett this can be merged now? |
2bfd6f8 to
93f627d
Compare
|
Rebased after #91182 was merged (which didn't cause a merge conflict but does break this PR). The first commit is the same as before. The second fixes |
|
@bors r+ |
|
📌 Commit 93f627d has been approved by |
… r=dtolnay Use verbatim paths for `process::Command` if necessary In rust-lang#89174, the standard library started using verbatim paths so longer paths are usable by default. However, `Command` was originally left out because of the way `CreateProcessW` was being called. This was changed as a side effect of rust-lang#87704 so now `Command` paths can be converted to verbatim too (if necessary).
Rollup of 6 pull requests Successful merges: - rust-lang#92519 (Use verbatim paths for `process::Command` if necessary) - rust-lang#92612 (Update stdlib for the l4re target) - rust-lang#92663 (Implement `Write for Cursor<[u8; N]>`, plus `A: Allocator` cursor support) - rust-lang#93263 (Consistently present absent stdio handles on Windows as NULL handles.) - rust-lang#93692 (keyword_docs: document use of `in` with `pub` keyword) - rust-lang#94984 (add `CStr` method that accepts any slice containing a nul-terminated string) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fix Windows nightly error caused by rust-lang/rust#92519
…ulacrum [beta] backports This backports/rolls up: * Quick fix for rust-lang#96223. rust-lang#96679 * [beta] Revert rust-lang#92519 on beta rust-lang#96556 * [beta] Clippy backport ICE/infinite loop fix rust-lang#96740 * Revert "Prefer projection candidates instead of param_env candidates for Sized predicates" rust-lang#96593
### Description
AsExecutablePathForCreateProcess shortens an executable's path with
GetShortPathNameW so it fits CreateProcessW's MAX_PATH-bound command
line, and currently fails outright when shortening can't bring it
under MAX_PATH.
This change adds a fallback: it launches the executable through
CreateProcessW's lpApplicationName, which is not subject to MAX_PATH,
in extended-length ("\\?\") form, keeping the original long path as
argv[0] in the command line.
Shortening is still attempted first, so the change is a pure superset
of the current behavior: paths that shorten today are launched
unchanged, and only the currently-failing ones take the fallback.
The fallback is limited to absolute, normalized paths to plain
executables; a batch file is not a valid lpApplicationName, as it
must be run through a command interpreter.
Verified at two levels: a util_test case that synthesizes an
unshortenable path, and a WindowsProcessesTest case that launches
cmd.exe from a >MAX_PATH directory of already-8dot3 components.
### Motivation
Fixes bazelbuild#19710.
Shortening fails in two situations, both handled here:
- 8dot3 short-name creation is disabled on the volume (common in
containers), so GetShortPathNameW is a no-op and a long runfiles
executable path stays over MAX_PATH;
- the path is nested so deeply that even fully 8dot3-shortened it
still overflows MAX_PATH.
The fix relies on "\\?\" bypassing MAX_PATH at the path-parsing
layer, independently of the LongPathsEnabled registry value and the
longPathAware manifest.
Prior art: Rust's std::process::Command passes a verbatim "\\?\" path
as lpApplicationName for a >MAX_PATH executable and special-cases
batch files the same way (rust-lang/rust#92519, rust-lang/rust#87704).
### Build API Changes
No
### Checklist
- [x] I have added tests for the new use cases (if any).
- [ ] I have updated the documentation (if applicable).
### Release Notes
RELNOTES: Fixed Bazel being unable to run a Windows executable whose
path is too long to shorten under MAX_PATH, e.g. when 8dot3 short
names are disabled (microsoft/Windows-Containers#507).
### Description `AsExecutablePathForCreateProcess` shortens an executable's path with `GetShortPathNameW` so it fits `CreateProcessW`'s `MAX_PATH`-bound command line, and currently fails outright when shortening can't bring it under `MAX_PATH`. This change adds a fallback: it launches the executable through `CreateProcessW`'s _lpApplicationName_, which is not subject to the `MAX_PATH` limit, in extended-length (`\\?\`) form. Supplying _lpApplicationName_ also lifts the `MAX_PATH` limit from the executable part of _lpCommandLine_, so the original path is kept there as `argv[0]`. Shortening is still attempted first, so the change is a pure superset of the current behavior: paths that shorten today are launched unchanged, and only the currently-failing ones take the fallback. The fallback is limited to absolute, normalized paths to plain executables: - a batch file is not a valid _lpApplicationName_ as it must be run through a command interpreter, - neither is a non-normalized path because its `.`/`..` components would survive unresolved in the extended-length path. The fix relies on `\\?\` bypassing `MAX_PATH` at the path-parsing layer, independently of the _LongPathsEnabled_ registry value and the _longPathAware_ manifest. ### Motivation Fixes bazelbuild#19710. Shortening fails in two situations, both handled here: - 8dot3 short-name creation is disabled on the volume (common in containers, microsoft/Windows-Containers#507), so `GetShortPathNameW` is a no-op and a long runfiles executable path stays over `MAX_PATH`, - the path is nested so deeply that even fully 8dot3-shortened it still overflows `MAX_PATH`. Prior art: Rust's `std::process::Command` passes a verbatim `\\?\` path as _lpApplicationName_ for a >`MAX_PATH` executable and special-cases batch files the same way (rust-lang/rust#87704, rust-lang/rust#92519). ### Build API Changes No ### Checklist - [x] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes RELNOTES: Fixed Bazel being unable to run a Windows executable whose path is too long to shorten under `MAX_PATH`, e.g. when 8dot3 short names are disabled (microsoft/Windows-Containers#507).
### Description `AsExecutablePathForCreateProcess` shortens an executable's path with `GetShortPathNameW` so it fits `CreateProcessW`'s `MAX_PATH`-bound command line, and currently fails outright when shortening can't bring it under `MAX_PATH`. This change adds a fallback: it launches the executable through `CreateProcessW`'s _lpApplicationName_, which is not subject to the `MAX_PATH` limit, in extended-length (`\\?\`) form. Supplying _lpApplicationName_ also lifts the `MAX_PATH` limit from the executable part of _lpCommandLine_, so the original path is kept there as `argv[0]`. Shortening is still attempted first, so the change is a pure superset of the current behavior: paths that shorten today are launched unchanged, and only the currently-failing ones take the fallback. The fallback is limited to absolute, normalized paths to plain executables: - a batch file is not a valid _lpApplicationName_ as it must be run through a command interpreter, - neither is a non-normalized path because its `.`/`..` components would survive unresolved in the extended-length path. The fix relies on `\\?\` bypassing `MAX_PATH` at the path-parsing layer, independently of the _LongPathsEnabled_ registry value and the _longPathAware_ manifest. ### Motivation Fixes bazelbuild#19710. Shortening fails in two situations, both handled here: - 8dot3 short-name creation is disabled on the volume (common in containers, microsoft/Windows-Containers#507), so `GetShortPathNameW` is a no-op and a long runfiles executable path stays over `MAX_PATH`, - the path is nested so deeply that even fully 8dot3-shortened it still overflows `MAX_PATH`. Prior art: Rust's `std::process::Command` passes a verbatim `\\?\` path as _lpApplicationName_ for a >`MAX_PATH` executable and special-cases batch files the same way (rust-lang/rust#87704, rust-lang/rust#92519). ### Build API Changes No ### Checklist - [x] I have added tests for the new use cases (if any). - [ ] I have updated the documentation (if applicable). ### Release Notes RELNOTES: Fixed Bazel being unable to run a Windows executable whose path is too long to shorten under `MAX_PATH`, e.g. when 8dot3 short names are disabled (microsoft/Windows-Containers#507).
In #89174, the standard library started using verbatim paths so longer paths are usable by default. However,
Commandwas originally left out because of the wayCreateProcessWwas being called. This was changed as a side effect of #87704 so nowCommandpaths can be converted to verbatim too (if necessary).