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.

Sign up for GitHub

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

Jump to bottom

Windows: Allow running processes whose path exceeds the legacy MAX_PATH #86406

Closed
wants to merge 2 commits into from
Closed

Windows: Allow running processes whose path exceeds the legacy MAX_PATH #86406

wants to merge 2 commits into from

Conversation

ChrisDenton
Copy link
Member

On Windows Command currently fails when run using paths longer than the legacy MAX_PATH. This PR allows running such executables while leaving the behaviour of shorter paths unchanged.

This does not risk changing the behaviour of currently working code. It only allows something to work that would previously fail.

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 17, 2021
@ChrisDenton
Copy link
Member Author

ChrisDenton commented Jun 22, 2021 •
edited
Loading

It was pointed out to me that my previous implementation meant paths would have slightly different behaviours, depending only on the length, because of the way .exe is, or isn't, appended after being passed to CreateProcessW. For the sake of consistency I've unified long and short path handling.

Note that CreateProcessW has slightly weird rules here and Rust could certainly use a simpler strategy. However I'd rather not do that as a side effect of this change. So I've opted to maintain the existing behaviour for now.

@Mark-Simulacrum
Copy link
Member

r? @joshtriplett perhaps? I'm not sure if we have anyone with close enough familiarity or knowledge of Windows primitives to review this, but I don't think I'm the right person.

@jyn514
Copy link
Member

jyn514 commented Jul 8, 2021

Cc @retep998

@bors
Copy link
Contributor

bors commented Jul 9, 2021

☔ The latest upstream changes (presumably #85832) made this pull request unmergeable. Please resolve the merge conflicts.

@ChrisDenton
Parse a Windows file name without normalization
3ea37e4
@ChrisDenton
Windows: Allow running processes whose path exceeds the legacy MAX_PATH
511c708 
Previously `Command` would simply fail when run using long paths.

This also implements the `CreateProcessW` rules for appending `.exe`. This is to ensure both backwards compatibility and consistent behaviour between both long and short paths. However, this could be changed (and hopefully simplified) in the future.
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 25, 2021
@ChrisDenton
Copy link
Member Author

I now minded to close this PR in favour of doing a bigger overhaul of how Command looks for executables on Windows. On the internals forum there seems (so far) to be consensus that it should only look in PATH. Doing that manually would make this unnecessary.

@ChrisDenton ChrisDenton closed this Aug 2, 2021
@ehuss ehuss mentioned this pull request Aug 6, 2021
Open
@ChrisDenton ChrisDenton deleted the long-path-command branch October 7, 2022 00:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@joshtriplett joshtriplett

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ChrisDenton @rust-highfive @Mark-Simulacrum @jyn514 @bors @joshtriplett @JohnCSimon