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

alternative registry support #336

Merged
merged 3 commits into from
Oct 27, 2019
Merged

alternative registry support #336

merged 3 commits into from
Oct 27, 2019

Conversation

tofay
Copy link
Contributor

@tofay tofay commented Sep 26, 2019

This PR adds support for alternative registries.

The main change is updating the fetch::get_latest_dependency to optionally accept a registry URL. I'm using URLs not registry names, because cargo metadata returns the URL of the index whenever the dependency is being sourced from an alternative registry.

When adding dependencies from alternative registries, the user specifies a registry name which is then converted to the relevant URL. For this purpose, the cargo config parsing code in registry.rs is updated to handle registries other than crates-io.

W.r.t testing, I've only added tests for the CLI/toml-editing aspects. I've been successfully using cargo upgrade with my company's alternative registry, but I've not added tests that actually query an alternative registry, because the tests currently rely on the user's cargo config.
Is that acceptable? (Cargo itself has private test fixtures for working with alternative registries - this could be reused when/if cargo-edit is moved into cargo.)

Fixes #201
Fixes #339

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

First of all, thank you for the PR and sorry for the delay in response!

I've not added tests that actually query an alternative registry, because the tests currently rely on the user's cargo config.

It actually emulates what cargo does and starts with manifest_path/../.cargo/config, so we could create a test dir with a non-default config and use something like https://github.com/ordian/crates.io-index for an alternative registry.

Please rebase on master as well.

cargo-add gains a new registry flag.
cargo-upgrade gets any alternative registry URLs from cargo metadata.
@tofay
Copy link
Contributor Author

tofay commented Oct 26, 2019

Thanks for the review. I've rebased and added a test fixture that uses https://github.com/ordian/crates.io-index.

I had a couple of issues relating to cargo config:

  • I hit Cargo add fails if a replaced repository hasn't been initialized. #339, and worked around it for now with a cargo search command.
  • The cargo config probing differs slightly between cargo and cargo-edit. Cargo searches from the invocation directory whereas cargo-edit searches from the parent of the manifest path. I've made sure the alternative registry cargo upgrade tests run commands from the temporary test directories, so that both the cargo metadata command and cargo-edit can detect the test cargo config.

@tofay tofay requested a review from ordian October 27, 2019 06:26
Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Thank you for addressing the issues so quickly!

I hit #339,

Suggested a patch to resolve it.

The cargo config probing differs slightly between cargo and cargo-edit.

Hmm, this seems concerning. The ultimate fix is to merge cargo-edit with cargo. If you want to help with this, see this comment.

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

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

Good job!

bors r+

bors bot added a commit that referenced this pull request Oct 27, 2019
336: alternative registry support r=ordian a=tofay

This PR adds support for alternative registries.

The main change is updating the `fetch::get_latest_dependency` to optionally accept a registry URL. I'm using URLs not registry names, because `cargo metadata` returns the URL of the index whenever the dependency is being sourced from an alternative registry.

When adding dependencies from alternative registries, the user specifies a registry name which is then converted to the relevant URL. For this purpose, the cargo config parsing code in `registry.rs` is updated to handle registries other than crates-io.

W.r.t testing, I've only added tests for the CLI/toml-editing aspects. I've been successfully using `cargo upgrade` with my company's alternative registry, but I've not added tests that actually query an alternative registry, because the tests currently rely on the user's cargo config.
Is that acceptable? (Cargo itself has private test fixtures for working with alternative registries - this could be reused when/if cargo-edit is moved into cargo.)

Fixes #201
Fixes #339

Co-authored-by: Tom Fay <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 27, 2019

@bors bors bot merged commit aaeba3c into killercup:master Oct 27, 2019
@tofay tofay deleted the alt-registries branch October 27, 2019 15:43
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.

Cargo add fails if a replaced repository hasn't been initialized. Add support for alternate registries
2 participants