preserve crate whose src dir is in the local_dst dir

[mykmelez]

The fix for bug 1323557 was necessary but isn't sufficient to enable the workflow described in comment 17 on that bug: "Nowadays we should have enough support in Cargo that all you need to do is to add [patch.crates-io] pointing directly at the sources in third_party."

The issue is that cargo-vendor::sync() ignores all path-based crates when determining the set of added_crates, then it removes all existing_crates that aren't in added_crates, which causes it to remove the source dir of a crate whose [patches.crates-io] entry points to a source dir in third_party.

This change adds those source dirs to added_crates, so they don't get removed.

(I considered creating a separate collection, something like "ignored_crates," for them, since it's arguably inaccurate to describe them as "added." But added_crates isn't used for anything other than determining which existing_crates to remove, and these are arguably "added" by the developer, so it's sufficient and perhaps even optimal to collect them in added_crates.)

[alexcrichton]

Thanks for the PR! I've added a test to this PR and it's unfortunately failing, mind taking a look into why it's failing?

[mykmelez]

Thanks for the PR! I've added a test to this PR and it's unfortunately failing, mind taking a look into why it's failing?

local_dst can be (and is, in the test) a relative path, but the paths returned by pkg.source_id().url().to_file_path() are always absolute, so we need to make sure we compare them to an absolute variant of local_dst. 15c77c3 does that by converting local_dst to an absolute path via Path::canonicalize().

Note: the sync() impl later converts local_dst to an absolute path via config.cwd().join(local_dst). To reduce duplication, I changed that to reuse the previously-converted path.

[alexcrichton]

Oh I think windows CI may be failing now?

[mykmelez]

Oh I think windows CI may be failing now?

Hmm, indeed and the log shows a strange path in the source.vendored-sources section:

[source.vendored-sources]
directory = "\\\\?\\C:\\projects\\cargo-vendor\\target\\tmp\\test1\\vendor"

I'll build and debug on Windows.

[alexcrichton]

I think it's probably best to compare paths via canonical locations but paths serialized into configuration and such probably want to stick to their relative versions today

[mykmelez]

I think it's probably best to compare paths via canonical locations but paths serialized into configuration and such probably want to stick to their relative versions today

Right, that makes sense.

The issue turned out to be that Path::canonicalize() returns a UNC path, while Url::to_file_path() returns an absolute path that isn't a UNC path, and PathBuf::starts_with() apparently doesn't account for this by normalizing its arguments before comparing them to each other.

(See rust-lang/rust#42869 for other issues with Path::canonicalize() returning UNC paths.)

1ec0be4 works around the problem by canonicalizing pkg.source_id().url().to_file_path() before comparing it to canonical_local_dst. And 6a5747e reverts to serializing a non-canonical path into configuration.

[alexcrichton]

Awesome, thanks!

[mykmelez]

Thanks for merging, @alexcrichton! Any chance you can push an update for this change to crates.io so we can use it in mozilla-central?

[alexcrichton]

Sure thing, done now!