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 --path" ignores lockfile #6983

Closed
RalfJung opened this issue May 26, 2019 · 11 comments
Closed

"cargo install --path" ignores lockfile #6983

RalfJung opened this issue May 26, 2019 · 11 comments
Labels
C-bug Category: bug

Comments

@RalfJung
Copy link
Member

Problem

In a project with a local lockfile, cargo install --path . seems to ignore that lockfile.

Steps

  1. Have a lockfile with some outdated dependencies.
  2. Do cargo install --path .

This will use the latest version of the dependency even though the lockfile says otherwise.

Possible Solution(s)

I suggest that cargo install --path . should respect the lockfile in that directory.

Notes

Output of cargo version:
cargo 1.36.0-nightly (c4fcfb7 2019-05-15)

@RalfJung RalfJung added the C-bug Category: bug label May 26, 2019
@ehuss
Copy link
Contributor

ehuss commented May 26, 2019

You can tell it to use the lock file with --locked.

This was intentionally changed in #6840 so that cargo install is consistent with all source kinds. That is, cargo install from a registry also ignores the lock file by default.

@RalfJung
Copy link
Member Author

I guess it's a tough call whether to be consistent with buildor install from a registry.

@ehuss
Copy link
Contributor

ehuss commented May 26, 2019

I agree, and I'm on the fence on this change. And now I'm questioning whether or not ignoring lock files really makes sense at all. 😖

@RalfJung
Copy link
Member Author

RalfJung commented May 26, 2019

Hm, but there must also be something else going on... I am working on miri with this lockfile. After doing cargo build --release, a second cargo build --release completes instantaneously. I would expect that cargo install --path . --locked -f would just copy the binaries, but instead it takes a minute:

$ cargo install --path . --locked -f
  Installing miri v0.1.0 (/home/r/src/rust/miri.2)
    Updating crates.io index
   Compiling num-traits v0.2.8
   Compiling num-integer v0.1.41
   Compiling chrono v0.4.6
   Compiling vergen v3.0.4
   Compiling miri v0.1.0 (/home/r/src/rust/miri.2)
    Finished release [optimized] target(s) in 1m 00s

(a) why does it update the index when it is using the lockfile, and (b) why is it rebuilding anything?

@ehuss
Copy link
Contributor

ehuss commented May 26, 2019

(a) It updates the index because it is checking for yanked dependencies (it will issue a warning if any are found).
(b) The reason that it compiles differently is because cargo install ignores [dev-dependencies]. In this case, this means that num-traits is compiled without any default features. That is:

vergen → chrono → num-traits (NO DEFAULT FEATURES)
vergen → chrono → num-integer → num-traits (NO DEFAULT FEATURES)

whereas with a regular build, dev-dependencies are included in the calculus, and:

colored → winconsole → cgmath → num-traits 0.1.43 → num-traits 0.2.8 (WITH DEFAULT FEATURES, which is std).

and due to feature unification, num-traits is built once with the std feature (and built without in cargo install).

Should probably document somewhere that dev-dependencies are ignored for cargo install. It should also be easier to see how features affects compilation.

@RalfJung
Copy link
Member Author

RalfJung commented May 26, 2019

Thanks! That makes sense. How can I find out the offending crate chain / feature when this happens again?^^

So is there a way that I can avoid having to rebuild anything when I do either of cargo build --release or cargo install first, and then do the other? (Parts of Miri only work when installed, but of course for the test suite I need to build, so I regularly switch between the two.)

I guess a hacky solution would be to just move all dev-dependencies to "full" dependencies.

@RalfJung
Copy link
Member Author

Btw, even if the current behavior stays, the documentation of the --locked flat could probably be improved:

--locked                 Require Cargo.lock is up to date

Says nothing about "without this the lock file is ignored". (To be fair, I didn't check the --help before reporting here because I thought it was a bug, but it would still help. :D )

@ehuss
Copy link
Contributor

ehuss commented May 26, 2019

Understanding how features resolve is difficult. The process I used above:

  1. Run cargo build -v and look at the --cfg flags passed. Notice that they were different from cargo install.
  2. Run cargo tree to figure out where num-traits is used. (Briefly curse because it doesn't work with cargo-features in the miri repo.)
  3. Look at the Cargo.toml file for each crate that uses num-traits, and see which features it enables.
  4. Take the union of all features for num-traits.

Although not really relevant, it is also complicated by num-traits including a build script which enables the has_i128 feature, but that doesn't affect resolution. Also, num-traits 0.1.43 does the trick of depending on a future version of itself.

It would probably be best to have something like cargo tree include details on features (see sfackler/cargo-tree#18), to make debugging issues like this easier. That is probably not an easy thing to do.

cargo build --release -Z avoid-dev-deps should result in the same behavior as cargo install.

As for the documentation, we are planning to stabilize the "publish lockfile" feature very soon, and as part of that I was planning on updating the documentation. I didn't really expect anyone to notice this change. 😝

@RalfJung
Copy link
Member Author

cargo build --release -Z avoid-dev-deps should result in the same behavior as cargo install.

But then when I do cargo test --release, it'll rebuild the main binary? Or not?

I didn't really expect anyone to notice this change.

I know that feeling. ;)

@ehuss
Copy link
Contributor

ehuss commented May 26, 2019

But then when I do cargo test --release, it'll rebuild the main binary? Or not?

I believe it will be rebuilt.

You could add a direct dependency to num-traits 0.2 with the default features enabled to prevent it from switching during cargo install.

This is generally a difficult problem, and I haven't really come to a conclusion on how it should work. It could build num-traits twice, once with std and once without. But that means most projects would incur a fairly significant increase in build times (and possibly executable sizes). It would also mean that some projects would have compile errors because the types between the two num-traits crates would no longer be compatible. For this specific case, something like automatic features might help where it would be smarter about when std is enabled. However, that wouldn't help in all situations like this. Leveraging public-private dependencies might help with detecting when it would be safe to compile a crate multiple times with different features, but that is complicated, and might only help in a limited number of situations.

@ehuss
Copy link
Contributor

ehuss commented Jun 12, 2019

We discussed this in the team meeting, and I think everyone feels like keeping cargo install consistent with itself was the best approach. We can always change our minds in the future, and for now using install --locked is the preferred approach. Some basic documentation was included in #7026. If more documentation is needed, PR's are always welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

2 participants