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 shouldn't check for dev-dependencies when running `cargo build` or `cargo install` #4988

Closed
infinity0 opened this issue Jan 29, 2018 · 17 comments

Comments

@infinity0
Copy link
Contributor

commented Jan 29, 2018

In the docs, dev-dependencies are advertised as "not used when compiling a package for building, but are used for compiling tests, examples, and benchmarks."

However, cargo fails if dev-dependencies are not available in a local registry. For example:

$ basename $PWD
aho-corasick
$ git describe
0.6.4
$ ls -1 ../registry/
libc-0.2.36/
memchr-2.0.1/
$ # ^^^ this is all the dependencies, but none of the dev-dependencies like csv
$ cat config 
[source.crates-io]
replace-with = "dh-cargo-registry"

[source.dh-cargo-registry]
directory = "./registry"
$ CARGO_HOME=$PWD cargo build
warning: path `$PWD/src/main.rs` was erroneously implicitly accepted for binary `aho-corasick-dot`,
please set bin.path in Cargo.toml
error: no matching package named `csv` found (required by `aho-corasick`)
location searched: registry https://github.com/rust-lang/crates.io-index
version required: = 0.15.0
exit code 101

This makes it awkward to package crates for distros, since we need to set up a local registry - we would have to add dev-dependencies in here as well, even though they're not actually used by build or install.

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

Looks like the behaviour here was last changed in 9e77919, any thoughts @alexcrichton ?

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

Currently cargo_compile.rs calls into resolve_ws_precisely where dev_deps is Required to be true. That commit is where this was done, and then subsequently moved around until today.

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

Here's a commit that fixes the issue for me: master...infinity0:master

However I'm not sure if you even want the functionality, and it opens up a can of worms:

  • Method really needs to be a tuple of dev_deps and the current enum, in the commit I duplicate dev_deps into Everything, but Everything seems mostly used to mean "all features" rather than "actually everything".
  • cargo fetch probably wants a --dev-deps option
  • cargo clean, not sure what to do here
  • and various other places I just guessed what dev_deps should be true or false, needs more thought
  • also fixed a discrepancy with the output of cargo build --help where it says --all-targets Build all targets (lib and bin targets by default) but actually CompilerFilter::Default seemed to act basically the same as --all-targets.
@ignatenkobrain

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

IIRC, @alexcrichton said that it needs all dependencies because it needs to prepare full Cargo.lock.

@ignatenkobrain

This comment has been minimized.

Copy link
Contributor

commented Jan 29, 2018

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

I see, my commit does indeed omit dev-dependencies from Cargo.lock and cargo test breaks because of this. I'll see if I can fix that up, though. Perhaps hide the functionality behind a --no-dev-deps flag or something.

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

FTR @ignatenkobrain is having to patch out the dev-dependencies in all Fedora rust crates' Cargo.toml files but I think it would be cleaner if cargo could do this itself.

If the cargo team agrees I'd be happy to get the patch mentioned above into working shape.

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

I pushed a fix for cargo test et al in infinity0/cargo@f32d6a4. Now the downside is that running cargo build and cargo test repeatedly, will continually rewrite Cargo.lock with versions with/without dev-dependencies, but that could be avoided by hiding the functionality behind a cargo build --avoid-dev-deps flag. Distros (who largely don't care about Cargo.lock) could then set this, but most users would get the current functionality (and a more stable Cargo.lock).

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jan 29, 2018

This seems closely related to #3369 , which made cargo install not look at optional dependencies for non-enabled features. I think we need exactly the same fix to make cargo install (and only cargo install) ignore missing dev-dependencies. Require them for cargo build and cargo test and every other cargo command, just not cargo install. That way, you don't need to worry about the lock file.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

It makes sense to me as well that cargo install wouldn't account for dev-dependencies, but the issue originally is using cargo build? I'd agree with @joshtriplett that we'd still want to require dev-deps for cargo build

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 30, 2018

I think it would be good to do both for cargo install and cargo build, but perhaps/probably hide it behind a flag for cargo build so that the existing behaviour is unchanged for everyone else.

What I'm doing here is getting debcargo ready for official Debian use, and I see that @joshtriplett (who originally wrote it) is calling cargo install for crates with binary outputs. For library crates, we currently don't actually invoke cargo at all during the (Debian) build. However I think it would be good to call cargo build even for library crates, just to test that everything at least compiles - even though we wouldn't actually install any of the rilbs (as per the Debian rust packaging policy). If we did call cargo build, then we would have detected this issue, and any other present/future issues, earlier in Debian.

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@infinity0 We can't call cargo build for many library crates, because the .crate file doesn't necessarily contain all the files needed to do so. That was discussed extensively during the initial creation of debcargo and dh-cargo. Only cargo install is guaranteed to work from the contents of the .crate file.

However, one thought occurs to me for how to address that. If you want to test-build a library crate, you can generate a dummy crate that lists the library crate as a dependency in Cargo.toml, then cargo build the dummy crate. Note that to do so, you'll need to have all the dependencies of the library installed, which you wouldn't normally need to build the library. For that reason, I'd suggest doing so via autopkgtest instead; we could easily have debcargo generate a debian/tests/control file that defines how to build the installed library package.

(That doesn't address running the testsuite of the library crate, which I'd love to do but which doesn't work from .crate files either. But build-testing seems like an improvement.)

So, my proposal would be to fix cargo install to not require dev-dependencies, and separately, improve debcargo to add an autopkgtest.

@sfackler

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@joshtriplett When can you cargo install the contents of a .crate but not cargo build it?

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@sfackler Crates that are part of workspaces with the rest of the workspace missing. @alexcrichton can go into further detail; he was the one who originally explained in very thorough and patient detail why running any cargo command directly in the unpacked .crate would not always work, and that only putting that source in a local registry and then running cargo install was guaranteed to work.

@sfackler

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

That may have been before #4030? Cargo now rewrites the manifest of published crates to remove path dependencies.

@infinity0

This comment has been minimized.

Copy link
Contributor Author

commented Jan 31, 2018

Note that to [build the dummy crate that depends on the library], you'll need to have all the dependencies of the library installed, which you wouldn't normally need to build the library.

I am a bit confused by this statement, could you clarify? I thought it would be necessary to have dependencies and build-dependencies available in the registry in order to build the library, but in order to build that dummy crate one would only need dependencies?

If possible - as @sfackler seems to be maybe suggesting - IMO it would be preferable to do a test cargo build as part of the Debian package build and less preferable as a autopkgtest - since the latter runs on a different infrastruture system asynchronously after the package is already uploaded to Debian official archives. (We could also do both, assuming the former is possible.)

@joshtriplett

This comment has been minimized.

Copy link
Member

commented Jan 31, 2018

@infinity0 I meant "which you wouldn't normally need to build the library .deb".

I don't think it makes sense to add all the dependencies of the package to its Build-Depends just so that we can do a dummy cargo build and throw the results away. I'd rather see that as an autopkgtest, run continuously. In particular, we can also arrange to have that re-run when rustc, cargo, and other tools get upgraded.

bors added a commit that referenced this issue Mar 7, 2018

Auto merge of #5012 - infinity0:pr4988, r=alexcrichton
Don't require dev-dependencies when not needed in certain cases

Specifically, when running `cargo install` and add a flag for this behaviour in `cargo build`.

This closes #4988.

@bors bors closed this in #5012 Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.