Skip to content
This repository has been archived by the owner on Jun 21, 2019. It is now read-only.

use cargo::sources to determine files to copy for vendoring #139

Merged
merged 1 commit into from Oct 31, 2018

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Oct 31, 2018

This change has the effect of honoring package.{include,exclude}, and it
also correctly handles vendoring from non-crates.io sources when the
vendored crate contains depended-upon crates.

Fixes #137.

@froydnj
Copy link
Contributor Author

froydnj commented Oct 31, 2018

There's no test for the "vendored crate contains depended-upon crates" scenario. (Note that this is different than workspaces!) The motivating case for that scenario was winapi-rs, but I wasn't sure how widely afield the tests were permitted to go in downloading random-ish crates from github. I can change that if random crates are permitted, or the repository could be forked to a not-going-away place and the tests can depend on that instead.

@froydnj
Copy link
Contributor Author

froydnj commented Oct 31, 2018

Hmph, the failure is:

    Updating git repository `https://github.com/alexcrichton/futures-rs`
   Vendoring futures v0.1.15 (https://github.com/alexcrichton/futures-rs?rev=03a0005cb6498e4330#03a0005c) (C:\Users\appveyor\.cargo\git\checkouts\futures-rs-a4f11d094efefb0a\03a0005) to \\?\C:\projects\cargo-vendor\target\tmp\test3\vendor\futures
error: failed to sync
Caused by:
  failed to copy over vendored sources for: futures v0.1.15 (https://github.com/alexcrichton/futures-rs?rev=03a0005cb6498e4330#03a0005c)
Caused by:
  The filename, directory name, or volume label syntax is incorrect. (os error 123)
thread 'git_only' panicked at 'not successful: exit code: 101', tests\vendor.rs:67:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.
---- git_simple stdout ----

And other failures are simply chaining off of that. :(

@alexcrichton
Copy link
Owner

Hm you may need to add some more .with_context(|_| ...) to figure out where that error is coming from, but I'd suspect the fs::copy. Want to do that and try to include the paths in the error message to see where it's coming from?

Other than that depending on git repos should be fine for these tests. They're not too critical to keep working 100% of the time and it's much easier to write tests against the network sources!

@froydnj
Copy link
Contributor Author

froydnj commented Oct 31, 2018

Hm you may need to add some more .with_context(|_| ...) to figure out where that error is coming from, but I'd suspect the fs::copy. Want to do that and try to include the paths in the error message to see where it's coming from?

Ah, I suppose it's possible we're doing the wrong thing on Windows somehow. We'll try adding chain_err as appropriate.

Other than that depending on git repos should be fine for these tests. They're not too critical to keep working 100% of the time and it's much easier to write tests against the network sources!

Thanks, test included for the winapi case now!

@froydnj
Copy link
Contributor Author

froydnj commented Oct 31, 2018

OK, so the failure is indeed in fs::copy--at least the one we were seeing!:

Caused by:
  failed to copy `C:/Users/appveyor/.cargo/git/checkouts/futures-rs-a4f11d094efefb0a/03a0005/benches/bilock.rs` to `\\?\C:\projects\cargo-vendor\target\tmp\test4\vendor\futures\benches/bilock.rs`

Is it complaining about the / vs. the \ used everywhere else in the destination path? Or did we botch the destination path somehow (the \\?\ prefix that the source doesn't have? I think given Microsoft's documentation on universal paths, it's a bit of both: \\?\-prefix names don't auto-convert slashes to backslashes. So we have to break the relative path into components, and join them to the destination path individually to make sure std sees the right pathname separators?

Copy link
Owner

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Hm ok interesting! I'm not sure where the \\?\ path comes from, although I suspect that the repository specifies the literal string benches/bilock.rs and probably becomes incompatible with UNC paths?

This may also be related to rust-lang/cargo#6198, but that issue hasn't seen a resolution yet AFAIK

src/main.rs Outdated Show resolved Hide resolved
This change has the effect of honoring package.{include,exclude}, and it
also correctly handles vendoring from non-crates.io sources when the
vendored crate contains depended-upon crates.

Fixes alexcrichton#137.
@froydnj
Copy link
Contributor Author

froydnj commented Oct 31, 2018

OK, we should be good to now once CI finishes!

@alexcrichton alexcrichton merged commit f03f126 into alexcrichton:master Oct 31, 2018
@alexcrichton
Copy link
Owner

👍 looks great!

Want a new version published for this as well?

@froydnj
Copy link
Contributor Author

froydnj commented Nov 1, 2018

A new version would be ideal, thank you!

@alexcrichton
Copy link
Owner

Ok done!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants