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

Make cargo install respect lockfiles by default #3585

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

casey
Copy link

@casey casey commented Mar 10, 2024

Rendered

Currently, cargo install does not respect lockfiles by default, which causes breakages when dependencies release breaking but semver-compatible versions. This RFC proposes to change cargo install to respect lockfiles by default.

There's been a lot of discussion on rust-lang/cargo#7169 without much progress, so I thought it would be productive to open an RFC proposing changing the default behavior of cargo install to respect lockfiles by default, to provide a concrete proposal that can be discussed.

Even if this RFC is ultimately rejected, it will provide some forward progress on the issue, since we can then focus on an alternative to changing the default behavior.

@epage
Copy link
Contributor

epage commented Mar 11, 2024

There's been a lot of discussion on rust-lang/cargo#7169 without much progress, so I thought it would be productive to open an RFC proposing changing the default behavior of cargo install to respect lockfiles by default, to provide a concrete proposal that can be discussed.

An RFC isn't the best tool to jumpstart progress but instead engaging with the issue is. If there is enough to move forward, i then recommend using Internals wxth a Pre-RFC as a sounding board to help flesh it out.

For myself, i felt this misses a lot of the nuance for why things are the way they are. Without a clear understanding of the problem, you can't talk much on possible solutions. Personally i would recommend closing this while that is iterated on and then going through the process i mentioned. I expect the conversations would then be much more productive, The caveat ill give is the Cargo team just went through a very contentious RFC and i at least am wanting to rebuild my energy before helping mentor the design of this. I can't speak for the others.

@epage epage added the T-cargo Relevant to the Cargo team, which will review and decide on the RFC. label Mar 11, 2024
@casey
Copy link
Author

casey commented Mar 11, 2024

An RFC isn't the best tool to jumpstart progress but instead engaging with the issue is. If there is enough to move forward, i then recommend using Internals wxth a Pre-RFC as a sounding board to help flesh it out.

I definitely understand the sentiment. My thought here is that the issue has been known and talked about for a long time, without seeing much progress. Even rejecting this RFC would be beneficial, since I think a lot of people would like the return of the pre-1.37 behavior of respecting lockfiles, and if that's off the table, the conversation can move forward productively to think about alternatives.

For myself, i felt this misses a lot of the nuance for why things are the way they are. Without a clear understanding of the problem, you can't talk much on possible solutions.

I think I touched on this with:

Currently, when running cargo install, the latest semver-compatible versions
of dependencies will be selected. This can be beneficial if these versions have
bugfixes.

Is there something else I'm missing? I checked out the original PR, and only saw picking up bugfixes and new features as the reason to ignore lockfiles.

Personally i would recommend closing this while that is iterated on and then going through the process i mentioned. I expect the conversations would then be much more productive, The caveat ill give is the Cargo team just went through a very contentious RFC and i at least am wanting to rebuild my energy before helping mentor the design of this. I can't speak for the others.

I'm definitely happy to close this, I don't feel too attached to this PR. However, there has been a lot of discussion in rust-lang/cargo#7169, and my personal read of the discussion there is that many users would prefer that cargo install respect lockfiles, and that at least proposing to do that with an RFC is reasonable. I think I understand the tradeoffs involved, and I personally think that they are in favor of respecting lockfiles, and I'm not sure that much more discussion would help.

@Aloso
Copy link

Aloso commented Mar 11, 2024

Even rejecting this RFC would be beneficial, since I think a lot of people would like the return of the pre-1.37 behavior of respecting lockfiles

From what I can tell, the behavior didn't change in 1.37. From the linked issue:

Ah, yea, in that sense there is no change in behavior. I mis-remembered how it used to work. The present stable behavior for install --path is that Cargo.lock is ignored by default, and --locked can be used to honor it. That hasn't changed. (What did change is installing from a registry, which didn't support lock files at all.)

@casey
Copy link
Author

casey commented Mar 11, 2024

From what I can tell, the behavior didn't change in 1.37. From the linked issue:

I believe the change in behavior was to cargo install <CRATE_NAME>, i.e., installing from crates.io.

@KirilMihaylov
Copy link

Correct me if I am mistaken, but isn't that why the --locked exists in the first place? If this becomes the default behavior, then a new flag has to be introduced specifying that it should update the dependencies.

This could be potentially harmful if you try installing a package that hasn't updated its dependencies yet, while there is a wild CVE going on, e.g. one of the TLS crates.

@BurntSushi
Copy link
Member

BurntSushi commented Mar 11, 2024

since I think a lot of people would like the return of the pre-1.37 behavior of respecting lockfiles

My memory is that this was never the case. cargo install binary-crate-from-crates.io never respected lock files. Could you say where you got the idea that it used to respect lock files from?

EDIT: With hands on keyboard and some digging, yes, cargo install bin never respected lock files. In particular, there was a time when Cargo.lock wasn't even published with the package to crates.io. See rust-lang/cargo#2263. And the fact that cargo install bin doesn't use a lock file by default has been biting me for a long time.

@epage
Copy link
Contributor

epage commented Mar 11, 2024

I definitely understand the sentiment. My thought here is that the issue has been known and talked about for a long time, without seeing much progress.

While the problem has been talked about for a while, not all of that has been productive and there is more productive conversation to be had. If nothing else, a summary of the state of the thread would be a big help.

Even rejecting this RFC would be beneficial, since I think a lot of people would like the return of the pre-1.37 behavior of respecting lockfiles, and if that's off the table, the conversation can move forward productively to think about alternatives.

In my mind, this RFC does not produce a good platform for us to outright reject. If I were to make a formal proposal to the cargo team for this, it would be to postpone this RFC rather than reject it.

Is there something else I'm missing?

Yes but, as I said, I'm staying out of this. You will either need to find another mentor or take on yourself a lot more of the burden to move this forward.

However, there has been a lot of discussion in rust-lang/cargo#7169, and my personal read of the discussion there is that many users would prefer that cargo install respect lockfiles, and that at least proposing to do that with an RFC is reasonable

rust-lang/cargo#2644 is our most upvoted and commented on issue and people would have likely said the same thing. With a gentle nudge, someone dug into it more and found there were fundamental problems with what "many users would prefer". While there are differences with this situation, I hope this clarifies that that isn't a metric of its own to base designs decisions off of.

ijackson added a commit to ijackson/macrotest that referenced this pull request Mar 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-cargo Relevant to the Cargo team, which will review and decide on the RFC.
Projects
Status: Maybe postpone
Development

Successfully merging this pull request may close these issues.

None yet

5 participants