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

Introduce dl key to allow per-crate download links #10484

Closed
wants to merge 1 commit into from

Conversation

w4
Copy link

@w4 w4 commented Mar 18, 2022

This change allows the registry to override its own global dl key defined in the root config.json for each package version.

    // This optional field specifies the download link for this package
    // only, overriding the global `dl` link declared in the registry's
    // `config.toml`. The `{crate}` markers et al. are not interpolated
    // in this context.
    "dl": "https://crates.io/api/v1/crates/foo/0.1.0/download"

What does this PR try to resolve?

Some third-party 'generic' registries can't (or aren't willing to) support the global 'one dl for everything' API that Cargo currently exposes for registries. This change allows much more flexibility by allowing the registry to return unique dl links for each individual package version.

How should we test and review this PR?

Create a new registry with a mock dl in the config.json and set the real link in the package version JSON line instead.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 18, 2022
@w4 w4 force-pushed the dl-override-per-package branch 2 times, most recently from 025e843 to 9e10fe3 Compare March 18, 2022 01:54
@Eh2406
Copy link
Contributor

Eh2406 commented Mar 18, 2022

I am not yet speaking for the team. I do not know what design work the Cargo Team will need before this will be excepted. One thing this will need is to be added to documentation. That will need to include: "Where can it be set?" and "What can it be set to?"

@w4 w4 force-pushed the dl-override-per-package branch from 9e10fe3 to 195b689 Compare March 18, 2022 11:57
@w4
Copy link
Author

w4 commented Mar 18, 2022

Thanks @Eh2406, added some documentation around this configuration value to the book.

@w4 w4 force-pushed the dl-override-per-package branch from 195b689 to 819f138 Compare March 18, 2022 12:08
@bors
Copy link
Collaborator

bors commented Mar 24, 2022

☔ The latest upstream changes (presumably #10470) made this pull request unmergeable. Please resolve the merge conflicts.

This change allows the registry to override its own global `dl`
key defined in the root config.json for each package version.
@w4 w4 force-pushed the dl-override-per-package branch from 819f138 to 3fc27b2 Compare March 28, 2022 15:17
@w4
Copy link
Author

w4 commented Mar 28, 2022

Rebased, would appreciate your thoughts on this @ehuss, thanks in advance!

@arlosi
Copy link
Contributor

arlosi commented Mar 28, 2022

@w4 I'm curious if you have any more details on how this would be used.

If you could share, I'm interested in which 3rd party 'generic' registries are requesting this change? Is there a reason why the existing system of constructing download URLs isn't flexible enough? Why would some crates need to be downloaded from a different location?

Also, I'm working on implementing rust-lang/rfcs#3139 which adds authentication support for registries when downloading crates. With the RFC implemented, if the registry sets auth-required=true, the authentication token would be sent when downloading.

If each crate crate had an individually configurable download URL as requested here, it greatly increases the number of places the token could be sent. Would we still want the authentication token always sent? Would we need that to be configurable as well?

@w4
Copy link
Author

w4 commented Mar 28, 2022

Hey @arlosi,

GitLab's the one I'm dealing with, I did try to implement a change in their API to support the looking up crates by [group]/[sha256]/[cratename]-[crateversion].crate so we could have a single registry entry per group but they were adamant they want to stick with [group]/[repository]/[cratename]-[crateversion].crate for performance reasons which would require a registry per repository.

auth-required would definitely be nice to support here so we wouldn't have to embed auth tokens in each link, potentially in a similar vein to the placeholders that's currently used in the registry-wide dl. But if it's only header-based I'm not sure it needs to be configurable, unless there's need to serve crates from different locations (like S3/local) but that'll probably be better solved using a redirect server.

For context, here's what I'm working on: https://github.com/w4/gitlab-cargo-shim, I've not publicised it at all thus far as it's in a less-than-ideal state where either Cargo or GitLab needs to be patched.

Thanks

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 28, 2022

Two questions,

  1. What does GitLab mean by group?
  2. Will this still be needed when GitLab has first class support, or is it only needed for your shim?

@w4
Copy link
Author

w4 commented Mar 28, 2022

A group is essentially GitLab vernacular for what GitHub calls an organisation (ie. rust-lang).

It's very likely GitLab will need this, or the lookup by hash that their performance guys weren't happy with, for any usable first-class implementation.

Any other approach would be a pretty massive API overhaul which, looking at their other registry implementations, is unprecedented for them.

We've offered some man hours on their first-class endeavour fwiw.

@Eh2406
Copy link
Contributor

Eh2406 commented Mar 30, 2022

This will have interactions with lots of things. We talked it over at the Cargo Meeting yesterday, we did not think of any blockers, but did not feel confident we had enumerated all the concerns.
We think this will require an RFC, mostly as an opportunity for stakeholders to speak up.
That document will need to list what all of the alternatives are, and why this is better.
What registries will need this (GitHub registry?) and why? What to registries that will not need this, and why?
And so on.

@Eh2406
Copy link
Contributor

Eh2406 commented May 11, 2022

I am closing to remove this from the PR queue, this is not a condemnation of your code or of the design. After an RFC discusses the design, this code may very well be acceptable. You can start an RFC discussion on https://internals.rust-lang.org/

@Eh2406 Eh2406 closed this May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants