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.

Sign up for GitHub

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 Workspace::is_install to detect cargo install #10966

Closed
wants to merge 1 commit into from
Closed

Add Workspace::is_install to detect cargo install #10966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 7 additions & 0 deletions src/cargo/core/workspace.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,13 @@ impl<'cfg> Workspace<'cfg> {
self.member_ids.contains(&pkg.package_id())
}

/// Returns true if this workspace is being used for a `cargo install` operation
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// Returns true if this workspace is being used for a `cargo install` operation
/// Returns true if this workspace is being used for a `cargo install` operation.

pub fn is_install(&self) -> bool {
// For now, this condition detects `cargo install`, but calling `is_install` allows us to
// more easily change that in the future.
self.is_ephemeral && self.ignore_lock
Comment on lines +565 to +568
Copy link
Contributor

Choose a reason for hiding this comment

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

cargo install --locked is still an install but wouldn't this function return false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, good point. I did this based on inspiration from #10891 (comment) . @ehuss, is there a variant of this that would make that code clearer, or should I just drop this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think there is a good way to detect if it is an install. Historically we have tried to avoid making that kind of information available since it is an abstraction loss (the lower-level build code shouldn't know about higher-level concepts like "install").

I think it might be ok to add something to BuildConfig to indicate which command initiated a build. The risk is that it will be abused for changing behavior based on the command. Perhaps if there is a stern comment emphasizing it should only be used for diagnostics would help?

I wouldn't make this a property of the workspace. I forgot to also say that is_ephemeral is only true when installing something from a registry (not a local or git install).

}

pub fn is_ephemeral(&self) -> bool {
self.is_ephemeral
}
Expand Down
14 changes: 5 additions & 9 deletions src/cargo/ops/cargo_compile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,13 +661,11 @@ pub fn create_bcx<'a, 'cfg>(
continue;
}

let guidance = if ws.is_ephemeral() {
if ws.ignore_lock() {
"Try re-running cargo install with `--locked`".to_string()
} else {
String::new()
}
} else if !unit.is_local() {
let guidance = if ws.is_install() {
"Try re-running cargo install with `--locked`".to_string()
} else if ws.is_ephemeral() || unit.is_local() {
String::new()
} else {
format!(
"Either upgrade to rustc {} or newer, or use\n\
cargo update -p {}@{} --precise ver\n\
Expand All @@ -678,8 +676,6 @@ pub fn create_bcx<'a, 'cfg>(
unit.pkg.name(),
current_version,
)
} else {
String::new()
};

anyhow::bail!(
Expand Down