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

Resolve: Be less strict while offline. #6814

Merged
merged 1 commit into from Apr 2, 2019

Conversation

Projects
None yet
5 participants
@ehuss
Copy link
Contributor

commented Apr 2, 2019

When offline, the resolver was requiring everything to be downloaded, even dependencies that are not used. This changes it so that the resolver can still resolve unavailable dependencies when offline. This pushes the failure to a later stage of Cargo where it attempts to download the dependency. This makes -Z offline work for target-cfg or optional dependencies that are not being used.

Fixes #6014.

This changes the error message significantly for the "unavailable" case (see test diff). I personally think the new error message is clearer, although it is shorter and provides less information. The old error message seemed large and scary, and was a little hard for me to grok. However, I'd be willing to look at tweaking the error behavior if not everyone agrees.

@rust-highfive

This comment has been minimized.

Copy link

commented Apr 2, 2019

r? @nrc

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

@alexcrichton
Copy link
Member

left a comment

Nice! This seems like a fix we definitely want because the referenced issue is definitely some crazy behavior!

The major purpose of the error message updates was to resolve a situation where you know that serde 1.0.39 is published but when you try to use it Cargo keeps saying it doesn't exist. The "by the way you're offline" is a hint to go find something passing --offline, either yourself or some tool in the middle.

I think that the error message here clearly indicates that (aka if you hit an error it says you're offline so you can go hunting). There's still more error messages coming out of the resolver because we may not pick up newer versions, but the error messages here in the resolver weren't changed so it'll only show for the more obscure resolver errors rather than the bland "this package wasn't downloaded" situation.

All that to say that I think this is a great fix, thanks @ehuss!

@bors: r+

// Do a build that downloads only what is necessary.
p.cargo("build")
.with_stderr_contains("[DOWNLOADED] used_dep [..]")
.with_stderr_does_not_contain("[DOWNLOADED] unused_dep [..]")

This comment has been minimized.

Copy link
@alexcrichton

alexcrichton Apr 2, 2019

Member

As a meta note, I personally find "assert does not contain" as a not-very-useful assertion in test suites because there's so many output that don't contain the string. For example if unused_dep here had a typo the test would pass but not because we thought it would.

That's where all the original tests from long ago do exhaustive checking of the output which allows for easily detecting bugs/changes. The downside of those tests though is that if you change the output it's really annoying to change the tests, and we have quite a few tests now!

(JFYI)

This comment has been minimized.

Copy link
@ehuss

ehuss Apr 2, 2019

Author Contributor

I agree. There is a note in the doc that it is dangerous. Whenever I use it, I try to write the test first so it fails, and then it should only pass when fixed. However, that doesn't really protect from the future when things change. I'll leave some notes in #6816.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@bors: r+

Always wanted to test and see if bors learned comments from pull request reviews, and now I know the answer is "no"

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

📌 Commit 2378a9b has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

⌛️ Testing commit 2378a9b with merge b9bba2d...

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

Auto merge of #6814 - ehuss:offline-optional-dep, r=alexcrichton
Resolve: Be less strict while offline.

When offline, the resolver was requiring everything to be downloaded, even dependencies that are not used. This changes it so that the resolver can still resolve unavailable dependencies when offline. This pushes the failure to a later stage of Cargo where it attempts to download the dependency. This makes `-Z offline` work for target-cfg or optional dependencies that are not being used.

Fixes #6014.

This changes the error message significantly for the "unavailable" case (see test diff). I personally think the new error message is clearer, although it is shorter and provides less information. The old error message seemed large and scary, and was a little hard for me to grok. However, I'd be willing to look at tweaking the error behavior if not everyone agrees.
@bors

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

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

@bors bors merged commit 2378a9b into rust-lang:master Apr 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@dtolnay dtolnay referenced this pull request Apr 3, 2019

Merged

Update cargo #59681

Centril added a commit to Centril/rust that referenced this pull request Apr 5, 2019

Rollup merge of rust-lang#59681 - dtolnay:cargo, r=alexcrichton
Update cargo

20 commits in
63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c
2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000
- Fix Init for Fossil SCM project (rust-lang/cargo#6792)
- Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799)
- Don't include email if it is empty (rust-lang/cargo#6802)
- Fix unused import warning (rust-lang/cargo#6807)
- Add some help and documentation for unstable flags (rust-lang/cargo#6791)
- Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803)
- Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804)
- Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805)
- Warn on version req with metadata (rust-lang/cargo#6806)
- cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801)
- Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800)
- Cargo clippy (rust-lang/cargo#6759)
- Don't include metadata in wasm binary examples (rust-lang/cargo#6812)
- Update glossary for `feature` (rust-lang/cargo#6809)
- Include proc-macros in `build-override` (rust-lang/cargo#6811)
- Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776)
- Add some docs for `Downloads` (rust-lang/cargo#6815)
- Resolve: Be less strict while offline (rust-lang/cargo#6814)
- Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)
- Fix doc link (rust-lang/cargo#6820)

<br>

I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock rust-lang#59076.

Mentioning @ehuss.

bors added a commit to rust-lang/rust that referenced this pull request Apr 5, 2019

Auto merge of #59681 - dtolnay:cargo, r=alexcrichton
Update cargo

20 commits in
63231f438a2b5b84ccf319a5de22343ee0316323..6f3e9c367abb497c64f360c3839dab5e74928d5c
2019-03-27 12:26:45 +0000 to 2019-04-04 14:11:33 +0000
- Fix Init for Fossil SCM project (rust-lang/cargo#6792)
- Fix member_manifest_version_error accessing the network (rust-lang/cargo#6799)
- Don't include email if it is empty (rust-lang/cargo#6802)
- Fix unused import warning (rust-lang/cargo#6807)
- Add some help and documentation for unstable flags (rust-lang/cargo#6791)
- Allow `cargo doc --open` with multiple packages (rust-lang/cargo#6803)
- Allow `cargo install --path P` to load config from P (rust-lang/cargo#6804)
- Add more suggestions on how to deal with excluding a package from a workspace (rust-lang/cargo#6805)
- Warn on version req with metadata (rust-lang/cargo#6806)
- cargo install: Be more restrictive about cli flags (rust-lang/cargo#6801)
- Support force-pushed repos with git-fetch-with-cli (rust-lang/cargo#6800)
- Cargo clippy (rust-lang/cargo#6759)
- Don't include metadata in wasm binary examples (rust-lang/cargo#6812)
- Update glossary for `feature` (rust-lang/cargo#6809)
- Include proc-macros in `build-override` (rust-lang/cargo#6811)
- Resolver: A dep is equivalent to one of the things it can resolve to (rust-lang/cargo#6776)
- Add some docs for `Downloads` (rust-lang/cargo#6815)
- Resolve: Be less strict while offline (rust-lang/cargo#6814)
- Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)
- Fix doc link (rust-lang/cargo#6820)

<br>

I specifically care about "Accept trailing comma in test of impl Debug for PackageId (rust-lang/cargo#6818)" to unblock #59076.

Mentioning @ehuss.
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.