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

fix(cli): realPath honors non-unc win32 path separator #6073

Closed
wants to merge 2 commits into from
Closed

fix(cli): realPath honors non-unc win32 path separator #6073

wants to merge 2 commits into from

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented Jun 3, 2020

Description:

This PR tries to fix path separator returned by realPah.

It looks like issue stemmed from rust-lang/rust#42869. Canonicalize on windows returns UNC path, stripping unc host (https://github.com/denoland/deno/blob/master/cli/ops/fs.rs?rgh-link-date=2020-06-02T04%3A57%3A06Z#L584) will leave rest of path do not honor win32 path separator. Given std canonicalize does not provide workaround yet, stripping UNC via drop-in replacement (gitlab.com/kornelski/dunce) seems the easiest way to fix instead of re-implementing UNC stripping logics.

It may possible using another crate may not be desired or other way to workaround may exist.

@CLAassistant
Copy link

CLAassistant commented Jun 3, 2020

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@bartlomieju bartlomieju added the cli related to cli/ dir label Jun 3, 2020
@ry ry requested a review from piscisaureus June 3, 2020 19:57
@piscisaureus
Copy link
Member

piscisaureus commented Jun 5, 2020

'dunce' definitely seems to be engineered very diligently and the current canonicalize situation is indeed terrible, so I'm leaning towards landing this. That said, I do have some comments, maybe other windows users can chip in with their opinion on things:

  • I don't know how I feel about dunce encoding all these dos peculiarities such as that files can't be named aux.txt etc (and thus it will still canonicalize to \\?\C:\x\y\z\aux.txt). Although it is true in practice that most programs can't open a file with that name, in node we used the much "dumber" strategy of just stripping \\?\ and I don't believe very many people had issues with it.
  • I remember from my node days that people that use subst didn't love it that realpath() changes the drive letter to the drive that contains the subst'ed directory. I think we might want to add an option (or even make it the default) to stay on the same drive if possible.
  • For a complete solution it would also have to fix up network paths. With dunce I still get this:
    fn main() {
      let s = r#"\\localhost\c$\windows"#;
      println!("{:?}", dunce::canonicalize(s).unwrap());
      // Outputs: "\\\\?\\UNC\\localhost\\c$\\Windows"
    }

@piscisaureus piscisaureus added windows Related to Windows platform user feedback wanted feedback from the community is desired labels Jun 5, 2020
@@ -577,12 +578,9 @@ fn op_realpath(
debug!("op_realpath {}", path.display());
// corresponds to the realpath on Unix and
// CreateFile and GetFinalPathNameByHandle on Windows
let realpath = std::fs::canonicalize(&path)?;
let mut realpath_str =
into_string(realpath.into_os_string())?.replace("\\", "/");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the possibly dumb question. Isn't the cause of the wrong path separator simply the replacement in this line? What is it for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nayeemrmn good catch, @piscisaureus PTAL

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what you guys think is wrong and what you expect.
I expect backslashes. So it was wrong and after this patch it is good.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piscisaureus Can we fix this by removing the \\ -> / replacement (and maybe adapting the trim statement below), instead of bringing in a new dependency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir user feedback wanted feedback from the community is desired windows Related to Windows platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

realPathSync returns paths with UNIX "/" separator on Windows
5 participants