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

Add a new update-registry subcommand for use by scripts and third-party tools #5961

Closed
wants to merge 1 commit into from

Conversation

joshtriplett
Copy link
Member

Scripts and third-party tools often need to update the registry, and
they resort to invoking other cargo commands such as search to do so,
which can and does potentially break when cargo changes the semantics of
those commands.

Add a dedicated command to update the registry instead.

…ty tools

Scripts and third-party tools often need to update the registry, and
they resort to invoking other cargo commands such as `search` to do so,
which can and does potentially break when cargo changes the semantics of
those commands.

Add a dedicated command to update the registry instead.
@rust-highfive
Copy link

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@joshtriplett
Copy link
Member Author

This fixes #3377, as well as multiple requests that have arisen since then.

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Sep 2, 2018

This looks perfect for permanently fixing cargo install-update's registry updating, thanks Josh!

@dwijnand
Copy link
Member

dwijnand commented Sep 2, 2018

If you move "fixes #3377" to the PR description it'll close the issue on merge.

@joshtriplett
Copy link
Member Author

@dwijnand I'm aware, but every time I've done that it also seems to generate half a dozen extra mails as those commits move through various branches and trees.

@dwijnand
Copy link
Member

dwijnand commented Sep 2, 2018

I avoid them in commits message to avoid the spam, putting them only in PR descriptions. But looks like bors uses the PR description as the commit message, which can generate spam on multiple test runs (e.g for flaky tests or whatnot).

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 2, 2018

@dwijnand Yeah, that's the problem I've run into.

If you understand the problem a bit better, perhaps you could help report an issue on bors about it? (In any case, this is off-topic for this PR.)

@joshtriplett
Copy link
Member Author

Crater needs this as well.

@alexcrichton
Copy link
Member

I would personally prefer to land this as a nightly-only subcommand or somehow land it as unstable to start off with. I still feel unease that our story around this isn't well formed yet and am hesistant to expose some low level utilities which look tantalizingly like they may solve problems and end up being the wrong tool for the job.

In any case I wouldn't be comfortable landing this on stable yet and would prefer some sort of unstable feature gate, but unfortunately we don't have an existing method to gate entire subcommands so a new strategy would need to be devised.

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 4, 2018

Edit: Sorry for the inappropriate aggressive tone and the factually inaccurate assertions.

Well if we want to add things as unstable then #5621 (comment) should not have made serch run against an out of date registry without an unstable flag opting into that behaviour. In fact we already have a unstable flag to ask cargo commands to not update the index, so why did serch change?

@ehuss
Copy link
Contributor

ehuss commented Sep 4, 2018

serch run against an out of date registry

I'm a little confused about this. Doesn't search run against the crates.io API? If I'm reading things correctly, I believe it only fetches the registry to get the config.json file. Is that correct?

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 4, 2018

Ah that makes sense. Sorry for how strongly worded my last comment was.

@alexcrichton
Copy link
Member

Yes search was largely changed basically because a PR was proposed. It uses config.json to figure out where the HTTP API is, and while not updating each time can result in stale results (if config.json changed) the URL there hasn't changed since inception, so that hopefully won't bite too many folks!

@joshtriplett
Copy link
Member Author

@alexcrichton People have been counting on search to update the registry, and that change is already riding the trains towards stable. We need some stable way for people to update the registry before this lands in stable.

@alexcrichton
Copy link
Member

Crater is a special case, but why is there a reliance on updating the registry? Why would we consider it a breaking change to make search have a better UI by not always updating the registry?

I don't personally understand the need for updating the registry.

@joshtriplett
Copy link
Member Author

@alexcrichton People building third-party commands or scripts atop Cargo need a way to update the registry before accessing it. (Projects that link to the Cargo crate can do so directly via its API, but not all third-party commands link to the Cargo crate.)

I'm not suggesting that cargo search intended to have "update the registry" as a documented stable side effect, but in practice, various projects used it as such for lack of another interface to do so.

@nabijaczleweli
Copy link
Contributor

nabijaczleweli commented Sep 5, 2018

why is there a reliance on updating the registry?

Because cargo search is (used to be) the only subcommand that updates registry without doing anything else.

Why would we consider it a breaking change?

