fixed workspaces issues due to non-canonical manifest path

[stefanhoelzl]

this PR fixes #7686

comparisons with the root manifest path failed even if paths were identical.
This was due to a comparison with a non-canonical root manifest path with the canonical one.

This PR canonicalizes the manifest path when a Workspace is created, so that all future comparison are using the canonical manifest path.

Before two identical path, just represented differently, where not equal e.g. a/../a != a
With this PR the comparison looks like this a == a, which is as expected equal

[rust-highfive]

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

[stefanhoelzl]

oh, looks like under windows the canonical path is a UNC path, see rust-lang/rust#42869
that's why the tests under windows are failing.

[alexcrichton]

Thanks for the PR! Unfortunately though this sort of user-space canonicalization is very tricky and we try to avoid it wherever we can. Can this same issue be solved with syscalls or some other form of path comparison which doesn't require manually interpreting the path?

[stefanhoelzl]

like I said fs::canonicalize does a normalization, but also creates UNC paths on windows.
There is this dunce crate which make a regular path out of UNC paths, but this would add another dependency.

I am not aware of other ways, but I am pretty new to rust, so maybe there is another solution

[ehuss]

dunce looks interesting, though it is a little concerning it is all user-space. Dealing with this properly is extraordinarily difficult, but I'm not sure if we need to aim for 100% perfection.

One of the fundamental questions is whether or not the normalization should follow symlinks. I'm not sure I have a good answer to that. Using symlinks in a workspace seems likely to be problematic.

Cargo has normalize_path, which is a somewhat simplified version. I suspect it is somewhat wrong in some cases, but again getting perfect is a monumental task. I would feel more comfortable replacing that with something that is known to be more correct.

One thing I've been curious about is whether or not GetFullPathNameW would be a good alternative to GetFinalPathNameByHandle.

[alexcrichton]

I think in general we've been bitten enough by any form of canonicalization that we want to avoid adding more canonicalization to Cargo. Can we perhaps solve this by not comparing paths? Is there something else we can do in Cargo?

[stefanhoelzl]

I was not aware of paths::normalize_path in cargo, but I tried it and it does the job, so my resolve_path method is not necessary, but we would still do the sanitation in user space.
Still would be the easiest fix and would not introduce some new canonicalization.

Another interesting crate could be same-file it compares files by comparing their inode-number on unix respectively the file_index on windows.

[alexcrichton]

Looking through some paths the normalization logic is actually already applied for --manifest-path arguments, so I think this may perhaps be only related to --path of the cargo install command? If that's the case can the fix be applied locally to just that one location to avoid repeated calls to normalization? Ideally we want the normalization to be as far away as possible as well to reduce implicit reliance on it within Cargo.

[bors]

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

[alexcrichton]

This has been here for quite some time now so I'm going to go ahead and close this, feel free to resubmit though!