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 a flag to force network access to be an error #2811

Merged
merged 1 commit into from
Jul 19, 2016

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jun 28, 2016

If a lock file is generated and some equivalent of cargo fetch is run then
Cargo shouldn't ever touch the network or modify Cargo.lock until any
Cargo.toml later changes, but this often wants to be asserted in some build
environments where it's a programmer error if Cargo attempts to access the
network.

The --locked flag added here will assert that Cargo.lock does not need to
change to proceed. That is, if Cargo.lock would be modified (as it
automatically is by default) this is turned into a hard error instead.

This --frozen will not only assert that Cargo.lock doesn't change (the same
behavior as --locked), but it will also will manually prevent Cargo from
touching the network by ensuring that all network requests return an error.

These flags can be used in environments where it is expected that no network
access happens (or no lockfile changes happen) because it has been pre-arranged
for Cargo to not happen. Examples of this include:

  • CI for projects want to pass --locked to ensure that Cargo.lock is up to
    date before changes are checked in.
  • Environments with vendored dependencies want to pass --frozen as touching
    the network indicates a programmer error that something wasn't vendored
    correctly.

A crucial property of these two flags is that they do not change the behavior
of Cargo
. They are simply assertions at a few locations in Cargo to ensure
that actions expected to not happen indeed don't happen. Some documentation has
also been added to this effect.

Closes #2111

@rust-highfive
Copy link

@alexcrichton: no appropriate reviewer found, use r? to override

@alexcrichton
Copy link
Member Author

r? @wycats
cc @rust-lang/tools

@alexcrichton
Copy link
Member Author

Ok, after some more dicussions with @wycats today this PR has now been modified with the addition of two new flags to Cargo:

  • --frozen - assert to Cargo that Cargo.lock should be up to date as well as Cargo's crate cache. That is, if Cargo.lock changes or if a network request would be made then an error is returned.
  • --locked - assert that Cargo.lock is up to date. This flag will still allow network downloads, but it is forbidden for Cargo.lock to change -- for example projects should run this on CI to ensure that changes to Cargo.lock are always checked in.

The idea here is to follow in Bundler's footsteps in these flags. You don't actually want to only assert that no network access happens, but rather crates want an assertion that nothing changes and Cargo just builds crates. That's why the --frozen flag also asserts that Cargo.lock is up to date. The --locked flag was then added to mirror --frozen except without the network restriction (to run on CI).

Eventually, we're also thinking that a cargo freeze subcommand can be added to pair with cargo build --frozen. This would likely be similar to cargo fetch today, but it would basically ensure that any future cargo build --frozen would succeed. Note that this subcommand is not added at this time and I will open an issue tracking it if this is merged.

Also note that I've added an entry to the FAQ about "How can Cargo work offline?". This is a tricky topic, so I'd appreciate review of that as well! (cc @steveklabnik)

hence the request for Cargo to work offline comes up frequently.

Cargo, at its heart, will not attempt to access the network unless told to do
so. That is, if no crates comes from crates.io, a git repository, or some other
Copy link
Member

Choose a reason for hiding this comment

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

might be nice to link this; just needs [] around it.

@nrc
Copy link
Member

nrc commented Jul 4, 2016

The overall strategy here sounds good. I'm a bit unsure about the flag names, they are not intuitive to me are they inherited from somewhere? If not, then I'd suggest --no-update for --locked and no-update-or-network (or preferably something less verbose) for --frozen.

@alexcrichton
Copy link
Member Author

@nrc yeah currently these names come from Bundler, but they're not perhaps the most appropriate. It may be beneficial to tie up semantic ideas in the flags though rather than specifically what they do in case we want to extend their functionality in the future.

@alexcrichton
Copy link
Member Author

r? @brson

@bors
Copy link
Contributor

bors commented Jul 13, 2016

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

@bors
Copy link
Contributor

bors commented Jul 18, 2016

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

If a lock file is generated and some equivalent of `cargo fetch` is run then
Cargo shouldn't ever touch the network or modify `Cargo.lock` until any
`Cargo.toml` later changes, but this often wants to be asserted in some build
environments where it's a programmer error if Cargo attempts to access the
network.

The `--locked` flag added here will assert that `Cargo.lock` does not need to
change to proceed. That is, if `Cargo.lock` would be modified (as it
automatically is by default) this is turned into a hard error instead.

This `--frozen` will not only assert that `Cargo.lock` doesn't change (the same
behavior as `--locked`), but it will also will manually prevent Cargo from
touching the network by ensuring that all network requests return an error.

These flags can be used in environments where it is *expected* that no network
access happens (or no lockfile changes happen) because it has been pre-arranged
for Cargo to not happen. Examples of this include:

* CI for projects want to pass `--locked` to ensure that `Cargo.lock` is up to
  date before changes are checked in.
* Environments with vendored dependencies want to pass `--frozen` as touching
  the network indicates a programmer error that something wasn't vendored
  correctly.

A crucial property of these two flags is that **they do not change the behavior
of Cargo**. They are simply assertions at a few locations in Cargo to ensure
that actions expected to not happen indeed don't happen. Some documentation has
also been added to this effect.

Closes rust-lang#2111
@wycats
Copy link
Contributor

wycats commented Jul 19, 2016

@nrc @alexcrichton the names aren't exactly taken from bundler, but they're loosely inspired by bundler.

The idea is:

  • --locked means "I have an up-to-date Cargo.lock" aka "I am locked"
  • --frozen is meant to be paired with a future cargo freeze, which will put the directory into a "frozen" state, and can be used with the --frozen assertion.

