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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/cargo/core/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,12 @@ impl<'a, 'cfg> Downloads<'a, 'cfg> {
/// eventually be returned from `wait_for_download`. Returns `Some(pkg)` if
/// the package is ready and doesn't need to be downloaded.
pub fn start(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
Ok(self
.start_inner(id)
.chain_err(|| format!("failed to download `{}`", id))?)
}

fn start_inner(&mut self, id: PackageId) -> CargoResult<Option<&'a Package>> {
// First up see if we've already cached this package, in which case
// there's nothing to do.
let slot = self
Expand Down
47 changes: 35 additions & 12 deletions src/cargo/sources/registry/index.rs
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ impl<'cfg> RegistryIndex<'cfg> {
ret.reserve(contents.lines().count());
let lines = contents.lines().map(|s| s.trim()).filter(|l| !l.is_empty());

let online = !self.config.cli_unstable().offline;
// Attempt forwards-compatibility on the index by ignoring
// everything that we ourselves don't understand, that should
// allow future cargo implementations to break the
Expand All @@ -215,11 +214,7 @@ impl<'cfg> RegistryIndex<'cfg> {
return None;
}
};
if online || load.is_crate_downloaded(summary.package_id()) {
Some((summary, locked))
} else {
None
}
Some((summary, locked))
}));

Ok(())
Expand Down Expand Up @@ -274,17 +269,43 @@ impl<'cfg> RegistryIndex<'cfg> {
yanked_whitelist: &HashSet<PackageId>,
f: &mut dyn FnMut(Summary),
) -> CargoResult<()> {
if self.config.cli_unstable().offline {
if self.query_inner_with_online(dep, load, yanked_whitelist, f, false)? != 0 {
return Ok(());
}
// If offline, and there are no matches, try again with online.
// This is necessary for dependencies that are not used (such as
// target-cfg or optional), but are not downloaded. Normally the
// build should succeed if they are not downloaded and not used,
// but they still need to resolve. If they are actually needed
// then cargo will fail to download and an error message
// indicating that the required dependency is unavailable while
// offline will be displayed.
}
self.query_inner_with_online(dep, load, yanked_whitelist, f, true)?;
Ok(())
}

fn query_inner_with_online(
&mut self,
dep: &Dependency,
load: &mut dyn RegistryData,
yanked_whitelist: &HashSet<PackageId>,
f: &mut dyn FnMut(Summary),
online: bool,
) -> CargoResult<usize> {
let source_id = self.source_id;
let name = dep.package_name().as_str();
let summaries = self.summaries(name, load)?;
let summaries = summaries
.iter()
.filter(|&(summary, yanked)| {
!yanked || {
log::debug!("{:?}", yanked_whitelist);
log::debug!("{:?}", summary.package_id());
yanked_whitelist.contains(&summary.package_id())
}
(online || load.is_crate_downloaded(summary.package_id()))
&& (!yanked || {
log::debug!("{:?}", yanked_whitelist);
log::debug!("{:?}", summary.package_id());
yanked_whitelist.contains(&summary.package_id())
})
})
.map(|s| s.0.clone());

Expand All @@ -308,9 +329,11 @@ impl<'cfg> RegistryIndex<'cfg> {
_ => true,
});

let mut count = 0;
for summary in summaries {
f(summary);
count += 1;
}
Ok(())
Ok(count)
}
}
84 changes: 76 additions & 8 deletions tests/testsuite/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1140,6 +1140,77 @@ fn main(){
.run();
}

#[test]
fn offline_unused_target_dep() {
// -Z offline with a target dependency that is not used and not downloaded.
Package::new("unused_dep", "1.0.0").publish();
Package::new("used_dep", "1.0.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.1.0"
[dependencies]
used_dep = "1.0"
[target.'cfg(unused)'.dependencies]
unused_dep = "1.0"
"#,
)
.file("src/lib.rs", "")
.build();
// 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 [..]")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.run();
p.cargo("clean").run();
// Build offline, make sure it works.
p.cargo("build -Z offline")
.masquerade_as_nightly_cargo()
.run();
}

#[test]
fn offline_missing_optional() {
Package::new("opt_dep", "1.0.0").publish();
let p = project()
.file(
"Cargo.toml",
r#"
[project]
name = "foo"
version = "0.1.0"
[dependencies]
opt_dep = { version = "1.0", optional = true }
"#,
)
.file("src/lib.rs", "")
.build();
// Do a build that downloads only what is necessary.
p.cargo("build")
.with_stderr_does_not_contain("[DOWNLOADED] opt_dep [..]")
.run();
p.cargo("clean").run();
// Build offline, make sure it works.
p.cargo("build -Z offline")
.masquerade_as_nightly_cargo()
.run();
p.cargo("build -Z offline --features=opt_dep")
.masquerade_as_nightly_cargo()
.with_stderr(
"\
[ERROR] failed to download `opt_dep v1.0.0`

Caused by:
can't make HTTP request in the offline mode
",
)
.with_status(101)
.run();
}

#[test]
fn incompatible_dependencies() {
Package::new("bad", "0.1.0").publish();
Expand Down Expand Up @@ -1282,14 +1353,11 @@ fn compile_offline_while_transitive_dep_not_cached() {
.with_status(101)
.with_stderr(
"\
error: no matching package named `baz` found
location searched: registry `[..]`
required by package `bar v0.1.0`
... which is depended on by `foo v0.0.1 ([CWD])`
As a reminder, you're using offline mode (-Z offline) \
which can sometimes cause surprising resolution failures, \
if this error is too confusing you may wish to retry \
without the offline flag.",
[ERROR] failed to download `baz v1.0.0`

Caused by:
can't make HTTP request in the offline mode
",
)
.run();
}
Expand Down
1 change: 1 addition & 0 deletions tests/testsuite/support/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ pub fn alt_api_url() -> Url {
///
/// p.cargo("run").with_stdout("24").run();
/// ```
#[must_use]
pub struct Package {
name: String,
vers: String,
Expand Down