Skip to content

fix: Respect --frozen everywhere --offline or --locked is accepted #15263

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

Merged
merged 9 commits into from
Mar 4, 2025
6 changes: 3 additions & 3 deletions src/cargo/core/resolver/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -393,13 +393,13 @@ pub(super) fn activation_error(
);

if let Some(gctx) = gctx {
if gctx.offline() {
if let Some(offline_flag) = gctx.offline_flag() {
let _ = write!(
&mut hints,
"\nAs a reminder, you're using offline mode (--offline) \
"\nAs a reminder, you're using offline mode ({offline_flag}) \
which can sometimes cause surprising resolution failures, \
if this error is too confusing you may wish to retry \
without the offline flag.",
without `{offline_flag}`.",
);
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/cargo/ops/cargo_add/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -251,11 +251,11 @@ pub fn add(workspace: &Workspace<'_>, options: &AddOptions<'_>) -> CargoResult<(
}
}

if options.gctx.locked() {
if let Some(locked_flag) = options.gctx.locked_flag() {
let new_raw_manifest = manifest.to_string();
if original_raw_manifest != new_raw_manifest {
anyhow::bail!(
"the manifest file {} needs to be updated but --locked was passed to prevent this",
"the manifest file {} needs to be updated but {locked_flag} was passed to prevent this",
manifest.path.display()
);
}
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ impl<'gctx> InstallablePackage<'gctx> {
lockfile_path.clone(),
)?;

if gctx.locked() {
if !gctx.lock_update_allowed() {
// When --lockfile-path is set, check that passed lock file exists
// (unlike the usual flag behavior, lockfile won't be created as we imply --locked)
if let Some(requested_lockfile_path) = ws.requested_lockfile_path() {
Expand Down
11 changes: 3 additions & 8 deletions src/cargo/ops/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,19 +50,14 @@ pub fn write_pkg_lockfile(ws: &Workspace<'_>, resolve: &mut Resolve) -> CargoRes
}
}

if !ws.gctx().lock_update_allowed() {
let flag = if ws.gctx().locked() {
"--locked"
} else {
"--frozen"
};
if let Some(locked_flag) = ws.gctx().locked_flag() {
anyhow::bail!(
"the lock file {} needs to be updated but {} was passed to prevent this\n\
If you want to try to generate the lock file without accessing the network, \
remove the {} flag and use --offline instead.",
lock_root.as_path_unlocked().join(LOCKFILE_NAME).display(),
flag,
flag
locked_flag,
locked_flag
);
}

Expand Down
8 changes: 2 additions & 6 deletions src/cargo/ops/registry/info/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,12 +198,8 @@ fn validate_locked_and_frozen_options(
) -> Result<(), anyhow::Error> {
// Only in workspace, we can use --frozen or --locked.
if !in_workspace {
if gctx.locked() {
bail!("the option `--locked` can only be used within a workspace");
}

if gctx.frozen() {
bail!("the option `--frozen` can only be used within a workspace");
if let Some(locked_flag) = gctx.locked_flag() {
bail!("the option `{locked_flag}` can only be used within a workspace");
}
}
Ok(())
Expand Down
16 changes: 11 additions & 5 deletions src/cargo/sources/git/source.rs
Original file line number Diff line number Diff line change
Expand Up @@ -299,10 +299,16 @@ impl<'gctx> Source for GitSource<'gctx> {
// If we're in offline mode, we're not locked, and we have a
// database, then try to resolve our reference with the preexisting
// repository.
(Revision::Deferred(git_ref), Some(db)) if self.gctx.offline() => {
(Revision::Deferred(git_ref), Some(db)) if !self.gctx.network_allowed() => {
let offline_flag = self
.gctx
.offline_flag()
.expect("always present when `!network_allowed`");
let rev = db.resolve(&git_ref).with_context(|| {
"failed to lookup reference in preexisting repository, and \
can't check for updates in offline mode (--offline)"
format!(
"failed to lookup reference in preexisting repository, and \
can't check for updates in offline mode ({offline_flag})"
)
})?;
(db, rev)
}
Expand All @@ -312,9 +318,9 @@ impl<'gctx> Source for GitSource<'gctx> {
// situation that we have a locked revision but the database
// doesn't have it.
(locked_rev, db) => {
if self.gctx.offline() {
if let Some(offline_flag) = self.gctx.offline_flag() {
anyhow::bail!(
"can't checkout from '{}': you are in the offline mode (--offline)",
"can't checkout from '{}': you are in the offline mode ({offline_flag})",
self.remote.url()
);
}
Expand Down
7 changes: 2 additions & 5 deletions src/cargo/sources/git/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -955,15 +955,12 @@ pub fn fetch(
gctx: &GlobalContext,
remote_kind: RemoteKind,
) -> CargoResult<()> {
if gctx.frozen() {
if let Some(offline_flag) = gctx.offline_flag() {
anyhow::bail!(
"attempting to update a git repository, but --frozen \
"attempting to update a git repository, but {offline_flag} \
was specified"
)
}
if !gctx.network_allowed() {
anyhow::bail!("can't update a git repository in the offline mode")
}

let shallow = remote_kind.to_shallow_setting(repo.is_shallow(), gctx);

Expand Down
4 changes: 2 additions & 2 deletions src/cargo/sources/registry/http_remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ impl<'gctx> HttpRegistry<'gctx> {
} else if self.gctx.cli_unstable().no_index_update {
trace!("using local {} in no_index_update mode", path.display());
true
} else if self.gctx.offline() {
} else if !self.gctx.network_allowed() {
trace!("using local {} in offline mode", path.display());
true
} else if self.fresh.contains(path) {
Expand Down Expand Up @@ -511,7 +511,7 @@ impl<'gctx> RegistryData for HttpRegistry<'gctx> {
return Poll::Ready(Ok(LoadResponse::NotFound));
}

if self.gctx.offline() || self.gctx.cli_unstable().no_index_update {
if !self.gctx.network_allowed() || self.gctx.cli_unstable().no_index_update {
// Return NotFound in offline mode when the file doesn't exist in the cache.
// If this results in resolution failure, the resolver will suggest
// removing the --offline flag.
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/registry/index/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,7 @@ impl<'gctx> RegistryIndex<'gctx> {
load: &mut dyn RegistryData,
f: &mut dyn FnMut(IndexSummary),
) -> Poll<CargoResult<()>> {
if self.gctx.offline() {
if !self.gctx.network_allowed() {
// This should only return `Poll::Ready(Ok(()))` if there is at least 1 match.
//
// If there are 0 matches it should fall through and try again with online.
Expand Down
2 changes: 1 addition & 1 deletion src/cargo/sources/registry/remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ impl<'gctx> RegistryData for RemoteRegistry<'gctx> {
}
self.mark_updated();

if self.gctx.offline() {
if !self.gctx.network_allowed() {
return Ok(());
}
if self.gctx.cli_unstable().no_index_update {
Expand Down
32 changes: 20 additions & 12 deletions src/cargo/util/context/mod.rs
Copy link
Member

Choose a reason for hiding this comment

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

The change looks good, but we still have offline_flag and locked_flag here competing {network,lock_update}_allowed. They are not documented and might be misused again in the future.

What if we make *_allowed return an enum like these to avoid misuse, and can stop exposing *_flag method?

enum LockfileUpdate {
    Allowed,
    Disallowed(&'static str),
}

enum NetworkAccess {
    Allowed,
    Disallowed(&'static str),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The *_flag functions are just like the bool functions, just with more data (and inverted). Not really seeing room for misuse like we had before.

As for an enum, while it makes it more explicit, it is also more cumbersome to use as you now need to access to the enum in each place you make the function call to compare the result. This also would encourage more of a match to make sure every case is handled when this is just a binary choice.

Copy link
Member

Choose a reason for hiding this comment

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

That is fair. Thanks for the explanation!

Original file line number Diff line number Diff line change
Expand Up @@ -1161,27 +1161,35 @@ impl GlobalContext {
}

pub fn network_allowed(&self) -> bool {
!self.frozen() && !self.offline()
!self.offline_flag().is_some()
}

pub fn offline(&self) -> bool {
self.offline
}

pub fn frozen(&self) -> bool {
self.frozen
}

pub fn locked(&self) -> bool {
self.locked
pub fn offline_flag(&self) -> Option<&'static str> {
if self.frozen {
Some("--frozen")
} else if self.offline {
Some("--offline")
} else {
None
}
}

pub fn set_locked(&mut self, locked: bool) {
self.locked = locked;
}

pub fn lock_update_allowed(&self) -> bool {
!self.frozen && !self.locked
!self.locked_flag().is_some()
}

pub fn locked_flag(&self) -> Option<&'static str> {
if self.frozen {
Some("--frozen")
} else if self.locked {
Some("--locked")
} else {
None
}
}

/// Loads configuration from the filesystem.
Expand Down
10 changes: 2 additions & 8 deletions src/cargo/util/network/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,9 @@ pub fn http_handle(gctx: &GlobalContext) -> CargoResult<Easy> {
}

pub fn http_handle_and_timeout(gctx: &GlobalContext) -> CargoResult<(Easy, HttpTimeout)> {
if gctx.frozen() {
if let Some(offline_flag) = gctx.offline_flag() {
bail!(
"attempting to make an HTTP request, but --frozen was \
specified"
)
}
if gctx.offline() {
bail!(
"attempting to make an HTTP request, but --offline was \
"attempting to make an HTTP request, but {offline_flag} was \
specified"
)
}
Expand Down
14 changes: 7 additions & 7 deletions tests/testsuite/offline.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ fn cargo_compile_offline_not_try_update() {
[ERROR] no matching package named `not_cached_dep` found
location searched: crates.io index
required by package `bar v0.1.0 ([ROOT]/bar)`
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--offline`.

"#]])
.run();
Expand All @@ -195,7 +195,7 @@ As a reminder, you're using offline mode (--offline) which can sometimes cause s
[ERROR] no matching package named `not_cached_dep` found
location searched: crates.io index
required by package `bar v0.1.0 ([ROOT]/bar)`
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--offline`.

"#]]).run();
}
Expand Down Expand Up @@ -384,13 +384,13 @@ fn update_offline_not_cached() {

p.cargo("update --offline")
.with_status(101)
.with_stderr_data(str![["
.with_stderr_data(str![[r#"
[ERROR] no matching package named `bar` found
location searched: [..]
required by package `foo v0.0.1 ([ROOT]/foo)`
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--offline`.

"]])
"#]])
.run();
}

Expand Down Expand Up @@ -607,7 +607,7 @@ candidate versions found which didn't match: 1.0.0
location searched: `dummy-registry` index (which is replacing registry `crates-io`)
required by package `foo v0.1.0 ([ROOT]/foo)`
perhaps a crate was updated and forgotten to be re-vendored?
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--offline`.

"#]]
)
Expand Down Expand Up @@ -754,7 +754,7 @@ fn main(){
[ERROR] no matching package named `present_dep` found
location searched: `dummy-registry` index (which is replacing registry `crates-io`)
required by package `foo v0.1.0 ([ROOT]/foo)`
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without the offline flag.
As a reminder, you're using offline mode (--offline) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--offline`.

"#]]
)
Expand Down
29 changes: 8 additions & 21 deletions tests/testsuite/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2431,14 +2431,10 @@ fn disallow_network_http() {
p.cargo("check --frozen")
.with_status(101)
.with_stderr_data(str![[r#"
[UPDATING] `dummy-registry` index
[ERROR] failed to get `foo` as a dependency of package `bar v0.5.0 ([ROOT]/foo)`

Caused by:
failed to query replaced source registry `crates-io`

Caused by:
attempting to make an HTTP request, but --frozen was specified
[ERROR] no matching package named `foo` found
location searched: `dummy-registry` index (which is replacing registry `crates-io`)
required by package `bar v0.5.0 ([ROOT]/foo)`
As a reminder, you're using offline mode (--frozen) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--frozen`.

"#]])
.run();
Expand Down Expand Up @@ -2467,19 +2463,10 @@ fn disallow_network_git() {
p.cargo("check --frozen")
.with_status(101)
.with_stderr_data(str![[r#"
[ERROR] failed to get `foo` as a dependency of package `bar v0.5.0 ([ROOT]/foo)`

Caused by:
failed to load source for dependency `foo`

Caused by:
Unable to update registry `crates-io`

Caused by:
failed to update replaced source registry `crates-io`

Caused by:
attempting to make an HTTP request, but --frozen was specified
[ERROR] no matching package named `foo` found
location searched: `dummy-registry` index (which is replacing registry `crates-io`)
required by package `bar v0.5.0 ([ROOT]/foo)`
As a reminder, you're using offline mode (--frozen) which can sometimes cause surprising resolution failures, if this error is too confusing you may wish to retry without `--frozen`.

"#]])
.run();
Expand Down