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

Closed
wants to merge 1 commit into from

Conversation

joshtriplett
Copy link
Member

This avoids hardcoding is_ephemeral && ignore_lock elsewhere in the
codebase.

This avoids hardcoding `is_ephemeral && ignore_lock` elsewhere in the
codebase.
@rust-highfive
Copy link

r? @ehuss

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2022
Copy link
Member

@Rustin170506 Rustin170506 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@@ -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.

Comment on lines +565 to +568
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
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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants