Skip to content

Cargo should skip crates.io check with --locked and all dependencies already resolved #10984

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

Open
mqudsi opened this issue Aug 12, 2022 · 9 comments
Labels
A-yanked Area: yanked dependencies Command-install S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@mqudsi
Copy link

mqudsi commented Aug 12, 2022

Last week there was a temporary outage of crates.io and my build requests were all hanging on "Updating crates.io index" even though they were subsequent invocations of the same cargo command, seconds apart, with --locked specified, e.g.

> cargo install --locked --path .
> cargo install --locked --path .

If all the dependencies in Cargo.toml are in Cargo.lock and Cargo.lock is itself in a fully consistent state with no missing dependencies, cargo install --locked should be equivalent to cargo install --offline --locked (which is what I ended up using). (Please let me know if I'm missing something that cargo needs internet access for if all dependencies are present and account for.)

I would also argue that there should be some sort of recency check before cargo install forces an update of the crates.io index, regardless of whether or not --locked was specified. If all dependencies are present and account for and the lock file is in a consistent state and all integrity checks pass, there's no reason for to back-to-back invocations of cargo install --path ... to hit the crates.io server (slowing down the effective/observed compile time, generating unnecessary load on the crates.io server, and raising privacy concerns).

Experienced with cargo 1.63.0-nightly (8d42b0e 2022-06-17)

@rustbot label +T-cargo +A-incr-comp +I-compiletime

@mqudsi mqudsi added the C-bug Category: bug label Aug 12, 2022
@rustbot rustbot added the T-cargo Team: Cargo label Aug 12, 2022
@ehuss ehuss removed T-cargo Team: Cargo C-bug Category: bug labels Aug 12, 2022
@ehuss ehuss transferred this issue from rust-lang/rust Aug 12, 2022
@rustbot

This comment was marked as resolved.

1 similar comment
@rustbot

This comment was marked as resolved.

@ehuss
Copy link
Contributor

ehuss commented Aug 12, 2022

Transferred to the cargo repo.

(Please let me know if I'm missing something that cargo needs internet access for if all dependencies are present and account for.)

It is checking if any dependencies have been yanked.

I'm not sure what might be a better way to handle that. At least it could provide a better error message, to perhaps suggest using --offline.

Copying the current error message:

    Updating crates.io index
warning: spurious network error (2 tries remaining): failed to connect to github.com: Connection refused; class=Os (2)
warning: spurious network error (1 tries remaining): failed to connect to github.com: Connection refused; class=Os (2)
error: failed to fetch `https://github.com/rust-lang/crates.io-index`

Caused by:
  failed to connect to github.com: Connection refused; class=Os (2)

@mqudsi
Copy link
Author

mqudsi commented Aug 12, 2022

  • Can (should?) --locked be interpreted as allowing yanked dependency checks to be skipped? (I'm not sure if cargo with --locked specified currently warns about a yanked dependency or if it's a hard error, although I thought it was only the former.)
  • Depending on the reason for checking crates.io, there may be no reason to error out at all (e.g. checking for yanked dependencies isn't required to build a crate, whereas connecting to crates.io to download a missing dependency obviously would be). EDIT: actually, I think updating the repo index should never be reason to error out - subsequent attempts to retrieve an actual dependency from the repo would be, though?
  • Can the timeout for connecting to crates.io/GitHub be reduced in all such non-essential cases? That would help in general, as the OS default can be rather high (Windows default is 72s, iirc). (When testing/mocking, make sure not to just refuse/reject the outgoing connection but rather silently drop the outgoing packets as the behavior differs.)
  • In general, a sane recency check for cargo install (even without --locked) prior to updating the crates.io index might not be a bad idea. Essential crates.io usages will continue to establish a connection to crates.io in order to download missing dependencies, but the worst case scenario here is that a just-updated crate will use a slightly older version (5 minutes stale? an hour?).

@epage
Copy link
Contributor

epage commented Aug 12, 2022

Can (should?) --locked be interpreted as allowing yanked dependency checks to be skipped? (I'm not sure if cargo with --locked specified currently warns about a yanked dependency or if it's a hard error, although I thought it was only the former.)

I believe its there to report warnings.

However, re-evaluating how high of fidelity the warnings have to be is reasonable.

Howver, at least in my testing, the scope of performance improvement, when the network is working, is small. I did some quick tests on my machine with --locked --offline for check and install. Maybe due to the synthetic nature of the test, I missed some cases though.

@mqudsi
Copy link
Author

mqudsi commented Aug 13, 2022

Yeah, in the happy case it's not too bad of a cost at all. The problem is the variability of the highly volatile nature of establishing network connections to third party servers as compared to all the other things that cargo does locally - there are just so many factors that could turn that 250-500ms overhead into a 2-5s overhead or a 20-50s overhead depending on your local environment (connection latency, packet loss), highly variable dns resolution timings (exacerbated by the variances resulting from the choice of dns server), the state of the cdn node you reach, and the load at the destination. GitHub has great uptime, but nothing's ever perfect.

(I'm not clear on how the internals of the update check are performed, can a git gc run ever delay this as well?)

@ehuss ehuss added Command-install A-yanked Area: yanked dependencies labels Aug 13, 2022
@epage
Copy link
Contributor

epage commented Aug 15, 2022

I feel I should add that, in my evaluation, I have a high quality fiber connection and so the results could be dramatically worse for regions that will have significantly worse internet connections.

@weihanglo weihanglo added the S-needs-team-input Status: Needs input from team on whether/how to proceed. label May 24, 2023
@epage
Copy link
Contributor

epage commented Nov 2, 2023

Note: we have an unstable no-index-update feature (#7404)

@mqudsi
Copy link
Author

mqudsi commented Nov 2, 2023

Thanks, Ed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yanked Area: yanked dependencies Command-install S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

5 participants