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

Allow vendoring multiple Cargo.locks #134

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sebastianblunt
Copy link

@sebastianblunt sebastianblunt commented Jun 7, 2019

To give people the option of maintaining a single vendor directory across multiple crates/workspaces.

@sebastianblunt
Copy link
Author

sebastianblunt commented Jun 8, 2019

I'm not sure this behaves correctly with --git. If two crates depend on the same version of a package, but the two dependencies aren't identical (if one or both are fetched via git), you'll get an error like

error: checksum for `autocfg v0.1.4` changed between lock files

this could be indicative of a few possible errors:

    * the lock file is corrupt
    * a replacement source in use (e.g., a mirror) returned a different checksum
    * the source itself may be corrupt in one way or another

unable to verify that `autocfg v0.1.4` is the same as when the lockfile was generated

This isn't unique to this PR though, for example the same behavior can be triggered using workspaces.

cargo-vendor seems to have a check for this:

error: failed to sync

Caused by:
  found duplicate version of package `autocfg v0.1.4` vendored from two sources:

	source 1: https://github.com/cuviper/autocfg#7ee9466a
	source 2: registry `https://github.com/rust-lang/crates.io-index`

Should we add that here as well?

@alexcrichton
Copy link
Collaborator

I'm sorry but I no longer have the time to maintain this repository. I don't have time to review this but if others are interested in maintaining this command I can transfer ownership!

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - I've taken over this project from Alex. I'm happy to review your PR, but it looks like you have an outstanding question regarding handling the case when there are conflicting sources for a crate version.

It would be great if you could give a better error message in this case, and by better I mean something that will make it easier for a user to solve the problem. For example, show which Cargo.lock files contained which sources for the conflicted crate. Further, if all the sources are identical then the error should explain the other possibilities - e.g. corruption.

Anyway, let me know when you're ready for me to review your changes and I'll have a look.

@sebastianblunt
Copy link
Author

Hi Chris, thanks for taking over.

So the error I quoted is not actually given by cargo-local-registry, it's given afterwards by cargo when trying to compile. cargo-local-registry currently gives no error at all on conflicting packages.

Multiple conflicting packages can already happen before my PR, my PR adds another avenue from which conflicting packages can occur (syncing two different Cargo.locks using different sources.)

Up to you whether you want to go ahead and review my changes, or you'd rather wait for a separate PR to add a check and warnings for conflicting packages.

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - yes I understand that cargo-local-registry doesn't detect the conflict. I think it should, although I get that you might not want to implement that checking to get your PR in.

Could you explain how conflicting packages can already happen without this PR? I'm sure you're right, just so that we have an example to make the discussion easier. Perhaps something to do with multiple registries. So a single .lock file declares two dependencies A and B where dependency A from registry 1 pulls in a transitive dependency T from registry 1 and and dependency B from registry 2 pulls in a transitive dependency T from registry 2. Is that what you mean?

In your comment on the 9th of June, you asked "cargo-vendor seems to have a check for this ... Should we add that here as well?", so it sounded like you might be prepared to add such a check. But from your comment yesterday, I'm not so sure. Would you be prepared to add one? If not, we can still put this PR in, there's just more potential for user confusion.

In terms of long term vision, I wonder whether the output registry directory should have subdirectories when there are multiple source (different registries, git repos etc.) and then the output from running cargo-local-registry would output a .cargo/config file that mapped the each of the source registries to the appropriate subdirectory. Perhaps then this gets around the whole problem with crate conflicts. I wonder what your PR would look like doing that? Is that something you'd be prepared to implement?

@sebastianblunt
Copy link
Author

sebastianblunt commented Aug 1, 2019

I wouldn't mind implementing the check for conflicting packages, it shouldn't be too hard.

Here's one way to trigger it right now:
./Cargo.toml:

[workspace]
members = [
	"bar",
	"foo",
]

./foo/Cargo.toml:

[package]
name = "foo"
version = "0.1.0"

[dependencies]
autocfg = "0.1.5"

./bar/Cargo.toml:

[package]
name = "bar"
version = "0.1.0"

[dependencies]
autocfg = { git = "https://github.com/cuviper/autocfg/", rev = "81ea9f3120f20d684fb97fe90f2848f137519dfc" }

and then fn main() {} in foo and bar's src/main.rs. Then running cargo-local-registry with --git will trigger the error.

The situation you describe sounds like it could also trigger it, but the dependency that pulls in the transitive different dependency would have to be specified as a --git dependency, because crates.io restricts adding dependencies from other sources.

Having different subdirectories would be really nice. Do you think that'd be feasible without having to modify cargo, by specifying multiple replacements in .cargo/config?

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - I strongly suspect that we could make multiple subdirectories work without modifying cargo.

A tricky bit is defining the name for the subdirectory for each source. It would likely have to contain all the information that defines the source (for git sources this might include a branch, tag or revision) and still be a valid directory name on all supported filesystems.

I think the situation I described with two registries has the potential for conflicting versions, even if a crate in crates.io can't depend on a crate from another source. If A comes from crates.io and B comes from patchedcrates.io registry (I just made that one up). A depends on T from crates.io and B depends on T from patchedcrates.io - conflict. In this situation, patchedcrates.io is in the same role as a git repo using a --git option.

Anyway, just decide what you'd like to do for now:
a) stick with the current PR
b) add some conflict checking
c) go full subdirectories!

It would be a shame to go with option a) - especially as you say you'd be happy to implement some checking.

@sebastianblunt
Copy link
Author

Let's go with b, I'll add conflict checking to this PR.

Then maybe we should open a separate issue for subdirectories? It would be a very convenient feature to have, and I suspect others may have input as well.

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - great!

I've created #153 to track the multiple subdirectories discussion.

I look forward to reviewing your PR when you're happy with it.

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - did you get anywhere with this? Just wondering whether we should close this PR.

@sebastianblunt
Copy link
Author

Sorry, I never did get around to starting on adding a check for conflicting dependencies. If anybody else wants to take up the work, please feel free, otherwise I'll start on it at some point and write so here.

@ChrisGreenaway
Copy link
Collaborator

@sebastianblunt - are you OK with closing this PR then? Feel free to open another.

@sebastianblunt
Copy link
Author

I still think it can stand on its own, but we can close it and then I can just reopen it later.

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

Successfully merging this pull request may close these issues.

None yet

3 participants