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

Include the registry field the index to support multiple registries #172

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

Conversation

jarhodes314
Copy link

When depending on a crate from git rather than crates.io, any transitive dependencies are listed as coming from the git repository. As a result, any transitive dependencies from crates.io via this repository cannot be reconciled with direct dependencies on the same crates. This then causes errors as cargo treats these dependencies as two separate crates with the same name.

This change adds support for the deps.registry field in the index to mark that dependencies originate from a different registry where relevant.

@dhovart
Copy link
Owner

dhovart commented Aug 3, 2022

Thanks for the PR. Tests just need to be adapted now that we have a more recent cargo version that supports namespaced features.

@jarhodes314
Copy link
Author

I've now fixed the merge conflicts in the tests.

@dhovart
Copy link
Owner

dhovart commented Aug 4, 2022

Thank you! It would be nice to have a test case covering this as well, do you think you can provide that ?
I can also add one later if you want.

@jarhodes314
Copy link
Author

I can try and add one, but I suspect the only simple way of testing multi registry support is to depend an a crate in a git repository that itself depends on crates from crates.io. I didn't focus on adding tests to this specifically as I needed to test the results of running this against my repos anyway, and a new test is likely more complicated than the added functionality. But I agree it's important to test that case.

To be honest, even with these changes, git dependencies are a bit of a mess. Once a dependency is coming from a registry (local or remote), cargo requires a checksum to be present in the index before unpacking the crate. This isn't the case with git dependencies, they currently have a checksum of null when downloaded in the local registry. This wasn't a problem for me as I already had a script wrapping cargo local-registry that could retrofit the sha256 hashes cargo uses to the downloaded index, but cargo local-registry should probably perform this itself as it's required.

I should probably do some further work to complete that functionality, and add a test where the registry field is non-null. I'll update you when this is done.,

@jarhodes314
Copy link
Author

I've now added the missing functionality. This involves running generate_lockfile after removing any source replacement in the cargo config (required to remove any manually added checksums before resolving packages) and generating the checksum manually if it doesn't already exist. I've also added a test which mimics the existing tests, but contains a non-null value in the newly added registry field.

@jarhodes314
Copy link
Author

I've created rust-lang/cargo#10939 since I don't think it's intentional behaviour that cargo requires a checksum for git dependencies. Therefore, I think this should not be merged at least until it has been determined whether that actually is a bug in cargo, at which point these changes could be partially reverted to not encompass checksums.

@dhovart
Copy link
Owner

dhovart commented Aug 5, 2022

Awesome, thank you. Subscribing to the issue you created.

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