Add an "-Z offline" flag to Cargo, altering it's dependency resolution behavior #4770

chabapok · 2017-12-03T18:54:17Z

This PR is implementation of the #4686 (without "Populating the global cache" feature)

rust-highfive · 2017-12-03T18:54:30Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

alexcrichton · 2017-12-04T20:08:33Z

Awesome, thanks @chabapok!

Mind adding some tests here as well for crates.io/git/path dependencies? The tests would go into tests/*.rs and you should be able to copy/paste from various locations.

chabapok · 2017-12-05T19:21:56Z

I'll try to do this.
But I do not understand how to hook all network activity (needs for test "no network access in offline mode". As far as I understand, this can not be done. It's too complicated.)

alexcrichton · 2017-12-05T22:11:40Z

Oh I'm mostly just thinking that you'd test things like error messages if the network is reached out to (or needs to be reached out to) and also tests for things like one project populating the cache so another can use it in offline mode (even if a more updated version is available)

chabapok · 2017-12-05T23:53:01Z

git repository may be remote or local.
Cargo in offline mode may allow to fetch branches from local repository to cache, but reject fetch from remote.
I have not done this yet. Now, cargo analyze only cached dependencies.

alexcrichton · 2017-12-06T19:31:03Z

Oh right yeah we've got support in tests to set up "path" git repositories and Cargo just currently treats any remote (be it actually on the filesystem or across the network) as a network operation, so we can use path git repos in tests for this sort of functionality.

chabapok · 2017-12-06T21:57:31Z

How to about to not treat -Z offline mode as --frozen by default?

Let's do it like this. If git url starts with file then this is the local filesystem git repo. So, fetching will not treats as network operation.

cargo build --frozen and cargo build --frozen -Z offline - forbid to fetch from ALL git repos

cargo build -Z offline - Offline, but not frozen mode. Allow to fetch from local git repos, but forbid to fetch from remote

chabapok · 2017-12-07T07:02:51Z

Working with local git repositories is done as suggested. Added some tests. Pls, check.

alexcrichton

Thanks and sorry for the delay in reviewing! Could you be sure to add a few more tests cases like these as well?

The index says two versions exist. The max version isn't downloaded, however. A build should still succeed.
Existing git checkouts should be used without being updated if the offline flag is passed
Exercising the error in resolve where crates can't be loaded. For example the index says it has crates foo and bar. foo depends on bar and while versions of foo have been downloaded no versions of bar have been downloaded.

alexcrichton · 2017-12-18T16:18:04Z

src/cargo/sources/git/utils.rs

+
+#[inline]
+pub fn is_local_filesystem(url: &Url) -> bool {
+    url.scheme() == "file"


Hm could we avoid this special casing? I don't think this is actually used that much in practice and treating all URLs uniformly would make writing tests much easier I think.

Thanks for the review. I will improve the tests.

But i am not sure about treat all URLs uniformly.

As far, as i understand:

Now we did not distinguish between a local or network git repository.
But in offline mode, we would like to do this. We can allow to work with local repositories, but forbid to work with network repositories.

This can be useful for usual offline cases. For example, we have many projects A, B ... Z,that depends on the 'master' branch of our library LIB located in local filesystem.
We want to add some functionality to LIB without breaking many projects, and then use that functionality in some of our projects. We do not have network.

We do this in separate branch (for example, 'my_cool_feature'), as usually.
But after merge 'my_cool_feature' to 'master' branch of the LIB, projects A ... Z can not see top of the master.
There is no way to cache top of the master to the cargo cache without networking.
This will look strange: there is a local git repository - but we can not use it without network.

This code (and some other places in pr) is needed to handle this case.
If we remove this, the code and tests will be easier, but we will not be able to do this.
If we remove the pull-up from the local git repository into the cache, this will worsen usability.

It turns out that if this functionality is missing, we can develop binaries - but we can not use new changes in libraries in the offline mode.

Perhaps this case requires additional discussion?

Oh yeah for sure we could make this distinction but I've never in practice seen anyone using a local git repository. Instead these are just used in tests, where it's quite useful to use the local filesystem as if it were the network to be able to write tests against.

Perhaps let's not make a special distinction here and leave that to a future PR?

alexcrichton · 2017-12-18T16:18:44Z

src/cargo/sources/git/source.rs

@@ -160,6 +160,10 @@ impl<'cfg> Source for GitSource<'cfg> {
                            self.source_id.precise().is_none();

        let (repo, actual_rev) = if should_update {
+            if self.config.cli_unstable().offline && !is_local_filesystem(self.remote.url()) {
+                bail!("Unable to fetch `{:?}`: you are in the offline mode", self.reference);


I think this may not be quite the behavior we want to have, instead if we're in offline mode we'll want to entirely skip the update of a git repository and move straight to checking it out below (if it already exists). I think we'll only want to return an error like this in the case that the database doesn't already exist.

alexcrichton · 2017-12-18T16:22:01Z

src/cargo/util/config.rs

@@ -506,7 +506,11 @@ impl Config {
    }

    pub fn network_allowed(&self) -> bool {
-        !self.frozen
+        !self.frozen() && !self.cli_unstable().offline


I'm a little worried about modifying this function which is already uses in a few other locations. For example I think this error message may no longer be right (or this one or this one). Perhaps these locations could use a central method to determine why the network isn't allowed?

Also I think that these error messages should never happen during offline mode, right? In that offline mode should divert Cargo from executing code paths like that.

alexcrichton · 2017-12-18T16:23:18Z

tests/build.rs

+    assert_that(p2.cargo("build").masquerade_as_nightly_cargo().arg("-Zoffline"),
+                execs().with_status(0)
+                    .with_stderr_does_not_contain("Updating registry")
+                    .with_stderr_does_not_contain("Downloading"));


Could this use with_stderr to match the output exactly?

alexcrichton · 2017-12-18T16:23:48Z

tests/build.rs

+    assert_that(p.cargo("build").masquerade_as_nightly_cargo().arg("-Zoffline"),
+                execs().with_status(101)
+                    .with_stderr_does_not_contain("Updating registry")
+                    .with_stderr_does_not_contain("Downloading"));


Could this also use with_stderr to match the output and error message exactly?

alexcrichton · 2017-12-18T16:24:04Z

tests/build.rs

+
+#[test]
+fn cargo_compile_offline_not_try_update() {
+    use cargotest::ChannelChanger;


It's ok to move imports like this to the top of the file.

alexcrichton · 2017-12-18T16:24:57Z

tests/git.rs

+                execs().with_status(101).
+                    with_stderr_does_not_contain("[UPDATING] git repository [..]").
+                    with_stderr_contains("\
+[..]Unable to fetch `Branch(\"master\")`: you are in the offline mode[..]")


Could this also use with_stderr to match the entire output?

Also could the output here perhaps get improved? The debug representation may not be the best thing to print out and present to users here.

alexcrichton · 2017-12-18T16:25:24Z

tests/registry.rs

+        .build();
+    assert_that(p.cargo("update").masquerade_as_nightly_cargo().arg("-Zoffline"),
+    execs().with_status(101).
+        with_stderr_contains("error: you can't update in the offline mode[..]"));


Could this use with_stderr to match the entire error output?

luser · 2017-12-19T20:12:07Z

Depending on how new the kernel is on the Travis instances cargo gets built on, you might be able to use unshare(2) to run cargo in a different network namespace so that it will functionally not have internet access. I poked around until I figured out that unshare -n -r <command> is enough to do just that. Here's an example of trying to cargo build a simple crate without network access, then doing so again after letting cargo have network access to generate Cargo.lock:

luser@eye7:/build/snippet$ unshare -n -r cargo build
    Updating registry `https://github.com/rust-lang/crates.io-index`
warning: spurious network error (2 tries remaining): [12/-1] curl error: Couldn't resolve host 'github.com'

warning: spurious network error (1 tries remaining): [12/-1] curl error: Couldn't resolve host 'github.com'

error: failed to load source for a dependency on `itoa`

Caused by:
  Unable to update registry https://github.com/rust-lang/crates.io-index

Caused by:
  failed to fetch `https://github.com/rust-lang/crates.io-index`

Caused by:
  [12/-1] curl error: Couldn't resolve host 'github.com'

luser@eye7:/build/snippet$ cargo build
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling itoa v0.3.4
   Compiling snippet v0.1.0 (file:///build/snippet)
    Finished dev [unoptimized + debuginfo] target(s) in 0.43 secs
luser@eye7:/build/snippet$ cargo clean
luser@eye7:/build/snippet$ unshare -n -r cargo build
   Compiling itoa v0.3.4
   Compiling snippet v0.1.0 (file:///build/snippet)
    Finished dev [unoptimized + debuginfo] target(s) in 0.42 secs

alexcrichton · 2017-12-20T20:07:09Z

@luser nice!

I'd actually be totally down with implementing that today in Cargo when --frozen is specified, that'd be great at catching bugs on Cargo's end.

For kernel support we'd probably just use dlsym to dynamically detect at runtime if the libc is new enough to contain the symbol.

chabapok · 2017-12-25T01:01:34Z

I had to rebase PR changes onto 26f768f because automerge conflict, and current master branch not passes all tests.

Changes (as proposed above):

all git repositories treated as remote repositories.
more pretty error printing,
added tests, fix program for that test cases
use with_stderr() in my tests

Pls, check.

alexcrichton

This is looking fantastic @chabapok, thanks so much for the comprehensive tests! Just a few small nits but otherwise I think this is ready to merge!

alexcrichton · 2018-01-03T16:02:02Z

tests/build.rs

+
+    assert_that(p2.cargo("build").masquerade_as_nightly_cargo().arg("-Zoffline"),
+                execs().with_status(0)
+                    .with_stderr_does_not_contain("Updating registry")


You're doing an exhaustive match of stderr below using with_stderr, so it's ok when using that to omit any calls to does_not_contain

alexcrichton · 2018-01-03T16:02:20Z

tests/build.rs

+                    .with_stderr_does_not_contain("Downloading")
+                    .with_stderr(format!("\
+[COMPILING] present_dep v1.2.3
+[COMPILING] bar v0.1.0 ({url})


It's ok to avoid using {url} and just use [..] here (aka don't match that part of the output)

alexcrichton · 2018-01-03T16:03:30Z

tests/build.rs

+[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
+                                         url = p2.url())));
+
+    assert_that(process(&p2.bin("foo")),


You can probably combine this assertion with the above one by using cargo run instead of cargo build, but either's fine!

alexcrichton · 2018-01-03T16:08:02Z

src/cargo/core/resolver/mod.rs

+    if let Some(config) = config {
+        if config.cli_unstable().offline {
+            msg.push_str("\nperhaps an error occurred because you are using \
+                              the offline mode");


I wonder if we can perhaps elaborate on this a bit? Maybe something like:

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 with to retry without the offline flag.

alexcrichton · 2018-01-03T16:08:42Z

src/cargo/sources/git/source.rs

@@ -151,6 +151,10 @@ impl<'cfg> Source for GitSource<'cfg> {

        let db_path = lock.parent().join("db").join(&self.ident);

+        if self.config.cli_unstable().offline && !db_path.exists() {
+            bail!("can't checkout from '{}': you are in the offline mode", self.remote.url());


Could the -Z offline flag be mentioned as part of this error message as well?

bors · 2018-01-08T19:12:24Z

☔ The latest upstream changes (presumably #4919) made this pull request unmergeable. Please resolve the merge conflicts.

chabapok · 2018-01-09T00:53:56Z

Updates based on review. (and rebase to the current master because automerge confict)

Pls, check.

alexcrichton · 2018-01-09T15:20:08Z

@bors: r+

Looks great, thanks @chabapok!

bors · 2018-01-09T15:20:09Z

📌 Commit 3ed3497 has been approved by alexcrichton

bors · 2018-01-09T15:39:52Z

⌛ Testing commit 3ed3497 with merge 3bb0536...

Add an "-Z offline" flag to Cargo, altering it's dependency resolution behavior This PR is implementation of the #4686 (without "Populating the global cache" feature)

bors · 2018-01-09T17:01:28Z

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

rust-highfive assigned alexcrichton Dec 3, 2017

alexcrichton reviewed Dec 18, 2017

View reviewed changes

est31 mentioned this pull request Dec 20, 2017

Add builders to verify that packaging rust in a typical configuration works rust-lang/rust#42786

Closed

matklad mentioned this pull request Dec 24, 2017

Initial addition of airplane mode #4686 #4864

Closed

chabapok force-pushed the master branch from 75bce8a to b6ed99d Compare December 24, 2017 23:56

alexcrichton reviewed Jan 3, 2018

View reviewed changes

add offline mode (-Z offline) with tests

ec5f78f

chabapok force-pushed the master branch from 0022322 to d1b2c72 Compare January 8, 2018 20:01

Updating based on code review

3ed3497

chabapok force-pushed the master branch from d1b2c72 to 3ed3497 Compare January 8, 2018 21:09

bors merged commit 3ed3497 into rust-lang:master Jan 9, 2018

bors mentioned this pull request Jan 9, 2018

Bump hamcrest to 0.1.5 #4886

Closed

Alex-Addy mentioned this pull request Jan 14, 2018

Add an "airplane" flag to Cargo, altering it's dependency resolution behavior #4686

Closed

Eh2406 mentioned this pull request Jan 15, 2018

generate-lockfiles without updating registry #3479

Closed

Eh2406 mentioned this pull request Mar 19, 2018

Add option to "cargo metadata" that avoids network and filesystem changes, and ignores errors due to missing crates #5174

Closed

matklad mentioned this pull request Jun 26, 2018

Offline mode #5655

Closed

ehuss added this to the 1.25.0 milestone Feb 6, 2022

Blisto91 mentioned this pull request Apr 26, 2022

Add support for offline compilation fornwall/rust-script#38

Open

Add an "-Z offline" flag to Cargo, altering it's dependency resolution behavior #4770

Add an "-Z offline" flag to Cargo, altering it's dependency resolution behavior #4770

Conversation

chabapok commented Dec 3, 2017

rust-highfive commented Dec 3, 2017

alexcrichton commented Dec 4, 2017

chabapok commented Dec 5, 2017

alexcrichton commented Dec 5, 2017

chabapok commented Dec 5, 2017

alexcrichton commented Dec 6, 2017

chabapok commented Dec 6, 2017

chabapok commented Dec 7, 2017 • edited Loading

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luser commented Dec 19, 2017

alexcrichton commented Dec 20, 2017

chabapok commented Dec 25, 2017

alexcrichton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Jan 8, 2018

chabapok commented Jan 9, 2018 • edited Loading

alexcrichton commented Jan 9, 2018

bors commented Jan 9, 2018

bors commented Jan 9, 2018

bors commented Jan 9, 2018

chabapok commented Dec 7, 2017 •

edited

Loading

chabapok commented Jan 9, 2018 •

edited

Loading