Because, for all intents and purposes, it's (a) the consensus way of doing it (without linking to cargo crate which doesn't actually work half the time), and (b) it was explicitly documented in what it did by printing it out every time.
And why is it the consensus way of updating the registry? Because nothing else does it, and this kind of red tape is what discouraged me from making issues about this (also, since, cargo search used to do it, it'd probably get closed because of that).

Tbph, this whole thing just feels like a big ole middle finger to maintainers – break a documented behaviour, thereby breaking dependent crates (for a small portion of users, admittedly), don't bump major, release the whole thing into nightly so it rolls out to everyone and stabilises in a couple weeks, refuse to consider a fix that will prevent it from breaking dependent crates for everyone. But hey, whatever, right?

@joshtriplett
Copy link
Member Author

@nabijaczleweli, I don't think anyone's interests would be well-served here by making this discussion more heated. Nobody here is trying to break people; there's no malice here. A well-intentioned change went into the search command, likely without knowledge that people were relying on one of its side-effects, and we need to deal with that appropriately.

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 5, 2018

We discussed this at length in the @rust-lang/cargo meeting.

There was some concern that adding an update-registry effectively implies some degree of stability of the registry itself, no matter how much we try to say otherwise. And long-term, there's a desire to provide the appropriate functionality for cargo to parse the registry itself and provide the appropriate information.

That said, there's a short-term need to address this now-missing functionality, which was available in stable and should continue to be in some form.

One proposal in the meeting was to use cargo install instead of cargo search. That does have the side effect of updating the registry, and (unlike cargo search) almost inherently has to. I observed, however, that this would either install a crate (not desirable) or generate an error and some noise (and in the process, also break the ability to distinguish errors in the registry update process).

Another proposal was to split this out into a separate crate that implements the equivalent functionality to update the registry. This would let people implement this without depending on the cargo library crate. However, duplicating that functionality outside of Cargo seems likely to lead to more breakage in the future, and in any case that code isn't written and would need to be. (Also, this would need to be both a library and binary crate.)

EDIT: There's also the approach taken by https://crates.io/crates/crates-index .

We didn't settle on a consensus solution for this. The one conclusion from the meeting was to bring the discussion back to this issue, mention the above alternatives, and invite further comment.

@nrc
Copy link
Member

nrc commented Sep 5, 2018

One proposal in the meeting was to use cargo install instead of cargo search

This is the approach currently used by Crater and is pretty much zero effort.

Another proposal was to split this out into a separate crate that implements the equivalent functionality to update the registry

This would take a little effort, but not much, since it is only really doing a git fetch, we wouldn't provide any functionality beyond 'update the registry'. It should be very easy to use and maintain. However, it does require some work to actually create (I imagine about on the same order of magnitude as this PR).

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 5, 2018

@nrc Creating that crate would take rather more effort than this PR, since it would duplicate a chunk of Cargo logic. (Leaving aside the issue of not wanting to duplicate that.)

@joshtriplett
Copy link
Member Author

joshtriplett commented Sep 5, 2018 via email

@dwijnand
Copy link
Member

dwijnand commented Sep 5, 2018

That's a noticeably worse user experience.

For clarity: anything outside of "break the ability to distinguish errors in the registry update process"?

@joshtriplett
Copy link
Member Author

@dwijnand Yes, it also suppresses progress output.

@nrc
Copy link
Member

nrc commented Sep 5, 2018

Creating that crate would take rather more effort than this PR, since it would duplicate a chunk of Cargo logic

If I understood correctly from the meeting, it would only need to git fetch the registry, it wouldn't need to duplicate any more of Cargo's logic. The only reason to put it into a crate rather than to get people to do that themselves is to encapsulate that part of Cargo's internals.

@alexcrichton
Copy link
Member

Are there comments from those who desired this feature that we could hear from? Right now the discussion after our in-person meeting it basically just reiterating everything that was said in the meeting. This is good for having things on record, but not so great for reaching a conclusion unfortunately!

FWIW I can list out the downsides to landing this PR as-is from my point of view, which will reiterate some of what's been said already:

  • This is providing a seemingly stable interface to a part of Cargo that we do not want to stabilized, the checkout of Cargo's own registry on your disk. We are providing this subcommand with very little documentation and guarantees about how it can be used, and as a result it's almost entirely a bolted-on-feature for (what appears to be only?) cargo-update. We do not want to stabilize Cargo's checkout on disk, nor do we want to encourage usage of Cargo's checkout on disk.
  • An "ideal" solution would look like a crate on crates.io which manages its own checkout of the index. This crate does not exist today, however, and would take work to create. The format of the registry is effectively stable at this point so this should be a fine crate to publish. I do not consider the fact that then you'd have two indexes (one for Cargo and one for this crate) to be enough of a downside to outweigh the previous point.
  • Small though it may be this subcommand is adding new maintenance burden on Cargo. We now have to add tests for this and keep it running. We need to field bug reports. We need to field users's confusion about this subcommand when it doesn't fit their own needs, etc etc.

A short term solution, already mentioned, is cargo install lazy_static (or any other crate guaranteed to not have a binary, perhaps one you own). The downside of this is indeed that error reporting isn't as easy as with cargo search foo which updated the index, but I do not consider this downside to outweigh the first point above.

All in all I see a short term solution that while not a 100% match works well enough (we do not want anyone to be using cargo's index checkout anyway) and a long term solution also exists and largely just requires someone to put in the effort to make it happen.

@dwijnand
Copy link
Member

How viable is the option of just subsuming cargo-update? Does that make any sense?

@alexcrichton
Copy link
Member

It's possible yeah but it's in the realm of "not trivial to do" as it likely requires an RFC

@nabijaczleweli
Copy link
Contributor

As previously mentioned by @Eh2406 on Discord, the only thing one really need replicate is git fetch refs/heads/master:refs/remotes/origin/master, so I'll just do that internally for the time being (seeing as you don't want to give people an official API for that, which is a very good point).
Might end up making a/the cargo registry crate, later, too, wouldn't be the first time :Р

Thanks, Alex! 💜

@alexcrichton
Copy link
Member

Ok thanks for the info @nabijaczleweli! @joshtriplett would you still like to merge this PR in light of that information?

@joshtriplett
Copy link
Member Author

@nabijaczleweli If that works for you, then that's fine. I'd certainly like to have that wrapped in a crate.

(I'd also suggest that we update the checked-out version, except it seems that cargo hasn't updated that in a year...)

@Eh2406
Copy link
Contributor

Eh2406 commented Sep 11, 2018

@nabijaczleweli If you do end up spinning a exterior cargo registry crate please cc me. I don't think the cargo team wants to officially support it, but I would be willing to do my best to ensure that the outside crate was up to date / aware of changes from within cargo.

Also there is crates/crates-index that is almost what @alexcrichton described as the "ideal" solution. It is also not officially supported, but used by crater. Maybe with some love this could be made two work.

In the distant future I'd love to see cargo spinning out the parts dealing with the registry into a separate crate with a fairly stable API, (or growing one of the above mentioned into it.) and have cargo use it. But that is going to be work, and I am not volunteering to leed it at this time. :-P

@nabijaczleweli
Copy link
Contributor

@alexcrichton I, personally, am pro-close on this now.

@joshtriplett Yeah, that seems to be the superiour solution in the long run, cheers for putting in this amount of work&time into it.

@Eh2406 Will do, and thanks for that link!

nabijaczleweli added a commit to nabijaczleweli/cargo-update that referenced this pull request Sep 13, 2018
…which has stopped supporting this

Does, effectively, a
    git fetch https://github.com/rust-lang/crates.io-index \
              refs/heads/master:refs/remotes/origin/master
like the original code:
    https://github.com/rust-lang/cargo/blob/1ee1ef0ea7ab47d657ca675e3b1bd2fcd68b5aab/src/cargo/sources/registry/remote.rs#L204

Big thanks to @Eh2406 for the original implementation,
              @joshtriplett for help and bastioning,
          and @alexcrichton for putting up with me.

Closes #93
Ref: rust-lang/cargo#3377
Ref: rust-lang/cargo#5961
@alexcrichton
Copy link
Member

I'm going to close this as it's been quiet for quite some time and I believe the urgent issue was fixed

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.

8 participants