Use canonical paths for checking equality

[matklad]

No description provided.

[rust-highfive]

r? @alexcrichton

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

[matklad]

With current cargo, this invocation will fail with bar believes it is in workspace foo, but foo does not think so. This happens because paths are compared, and they look like foo/Cargo.toml and bar/../foo/Cargo.toml.

[matklad]

@alexcrichton this is a stop-gap solution. A better solution I have in mind is to introduce a CanonicalPath(PathBuf) newtype wrapper, which would enforce this properly. It should solve #3436 in a more principal way and I think it can even pave the way for #3273?

What do you think?

[matklad]

Hm, windows tests fail because canonicalize returns \\?\ flavor of paths on windows. I think it would be better just to implement CanonicalPath idea rather then hunt for each Path on case by case basis. Would you be ok with this solution, or do you have something better in mind?

[alexcrichton]

It sounds like we may not need literally canonical paths (e.g. a syscall) but perhaps just a normalization instead?

[alexcrichton]

In general I'm very wary of reinterpreting paths as it seems to always introduce a bug one way or another, so I'd be interested in slicing this down to as few normalized paths as possible if we can.

[matklad]

Yes, that's most reasonable! I've actually tried to find a path normalization crate, but it didn't occur to me that this function may already be present in Cargo :)

[matklad]

Hm, I think there's at least one plausible case where the normalization is not enough. If the filesystem is case-insensitive, paths in members and the one we get walking the tree up may have different case. We had a similar problem in IntelliJ-Rust, and solved it by properly using IntelliJ virtual file system, which abstracts this stuff more or less correctly. Not sure if this can be handled in Cargo without massively complicated implementation.

[alexcrichton]

@bors: r+

Looks good to me, thanks!

I feel like it's ok if Cargo doesn't dive that deeply into filesystem weirdness. At some point you can also just configure it a little differently and have it work out...

[bors]

📌 Commit 083da14 has been approved by alexcrichton

[bors]

⌛ Testing commit 083da14 with merge 7060ca1...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 7060ca1 to master...

[matklad]

@alexcrichton an unrelated question: am I correct that using / as path separators for relative paths in Cargo.toml is cross-platform, because Windows API converts / to \ automatically?