[WIP] Use absolute_path instead of fs::canonicalize #3324
Conversation
| let current_dir_manifest = current_dir.join("Cargo.toml"); | ||
| let workspace_root_path = PathBuf::from(&metadata.workspace_root).canonicalize()?; | ||
| let workspace_root_path = absolute_path(PathBuf::from(&metadata.workspace_root))?; | ||
| let in_workspace_root = workspace_root_path == current_dir; | ||
|
|
||
| for package in metadata.packages { |
There was a problem hiding this comment.
based on the findings in https://github.com/rust-lang/rustfmt/pull/3309/files#diff-0f04fceb10cfdeda3716c7824a8dc2deR246, maybe you need to get the absolute_path of manifest_path:
absolute_path(PathBuf::from(&package.manifest_path)) == current_dir_manifestThere was a problem hiding this comment.
@topecongiro can we merge this PR with #3309 ? this would fix also @rivy 's comment #3324 (comment)
There was a problem hiding this comment.
I think this should be a bug against PathBuf.canonicalize(). Any conversion to UNC must use the absolute path. PathBuf is taking that responsibility and should do it itself.
Here, as a workaround, I think you should use both; canonicalize the absolute path...
let workspace_root_path = absolute_path(PathBuf::from(&metadata.workspace_root))?.canonicalize()?;There was a problem hiding this comment.
I expect a wrapper function (maybe just a passthru for non-windows OS) which executes this work-around would be a good way to proceed. And, I'd replace all uses of canonicalize() in the code base. A quick repo search shows only about about eight uses.
Not meaning to add more work, but any path comparisons should probably be reviewed for possibly needing canonicalizing of both sides. And I'm not sure where those might be scattered throughout the code base.
$0.02
Thanks for taking the time to review and repair this issue.
There was a problem hiding this comment.
review this in #3309 once this PR is merged.
|
This fix corrects the error that I've been having... Thanks. But the fix leaves the path that it uses ("C:\Users\Roy\OneDrive\Projects\rust\coreutils\src\factor....\mkmain.rs") in a form that might fail for longer paths (eg, over the MAX_PATH length [~260 characters]). In that instance, using an absolute path in the original extended form (\\?\...) would be the only way that I know of that allows successful use of the path. |
|
@topecongiro is there anything blocking this ? Would you like some help to get this over the finishing line ? |
|
Oh I completely forgot about this. Just now I tried to follow the instructions to test this but it doesn't compile anymore... |
|
@CasualX should be fine to test now, thanks! |
|
|
||
| // FIXME: This `MAX_PATH` may be valid only from Windows 10, version 1607. | ||
| // https://docs.microsoft.com/ja-jp/windows/desktop/FileIO/naming-a-file#paths | ||
| const MAX_PATH: usize = 32767; |
There was a problem hiding this comment.
I don't think this is correct.
From my reading of the documentation noted above, it looks like a MAX_PATH of 32000+ can only be depended on for Windows 10, for certain later versions, and only if the system/user has opted-in, by changing a registry entry. This, to me, means that it can never be depended on and should likely not be used.
MAX_PATH should be ~260 characters for normal paths (absolute or relative). For longer paths (up to 32000+ characters), the path would need to be in "extended" format which means absolute form prefixed with "\\?\".
There was a problem hiding this comment.
It seems like you cannot use \\?\ with relative path according to https://docs.microsoft.com/en-gb/windows/desktop/FileIO/naming-a-file#maximum-path-length-limitation
Because you cannot use the
\\?\prefix with a relative path, relative paths are always limited to a total of MAX_PATH characters.
There was a problem hiding this comment.
I tried to prefix the path with \\?\ and I get an error:
The specified path is invalid. (os error 161)
|
I'm on Windows and the build is still failing (appveyor has the error), the fix seems trivial but I've no idea how I can tweak this on my end. |
|
Currently I do not have a working Windows environment, so I am not able to fix this for a while, sorry. |
|
@topecongiro I've got a windows environment up and running. I will try to test your PR. |
|
got the same < The specified path is invalid. (os error 161) > error on windows,
windows could not recognize .. in path |
|
@cheblin Can you try the branch from #3590 ?
Looks like there is a backslash missing after |
|
Closing in favor of #3590. |
|
@topecongiro If you want I could create a branch within the rustfmt repo with for these changes, so that anyone can work on it more easily ? |
Closes #1873.
@BusyJay @CasualX @povilasb @rivy If you have time, could you please try this PR locally?
git clone https://github.com/topecongiro/rustfmt cd rustfmt git checkout absolute-path cargo build --release ./target/release/rustfmt path/to/file