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.

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

Support large paths on windows #112

Closed
wants to merge 5 commits into from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jul 18, 2021

@rbtcollins
Copy link

The docs on https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation don't say anything about it failing on network servers, but I can see that https://docs.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-getfinalpathnamebyhandlea which is underneath canonicalize() does - I would not use canonicalize() here at all. There is a defined mapping from every path to an extended length path;

If !path.startswith("\"), prepend "\?", otherwise if not startswith "\?\UNC" prepend with "\?\UNC" (the second case is to deal with \server\share paths.

This has to be done before calling any APIs anyway, because you can't canonicalize an already-too-long path.

@bjorn3
Copy link
Member Author

bjorn3 commented Sep 21, 2021

Just prepending \\?\ isn't valid as it doesn't just increase the max path length, but also suppresses canonicalizations like converting forward slashes to backward slashes, thus making the usage of forward slashes invalid.

This has to be done before calling any APIs anyway, because you can't canonicalize an already-too-long path.

For the code written as is the canonicalization happens before the path gets too long at least when the rust build dir uses a short path like on CI.

If only windows 10 compatibility was necessary, I think the best option would be to use the manifest option that signals long path compatibility to disable the MAX_PATH limit for all paths.

@rbtcollins
Copy link

Yes, you need to fixup /, and perhaps foo/../ should be processed - but these can all be done safely in segment processing code; it is symlink resolving and junctions and so on that can't be done in a non-racy fashion, and shouldn't be - and isn't necessary.

canonicalize is an unneeded second syscall, and this seems to be in your inner loop, not at the root of the tree. These aren't free and will slowly eat up performance. I'd really think twice about doing this unless there is a correctness or safety issue present - and AFAICT there isn't one.

In rustup we canonicalize:

  • when doing symlink creation
  • opening a toolchain
  • getting a path out of Settings
  • printing a couple of errors

This means that all the paths we construct within a toolchain dir, for instance, are already in extended-path form.

But - we haven't actually sat down and made sure that someone running rustup with CARGO_HOME + RUSTUP_HOME set to deep directories will work - and I've checked - it likely won't :)) - OTOH remove_dir_all has been tested on long paths and does this canonicalize once strategy: https://github.com/XAMPPRocky/remove_dir_all/blob/c5c5d11d1634e3612b1841496b4b298f4230bea9/src/fs.rs#L81

@mati865
Copy link
Contributor

mati865 commented Nov 11, 2022

Is this still relevant after changes like rust-lang/rust#89174?

@bjorn3
Copy link
Member Author

bjorn3 commented Nov 11, 2022

I believe I tried this after that PR but still got the failure.

@bjorn3
Copy link
Member Author

bjorn3 commented May 28, 2024

In the end rust-lang/rust#81746 has landed without needing this.

@bjorn3 bjorn3 closed this May 28, 2024
@bjorn3 bjorn3 deleted the better_error_messages branch May 28, 2024 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants