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 some help with updating the registry in offline mode. #6871

Merged
merged 4 commits into from Apr 29, 2019

Conversation

Projects
None yet
5 participants
@ehuss
Copy link
Contributor

commented Apr 24, 2019

This includes the following:

  • Move offline tests to a dedicated file.

  • If an index has never been downloaded, and -Z offline is used, provide a suggestion to run without -Z offline.

  • If an index needs to be updated, and the network appears to be down, suggest running with -Z offline. (removed this)

  • The offline_resolve_optional_fail test is added to show that resolver errors are still possible (though hopefully rare!).

@ehuss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 24, 2019

This is an attempt to improve some error message, but I'm on the fence if it actually does that, so I figured I'd post this for comments.

I think repo.is_empty() should be a reliable way to determine if the repository has never been fetched, but I'm not certain.

I'm not sure if the offline && is_empty() check might give a false positive for the error. I can't think of any, though.

The maybe_spurious check may have false positives.

I'm not sure if the wording is clear.

@rust-highfive

This comment has been minimized.

Copy link

commented Apr 24, 2019

r? @nrc

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

@ehuss ehuss force-pushed the ehuss:offline-spurious-suggestion branch from 42e95c2 to f876910 Apr 24, 2019

if let Err(e) = result {
let extra = if maybe_spurious(&e) && nightly_features_allowed() && !repo.is_empty()? {
"\nIf the network is unavailable, consider running with the -Zoffline \
flag to run in offline mode."

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 25, 2019

Member

This warning I'm somewhat wary about because it feels a little aggressive about mentioning -Zoffline. We're already pretty fast-and-loose with what we interpret as spurious network error (we're rarely sure that it's a spurious error, it's just a random bucket of things that are all somewhat network related).

I'm personally wary of CI tools that make suggestions which often don't end up being applicable as well because it can be a frustrating experience. Ideally we'd recognize a spurious error, actually run resolution in offline mode, realize it works, and only then actually emit an appropriate error. That seems a bit heavy handed and a bit far fetched though.

Do you think though that if we don't have this sort of an error message then -Zoffline will be difficult to discover?

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 25, 2019

Author Contributor

Yea, this is only for discoverability. Should I just remove this, or would it be worthwhile to emit a warning and see if it can succeed to resolve? I lean towards just removing it.

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 25, 2019

Author Contributor

This also fires for cargo install which will give a bad suggestion, cargo install foo -Z offline will fail, so I think the risk of a bad suggestion is a little too high.

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 26, 2019

Member

Hm those are good points yeah. I'm fine with landing without this and we can sort of see over time whether we feel that we need to improve the error messages for offline.

Run `cargo fetch` to download the registry index and all \
of a project's dependencies before going offline.",
self.source_id
);

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 25, 2019

Member

Could you add a comment here that we're explicitly switching the Ok to an Err because if we're updating the index we're guaranteed to require at leats one crate and since the index is empty it's guaranteed to have zero crates, therefore we're trying to give a better error message

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 25, 2019

Author Contributor

Hm, this suggestion is also a little sketchy. If you run cargo install foo for the first time, and the index is unavailable, it suggests running cargo fetch which will definitely not work. Maybe it is not worth it?

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 26, 2019

Member

That's a good point yeah, but I think that could be fixed by tweaking the error message? It could mention that you should try running with out --offline or otherwise run cargo fetch sooner perhaps?

@ehuss ehuss referenced this pull request Apr 25, 2019

Closed

Offline mode #5655

ehuss added some commits Apr 23, 2019

Add some help with updating the registry in offline mode.
This makes two changes:

- If an index has never been downloaded, and `-Z offline` is used, provide a
  suggestion to run without `-Z offline`.

- If an index needs to be updated, and the network appears to be down, suggest
  running with `-Z offline`.
Revert attempt to provide a -Zoffline suggestion if updating fails.
This is just too likely to be a false positive, or otherwise a bad suggestion.

@ehuss ehuss force-pushed the ehuss:offline-spurious-suggestion branch from f876910 to c6fad3a Apr 29, 2019

@ehuss

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

OK, I reverted the spurious error, and tweaked the text of the other one.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

@bors: r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

📌 Commit c6fad3a has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

⌛️ Testing commit c6fad3a with merge 2de6c3e...

bors added a commit that referenced this pull request Apr 29, 2019

Auto merge of #6871 - ehuss:offline-spurious-suggestion, r=alexcrichton
 Add some help with updating the registry in offline mode.

This includes the following:

- Move offline tests to a dedicated file.

- If an index has never been downloaded, and `-Z offline` is used, provide a suggestion to run without `-Z offline`.

- ~~If an index needs to be updated, and the network appears to be down, suggest running with `-Z offline`.~~ (removed this)

- The `offline_resolve_optional_fail` test is added to show that resolver errors are still possible (though hopefully rare!).
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: alexcrichton
Pushing 2de6c3e to master...

@bors bors merged commit c6fad3a into rust-lang:master Apr 29, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.