In other words, "locked" and "frozen" are states a Cargo directory can be in, and --locked and --frozen are flags that assert that the directory is in that state (in contrast with flags that sound like they instruct Cargo to do something different than the usual, which is emphatically not what these flags do).

@alexcrichton
Copy link
Member Author

@nrc although they are similar they're also doing relatively similar things under the hood, and I was unable to think of some better names. Do you feel ok moving forward with these flag names?

@nrc
Copy link
Member

nrc commented Jul 19, 2016

Yeah, I'm ok with moving forward here. I like @wycats' point that locked can be related to the Cargo.lock file. I think it is probably worth making that intuition clear in the docs.

@alexcrichton
Copy link
Member Author

Ok cool, in that case,

ping r? @brson

@joshtriplett
Copy link
Member

One request before this goes in: since --frozen means --no-update-or-network, would you consider providing an option that just means --no-network, but allows updating Cargo.lock (as long as doing so doesn't require touching the network)? That would help for the use case of obtaining all dependencies from a local registry or directory registry.

@alexcrichton
Copy link
Member Author

@joshtriplett isn't that just cargo freeze and/or cargo fetch followed by cargo build --frozen?

@alexcrichton
Copy link
Member Author

Put another way, Cargo.lock existing is proof that it's even possible to not use the network, without that there's basically no guarantees and something needs to be fetched most likely.

@joshtriplett
Copy link
Member

joshtriplett commented Jul 19, 2016

@alexcrichton No, I'd like to run cargo build but have it fail if it would hit the network rather than the directory registry. Fetching from the directory registry is fine; fetching from the network is not. That's what I'd like to enforce.

Of course, given the existence of crates that actually fetch from the network in their build.rs (and don't even do any verification of the result), running the whole build in a network namespace with no network access is probably a good idea too. But if Cargo could trivially support an option to tell it to not fetch from the network using the same machinery you've already introduced, that'd be really handy.

@alexcrichton
Copy link
Member Author

@joshtriplett given that the motivation is for packagers, I think that we'll want to have a more dedicated discussion or RFC towards that use case to ensure we've fleshed out everything. We can of course always add more flags to Cargo!

@wycats
Copy link
Contributor

wycats commented Jul 19, 2016

@alexcrichton @joshtriplett Can you open up an issue on the RFC repo to help flesh out the "motivation" section for a packager RFC? I'd like to fully enumerate the requirements in detail and then move on to a detailed proposal.

When doing this, please focus on the underlying motivations rather than the top level requirements (e.g. try to explain why hitting the network is verboten; if the answer is a requirement of a build bot, fleshing out the details of how the build bots work and why they require what they require would be very useful).

@cuviper
Copy link
Member

cuviper commented Jul 19, 2016

Generally speaking, everything needed to build distro packages should be contained in the distro. If you have to reach a network resource to complete a build, it's clearly not self-contained.

@brson
Copy link
Contributor

brson commented Jul 19, 2016

I also think @joshtriplett's case is important. I've definitely hit cases where I changed the dependencies, but knew the entire graph could be already be solved with cached local deps, but was stuck on an airplane and could not proceed.

@wycats
Copy link
Contributor

wycats commented Jul 19, 2016

I've definitely hit cases where I changed the dependencies, but knew the entire graph could be already be solved with cached local deps, but was stuck on an airplane and could not proceed.

The various flags proposed in this PR assert that you won't need to hit the network. In general, if you don't need to hit the network but end up blocked on proceeding due to the lack of network that is a bug in Cargo. it is important to me and @alexcrichton that this is true.

The most common source of this kind of bug is problems with the override feature, which we're in the process of reforming and replacing.

@brson
Copy link
Contributor

brson commented Jul 19, 2016

@wycats OK, I'll file some bugs then.

@bors r+

@bors
Copy link
Contributor

bors commented Jul 19, 2016

📌 Commit a504f48 has been approved by brson

@bors
Copy link
Contributor

bors commented Jul 19, 2016

⌛ Testing commit a504f48 with merge 664125b...

bors added a commit that referenced this pull request Jul 19, 2016
Add a flag to force network access to be an error

If a lock file is generated and some equivalent of `cargo fetch` is run then
Cargo shouldn't ever touch the network or modify `Cargo.lock` until any
`Cargo.toml` later changes, but this often wants to be asserted in some build
environments where it's a programmer error if Cargo attempts to access the
network.

The `--locked` flag added here will assert that `Cargo.lock` does not need to
change to proceed. That is, if `Cargo.lock` would be modified (as it
automatically is by default) this is turned into a hard error instead.

This `--frozen` will not only assert that `Cargo.lock` doesn't change (the same
behavior as `--locked`), but it will also will manually prevent Cargo from
touching the network by ensuring that all network requests return an error.

These flags can be used in environments where it is *expected* that no network
access happens (or no lockfile changes happen) because it has been pre-arranged
for Cargo to not happen. Examples of this include:

* CI for projects want to pass `--locked` to ensure that `Cargo.lock` is up to
  date before changes are checked in.
* Environments with vendored dependencies want to pass `--frozen` as touching
  the network indicates a programmer error that something wasn't vendored
  correctly.

A crucial property of these two flags is that **they do not change the behavior
of Cargo**. They are simply assertions at a few locations in Cargo to ensure
that actions expected to not happen indeed don't happen. Some documentation has
also been added to this effect.

Closes #2111
@bors
Copy link
Contributor

bors commented Jul 19, 2016

☀️ Test successful - cargo-cross-linux, cargo-linux-32, cargo-linux-64, cargo-mac-32, cargo-mac-64, cargo-win-gnu-32, cargo-win-gnu-64, cargo-win-msvc-32, cargo-win-msvc-64
Approved by: brson
Pushing 664125b to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants