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

"cargo install" apparently ignores "Cargo.lock" as opposed to "cargo build" #7169

Open
Tracked by #84
df5602 opened this issue Jul 23, 2019 · 90 comments
Open
Tracked by #84
Labels
Command-install S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@df5602
Copy link

df5602 commented Jul 23, 2019

I've encountered a strange issue today.

Background: The percent-encoding crate recently made a new major release. Knowing that stuff could break I held off on updating the crate because I haven't yet had time to check what changed.

Problem
When I compile my binary crate using cargo build (with and without --release) everything builds fine. However, if I compile my binary using cargo install --path . -f, compilation fails with an error related to percent-encoding (the breaking change warranting the major version bump I assume..). In the output I see that it compiled percent-encoding v2.0.0 instead of percent-encoding v1.0.1 as specified in Cargo.lock. (Note: The Cargo.toml doesn't specify a specific version.)

The crate is the following, if you want to try at home:
https://github.com/df5602/bingers

Steps

  1. $ cargo clean
  2. $ cargo build
...
Compiling percent-encoding v1.0.1
...
Finished dev [unoptimized + debuginfo] target(s) in 48.32s
  1. $ cargo build --release
...
Compiling percent-encoding v1.0.1
...
Finished release [optimized] target(s) in 1m 25s
  1. $ cargo install --path . -f
  Installing bingers v0.1.0 (/path/to/bingers)
    Updating crates.io index
   Compiling percent-encoding v2.0.0
   Compiling bingers v0.1.0 (/path/to/bingers)
error[E0432]: unresolved import `percent_encoding::QUERY_ENCODE_SET`
  --> src/tvmaze_api.rs:10:45
   |
10 | use percent_encoding::{utf8_percent_encode, QUERY_ENCODE_SET};
   |                                             ^^^^^^^^^^^^^^^^ no `QUERY_ENCODE_SET` in the root

error: aborting due to previous error

I can work around it by specifying an explicit version in Cargo.toml, but the observed behaviour was surprising to me...

Notes

Output of cargo version:

cargo 1.36.0 (c4fcfb725 2019-05-15)

OS: Mac

@df5602 df5602 added the C-bug Category: bug label Jul 23, 2019
@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2019

This has been changed in 1.37 (beta). Starting in that version the --locked flag can be used to force cargo to use the Cargo.lock file.

The intent is that cargo install will behave the same whether it is from a registry or a local path.

We discussed this a bit (#6840 (review) is most relevant). Ideally Cargo would issue a message with a hint to try --locked, but I'm not so confident that would work well. I think since it can result in any error, it wouldn't be able to know if --locked would actually fix it, so I think it would be misleading too often.

@df5602
Copy link
Author

df5602 commented Jul 23, 2019

  1. I'm using cargo 1.36 (stable) which shouldn't show that behaviour yet, right? (Running cargo install with --locked worked, though...)

  2. Reading up on the linked issue: if I understand correctly, going forward, the behaviour will be different between cargo build and cargo install regarding dependency resolution (at least for binary crates)? If yes, I would consider that as very surprising! (So, what's the Cargo.lock for? Basically like this, you compile, run your tests and then cargo install possibly picks different dependencies?)

I know, I should probably fix my Cargo.toml and specify proper versions instead of using "*". But until now, it never really mattered for binary crates. Updating dependencies was always a manual process, so that you can deliberately audit the changes if you wish to do so (for me it was basically running cargo outdated, check the listed crates and look for the changelog, and eventually running cargo update -p xyz).
If that behaviour changes, I would expect at least a warning à la "Warning: Ignoring lock file!" and probably a PSA in the Rust release notes.

@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2019

1. Running `cargo install` with `--locked` worked, though...

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

It's difficult to balance the defaults here. I think it would be confusing if cargo used Cargo.lock differently based on the source. And for now we don't want to default to using Cargo.lock for installing from a registry. I think that's up for debate on changing it, but it was the conservative choice we made for now. If we ever decide to change it, we would probably want to add a flag to override it, and that adds more complexity.

@df5602
Copy link
Author

df5602 commented Jul 23, 2019

Ah ok, so I guess it was the first time I ran into this because I deliberately held off on updating a dependency (due to breaking changes) which happens quite rarely...

It's good to know that this is the default behaviour, but I think this should be a lot better documented, because it's highly unintuitive and surprising:
Until now, my mental model of cargo install --path was basically "run cargo build --release and copy the binary somewhere". But apparently that's wrong, it should be "compile the binary slightly differently than cargo build --release and copy the binary somewhere".

I think it's the wrong default for binary crates because it means that you're deploying different binaries than you're testing! (In my opinion it would be ok, if cargo build would ignore lockfiles by default and you could explicitly opt-in to using lockfiles. But currently the default is otherwise.)

@ehuss
Copy link
Contributor

ehuss commented Jul 23, 2019

I think this should be a lot better documented

There's a fairly lengthy paragraph on https://doc.rust-lang.org/nightly/cargo/commands/cargo-install.html that explains how cargo install handles Cargo.lock (this is new for 1.37, so it hasn't hit stable, yet). If there's any part that could be clarified let us know, we're always eager to improve the documentation.

@df5602
Copy link
Author

df5602 commented Jul 24, 2019

I assume this will also be part of the cargo install --help page? I think the behaviour is described very clearly in the updated help page.

Given that the described behaviour is apparently expected behaviour, I think we can close this issue?

@MaulingMonkey
Copy link

MaulingMonkey commented Jul 25, 2019

https://doc.rust-lang.org/nightly/cargo/commands/cargo-install.html has some misleading documentation under "Manifest Options" for --frozen / --locked:

The --frozen flag also prevents Cargo from attempting to access the network to determine if it is out-of-date.

This is untrue for cargo install. In fact, I don't think this can ever succeed for install unless the crate has no dependencies. Maybe not even then.

These may be used in environments where you want to assert that the Cargo.lock file is up-to-date (such as a CI build) or want to avoid network access.

This is inaccurate for cargo install. If anything, you'd use these flags to use an "out-of-date" lock file - in the sense that all the documentation suggesting cargo install xyz will be using different versions.

If the lock file is missing, or it needs to be updated ...

This is technically accurate, but the way it implies it might not need to be updated is kinda confusing, as which crates are used will always be updated without these flags - for the install build at least - regardless of if they "need to be" or not.

If this documentation is automatically shared with other commands, it might at least be worth breifly mentioning cargo install being a special case. Maybe something like:

Unlike cargo build, cargo install will ignore Cargo.lock files completely without --locked."

?

Ideally Cargo would issue a message with a hint to try --locked, but I'm not so confident that would work well.

Instead of trying to guess when an error can be fixed by --locked, it might be useful to instead warn if Cargo.lock is present, and specifies different versions than are being used, since that's just begging for version confusion and building everything twice needlessly. --locked is one possible fix, but cargo update is another - since presumably most docs suggest cargo install xyz rather than cargo install xyz --locked?

@MaulingMonkey
Copy link

(I also suspect I landed on the "Manifest Options" section by searching the cargo install docs page for "--frozen", which caused me to skip over the earlier/better explaination of locking behavior entirely.)

@ehuss
Copy link
Contributor

ehuss commented Jul 28, 2019

I assume this will also be part of the cargo install --help page?

Not for now, unfortunately we don't share the man pages with the help text. The long-term intent is to make it available in some fashion.

If this documentation is automatically shared

Yes, it is shared. We can probably make it special-cased for install, but that would take a bit of restructuring.

@df5602
Copy link
Author

df5602 commented Jul 29, 2019

In that case I take back what I said earlier: I don't think I'd have the idea to look-up a description of a command online, if said command also has an extensive man page, so I still consider it undocumented behaviour...

I also agree with @MaulingMonkey: If this is the desired behaviour, then it should at least generate warnings, something along those lines (modulo wordsmithing):

Warning: Ignoring `Cargo.lock` file!
Detecting version mismatch:
    | Specified in `Cargo.lock` | Used in build
------------------------------------------------
foo |                    v1.0.1 |        v1.0.3

... possibly followed by a few suggestions on how to deal with the situation (i.e. use --locked, run cargo update, etc.)

(Although I don't know the inner workings of cargo, so I can't judge how much work that would be...)

@Nemo157
Copy link
Member

Nemo157 commented Sep 11, 2019

(EDIT: Moved to #7495)

@ehuss ehuss removed the C-bug Category: bug label Sep 21, 2019
@matklad
Copy link
Member

matklad commented Oct 7, 2019

I've hit this in rust-analyzer a couple of times, and I would appreciate the warning. How does this interact with publish-lockfile? Will adding publish-lockfile = true to Cargo.toml make Cargo to use lockfile when installing? Or is the publish-lockfile feature effectively a no-op?

@ehuss
Copy link
Contributor

ehuss commented Oct 7, 2019

How does this interact with publish-lockfile?

publish-lockfile has been removed and is a no-op now. Cargo now automatically publishes the lock file if there are any binaries in the package. cargo install always ignores the Cargo.lock unless you specify --locked.

@nixpulvis
Copy link

nixpulvis commented Oct 7, 2019

Let me see if I'm understanding this correctly, because lockfiles have been a pet-peeve of mine in other ecosystems (looking at you NPM).

  • cargo build will respect the Cargo.lock file installing only dependencies that are known to work, since those are the versions everyone is building with
  • cargo install will ignore the Cargo.lock file, installing the latest matching versions with respect to the Cargo.toml file, presumably because there are security or other important patching an installing user may want

This seems wrong, since it seems like this whole setup assumes crates are following semantic versioning, strictly. As far as I'm concerned cargo build is to make as cargo install is to make install (as things are typically implemented). If we have a lockfile, both operations should respect it. How else will I know that people's builds (and installs) will work!?

As the author of Rust software, I want to use cargo update to try to update my dependencies, but I might hit problems, and they'll need to be resolved before I publish my updated lockfile. Of course, an ideal release of software is tested in some sense!

On the other hand I may find myself using only dependencies who follow "semantic versioning" effectively, and thus may decide to offer users automatic updates for dependencies. In this case I'd need to lock non-conforming dependencies explicitly in the Cargo.toml moving forward.

Now, since we do allow published crates to be installed remotely, and some of these crates will become stale, it makes sense to allow new users of software correctly following semantic versioning to automatically get the updates. But this should be an opt-in process, since not all software follows semantic versioning (or is generally capable of handling automatic updates to dependencies).

I might be missing a lot of details for why this next idea won't work, but in an automatically updating setup (like one following semver), I think I'd expect to not have a Cargo.lock file at all.

@ehuss
Copy link
Contributor

ehuss commented Oct 7, 2019

I personally would be fine with switching the default cargo install behavior, as I feel reliable builds are more important than possibly getting "fixed" dependencies. But there has been resistance to this in the past. I think I have mitigated some of the concern by issuing warnings on yanked dependencies. But presuming few use --locked, we don't really know what the impact of that will be. The warning isn't very helpful, since most users will have no idea what it means or control over updating those dependencies, so I imagine it has a risk to be confusing at the least.

There's also a bit of risk that this would change current behavior, which some projects may be relying upon in some way. I'm not sure how things like dh-cargo would be impacted, or what they would prefer.

@ojeda
Copy link

ojeda commented Oct 9, 2019

It is surprising, indeed, and a security concern too: consider projects/apps that may just provide source tarballs or a repo somewhere, specially small ones within the Linux ecosystem.

End-users will download a package and then run cargo install. They may or may not know about Rust/Cargo. Now:

  • If they are lucky, the binary works as intended.
  • If they are not so lucky, it doesn't.
  • And if they are really unlucky, arbitrary nasty code just got executed.

And all this happens even if they have verified the checksums/signatures of their downloads or their checked out signed Git tags!

I would say:

  • At least both build and install should behave the same in all respects (except actually installing, of course). I believe this is what users of traditional build systems expect.
  • Both commands should default to failure if Cargo.lock needs to be modified for any reason. For binaries, they should also fail if it has to be created. This makes the behavior more consistent by default (i.e. whether Cargo.lock is there does not change behavior of build), makes the act of updating the dependencies explicit, whether via update or a shorthand, and ensures source releases work as intended.

@anguslees
Copy link

(This is another example of "I don't want anything to change, but I also want new changes". It's surprising how often this obviously (when expressed in these simple terms) irreconcilable conflict arises :P )

@NickeZ
Copy link

NickeZ commented Jan 6, 2020

As a "simple user" I find this behavior completely unexpected and therefore breaks the most important rule of all rules when it comes to software development: https://en.wikipedia.org/wiki/Principle_of_least_astonishment

I have no idea if you have the power to change this, but I would change it even if it breaks backwards compatibility. AFAIU cargo install will strictly work more often than currently by changing the default behavior to enable --locked. Are there any cases where cargo install would not work if it used --locked?

Edit: Also I looked like an idiot reporting this issue in the "Alacritty" repository when this bug is in Cargo. And this is like the second most important rule in UX. Never make your users look stupid.

@nixpulvis
Copy link

nixpulvis commented Jan 6, 2020

@NickeZ I think the "case" that won't work is automatic (potentially security related) updates for downstream installers. However, as I've mentioned above, I believe that should be handled differently. Also, don't worry too much about looking like an idiot. This behavior surprised me too, and having some discussion on it in a now closed issue doesn't bother us.

@ghost
Copy link

ghost commented Feb 27, 2020

Our team got hit by this today and we found this behavior to be very surprising. Consistency is the key here, defaulting to use lock file for something and not for others are very inconsistent / surprising / confusing.

@chrisduerr
Copy link
Contributor

@est31 Having a lockfile that automatically gets updated is just the worst of both worlds. The reason lockfiles are great because it ensures the output binary behaves exactly the same, not just that the output compiles. If you updated them automatically you'd just run into more esoteric issues that are essentially the same.

If I'm uploading a binary to crates.io, I want people to run exactly what I released. I do not want any dependencies updated, even if those are "security" updates. That should be up to me.

@ssokolow
Copy link

ssokolow commented Mar 14, 2024

If I'm uploading a binary to crates.io, I want people to run exactly what I released. I do not want any dependencies updated, even if those are "security" updates. That should be up to me.

You are aware, I hope, that there are already a lot of people who have a low opinion of distribution channels that aren't Linux distro packages because of how much responsibility they place on the upstream maintainer to respond promptly to security advisories.

(i.e. "Look at how badly Docker failed on ensuring a steady supply of security updates! Flatpak and statically linked languages like Go and Rust are evil and should not be allowed!")

...and Cargo.lock doesn't have the dodge Flatpak does, that the things which are most likely to need security updates are in the runtime, not the package.

I don't think there's a simple solution to reconcile the two conflicting needs here.

Not to mention that we don't know how places that focus on giving the downstream the tools to protect themselves (eg. lib.rs) would react to packages with stale lockfiles if something like this doesn't include a mechanism for allowing Crates.io to overrule Cargo.lock pins for packages when the maintainer is going through a bad bit and isn't in a state of mind to push an update that would have just gone through without a Cargo.lock.

We could easily wind up with packages getting tarred with a bad reputation because they don't meet some externally imposed standard for how quickly security fixes must be rolled out... heck, imposing unreasonable demands for prompt security response on hobby projects was one of the things that sparked a lot of fear in the earlier drafts of the E.U.'s new rules for safer software infrastructure.

@chrisduerr
Copy link
Contributor

You are aware, I hope, that there are already a lot of people who have a low opinion of distribution channels that aren't Linux distro packages because of how much responsibility they place on the upstream maintainer to respond promptly to security advisories.

Yes, that includes myself. But this just makes it worse, not better. If you're asking me, then this does nothing (or close to nothing) for security but has significant impact on reliability. You can't just cargo update some random abandoned project and pretend that suddenly makes it secure and up-to-date.

And it's not like cargo has a good way of updating binaries installed through it anyway. So the people most likely to be using outdated dependencies, even if upstream has already pushed a new lockfile, are the most vulnerable. Not the ones just installing it.

We could easily wind up with packages getting tarred with a bad reputation because they don't meet some externally imposed standard for how quickly security fixes must be rolled out... heck, imposing unreasonable demands for prompt security response on hobby projects was one of the things that sparked a lot of fear in the earlier drafts of the E.U.'s new rules for safer software infrastructure.

Updating the lockfile changes almost nothing about this. Security issues are introduced in binaries regularly, not just libraries. And it's also possible that a breaking change is required to fix them. If people don't update their software, it's not safe to use, that's just the nature of the beast.

@ssokolow
Copy link

ssokolow commented Mar 14, 2024

You can't just cargo update some random abandoned project and pretend that suddenly makes it secure and up-to-date.

As a maintainer of some projects I don't yet feel are ready to push to crates.io, I find that GitHub Dependabot is quite good at offering me dependency security fix PRs that pass all my GitHub Actions runs while only doing the kind of semver bumping that cargo install does implicitly.

(So long as the dependency in question actually offers a minor version update for Dependabot to generate a security-update PR from, of course.)

Updating the lockfile changes almost nothing about this. Security issues are introduced in binaries regularly, not just libraries. And it's also possible that a breaking change is required to fix them. If people don't update their software, it's not safe to use, that's just the nature of the beast.

That smells like a "Perfect solution or status quo. There's no such thing as an incremental improvement." argument to me.

...especially in projects like mine, where I'm generally cautious enough about how I design my things that any vulnerabilities are far more likely to be in my dependencies than in how I'm gluing them together.

In the real world, people install unmaintained software. It's just human psychology that "I need this solved now" is a more significant psychological input than "there might be a hidden flaw in this which could bite me later".

If anything, the current cargo install behaviour could be considered safer because, not only does it provide some level of protection against stale dependencies, it's more likely to turn "unmaintained" into "adding a speed-bump for users who might otherwise blindly install unmaintained software".

@charles-r-earp
Copy link
Contributor

At the very least, Cargo and crates-io could provide more of a hint to users to use --locked when the package has a lockfile.

Packages published with bins always have a lockfile.

We do suggest --locked when a dependency is found that isn't compatible with the toolchain you are using. I think it would be a good quality of life improvement to also suggest --locked if there is a compilation error. Even better if we can limit the suggestion to non-local dependencies. I also suspect we should print the project's homepage URL on error to help connect users with where to go for support.

It is nice that cargo does include a hint to try using --locked when install fails, but I was suggesting that this be a warning upfront. And in the case of crates-io, it suggests to do cargo add or add the package to the manifest, which is of course a current issue. So it would be nice if when that was fixed, in addition to suggesting to install the crate, it could hint that the package has a lock file that can be used via --locked.

@casey
Copy link
Contributor

casey commented Mar 17, 2024

Since concrete instances of issues stemming from not respecting lockfiles are well understood and documented, it would be useful to collect information on concrete instances of the benefits of respecting lockfiles by default.

It would be very helpful to know:

  • When has respecting lockfiles by default prevented binary crates from being compiled and installed with bugs, when those bugs would have been prevented by using the latest semver-compatible versions of dependencies?

  • When has respecting lockfiles by default prevented binary crates from being compiled and installed with security vulnerabilities, when those vulnerabilities would have been prevented by using the latest semver-compatible versions of dependencies? I mention security vulnerabilities separately from bugs, since avoiding security vulnerabilities is often called out specifically as one of the benefits of not respecting lockfiles by default.

For either of these to be evidence that not respecting lockfiles is beneficial, it isn't enough for a crate a dependency to have a bug or security vulnerability, and the semver-compatible version that is selected by cargo install to not have the bug or security vulnerability. Crates often use only a subset of the features of dependencies, a bug or security vulnerability might exist in a dependency that cannot be encountered when using the compiled binary. So, it must be possible to encounter the bug when using the dependent.

@casey
Copy link
Contributor

casey commented Mar 17, 2024

Other things which would be useful to know:

  • How many crates on crates.io build with cargo install --locked but not with cargo install? The opposite is possible as well, although less likely.
  • Which crates have tests where one of cargo test and cargo update && cargo test passes and the other fails? This is a pretty strong signal of respecting lockfiles being good or bad. If cargo test passes but cargo update && cargo test fails, then new dependencies are introduced which break the crate. If cargo install succeeds in that case, then that's doubly bad, because cargo install may have installed a binary with buggy behavior that was introduced by cargo update.
  • Over all crates on crates.io, which dependencies are selected by cargo install --locked, and how do they differ from the dependencies in the lockfile? Inspecting each diff by hand would be impractical, but it would be easy to compile a database with statistics of the different versions that have been selected.

Jake-Shadle added a commit to EmbarkStudios/cargo-deny that referenced this issue Mar 23, 2024
Fucked again by `cargo install` not using the lockfile unless explicitly
specified.

rust-lang/cargo#7169

Resolves: #641
@ckcr4lyf
Copy link

ckcr4lyf commented May 9, 2024

There's also the extreme case of "protestware", e.g. author intentionally introduces a breaking change in patch version. Now sure, this would eventually get caught by cargo mods (I am not entirely sure whom), but if I want to protect my users, I would want my lockfile to be respected.

Commenting on this point a bit more with the whole xz backdoor thing - if my binary package relies on a lib that got social engineered into someone being able to introduce a backdoor (and publish it as patch version), the current default cargo behavior would end up "upgrading" to the backdoored file if one of my users did cargo install without --locked.

I think this is another good case for locked to be the default, if the author of a project has a lockfile in place.

@ssokolow
Copy link

ssokolow commented May 9, 2024

I still think studies need to be done to calculate how often the current behaviour pushes out security updates faster than the dev updates their lock files vs. how often it prevents an exploit from being rolled out.

The xz backdoor thing is certainly dramatic, but we don't want to succumb to what I'll call argumentum ad dramatum for now. Reasoning like that leads to people being so scared of dying in an airplane that they discount how you're much more likely to die in the car on the way to the airport.

@Darkcashi86
Copy link

Como encuentro una dirección

@olson-dan
Copy link

I saw that there was a related RFC but it seems this issue is still somewhat active. I ran into significant problems with this trying to use cargo install from a docker container build step against an older Rust version.

It is my belief that the current behavior is broken. Having done some small spelunking it appeared that it was only chosen back in 1.39 because there was a desire not to change away from the previous behavior. At this point it's clearly correct to invert the default and provide an --unlocked option, but since this is controversial I want to propose an alternative:

cargo install should default to the --locked behavior when specifying a specific version with the --version argument. This gives users just running it from command line to install software access to both the security fixes and semver breakages they know and love, while giving people trying to install an older version for any reason some slim hope of a compile that succeeds.

I want to briefly mention that the problem of building an older version is compounded by MSRV which is not respected by cargo when choosing the specific dependency to build against. That's a separate issue. I only mention it because the specific task of building anything against an older Rust version is a major pain point with the tooling right now, and making this small change to cargo install will go a long way towards helping.

@RalfJung
Copy link
Member

Here we can see our own infra team being surprised by npm not using a lockfile when installing binaries, and this is causing issues with CI failures right now. The same arguments would also apply to cargo.

@ckcr4lyf
Copy link

ckcr4lyf commented Dec 11, 2024

Here we can see our own infra team being surprised by npm not using a lockfile when installing binaries

FYI, the problem here is that the publishers of datadog-ci have not uploaded a lockfile to npm. you can verify this by checking out https://www.npmjs.com/package/@datadog/datadog-ci?activeTab=code. package.json is more like Cargo.toml and is not a lockfile. package-lock.json or npm-shrinkwrap.json are the npm versions of lockfiles.

It's a separate matter that binary packages should use npm-shrinkwrap.json since that's what is uploaded to the npm registry.

If npm-shrinkwrap.json is present, then npm WILL use it when installing binaries (e.g. my personal project: https://www.npmjs.com/package/qbit-race?activeTab=code)

@RalfJung
Copy link
Member

RalfJung commented Dec 11, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Command-install S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests