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

Incorrect dev-dependency feature resolution #1796

Open
farcaller opened this issue Jul 10, 2015 · 21 comments
Open

Incorrect dev-dependency feature resolution #1796

farcaller opened this issue Jul 10, 2015 · 21 comments
Assignees
Labels

Comments

@farcaller
Copy link

@farcaller farcaller commented Jul 10, 2015

If same crate is used in both dependencies and dev-dependencies with the only change of features, that features leak from dev-dependencies to dependencies.

Here's the smallest test case I managed to generate.

Root crate Cargo.toml:

[package]
name = "test"
version = "0.1.0"
authors = ["Vladimir Pouzanov <farcaller@gmail.com>"]

[lib]
name = "test"
crate-type = ["lib"]

[target.thumbv6-none-eabi.dependencies.core]
git = "https://github.com/hackndev/rust-libcore"

[dev-dependencies]
expectest = "*"

[dev-dependencies.dep]
path = "./dep"
features = ["replayer"]

[dependencies.dep]
path = "./dep"

this crate uses expectest for testing only and a dep crate for either testing or runtime build. It is expected to be a cross-compiled staticlib, so it does not depend on core (neither is dep).

Dep is defined like this:

[package]

name = "dep"
version = "1.0.0"
authors = ["Vladimir Pouzanov <farcaller@gmail.com>"]

[lib]
name = "dep"
path = "lib.rs"
crate-type = ["rlib"]

[features]
replayer = ["expectest"]

[target.thumbv6-none-eabi.dependencies.core]
git = "https://github.com/hackndev/rust-libcore"

[dependencies.expectest]
optional = true

If the replayer feature is used, this crate is being built for test, thus requiring expectest in runtime. Otherwise this crate is std-less as well.

When cross-compiled, the expected outcome is to have test and dep crates being cross-built, where dep is being built without expectest. This is not the case:

% cargo build --target=thumbv6-none-eabi --verbose -j1 --lib
   Compiling rustc-serialize v0.3.15
     Running `rustc /Users/farcaller/.cargo/registry/src/github.com-0a35038f75765ae4/rustc-serialize-0.3.15/src/lib.rs --crate-name rustc_serialize --crate-type lib -g -C metadata=c1e8163a38ed3d54 -C extra-filename=-c1e8163a38ed3d54 --out-dir /Users/farcaller/temp/bug/test/target/thumbv6-none-eabi/debug/deps --emit=dep-info,link --target thumbv6-none-eabi -L dependency=/Users/farcaller/temp/bug/test/target/thumbv6-none-eabi/debug/deps -L dependency=/Users/farcaller/temp/bug/test/target/thumbv6-none-eabi/debug/deps -Awarnings`
/Users/farcaller/.cargo/registry/src/github.com-0a35038f75765ae4/rustc-serialize-0.3.15/src/lib.rs:1:1: 1:1 error: can't find crate for `std`
/Users/farcaller/.cargo/registry/src/github.com-0a35038f75765ae4/rustc-serialize-0.3.15/src/lib.rs:1 // Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT

rustc_serialize is being pulled in as a test dependency through test -> dep -> expectest chain and fails to compile in cross environment.

@farcaller

This comment has been minimized.

Copy link
Author

@farcaller farcaller commented Jul 10, 2015

I traced it down to https://github.com/rust-lang/cargo/blob/master/src/cargo/ops/cargo_compile.rs#L152 which seem to be the problem.

This simple patch works for me (both cargo build and cargo test work as expected):

diff --git a/src/cargo/ops/cargo_compile.rs b/src/cargo/ops/cargo_compile.rs
index a927208..5d9ff3e 100644
--- a/src/cargo/ops/cargo_compile.rs
+++ b/src/cargo/ops/cargo_compile.rs
@@ -149,7 +149,8 @@ pub fn compile_pkg<'a>(package: &Package,
         let platform = target.as_ref().map(|e| &e[..]).or(Some(&rustc_host[..]));

         let method = Method::Required{
-            dev_deps: true, // TODO: remove this option?
+            dev_deps: options.mode == CompileMode::Test ||
+                options.mode == CompileMode::Bench,
             features: &features,
             uses_default_features: !no_default_features,
             target_platform: platform};
@farcaller

This comment has been minimized.

Copy link
Author

@farcaller farcaller commented Jul 11, 2015

After tinkering around the source it seems that it's more of an architectural problem.

ops::resolve_with_previous resolves dependencies for a "Package", so all targets of that package will have same set of dependencies, both library and examples.

This makes me come to a conclusion that the only difference between dependencies and dev-dependencies is propagation factor to other crates.

To sum up why am I here at this point: cargo build will pull in dev-dependencies even if it builds only the lib crate. It makes it very non-trivial to provide nice unit-tests for a cross-compiled no-std crate.

So far all my attempts to unbreak the examples (building them with dev-deps) failed and it looks like the change is beyond my understanding of cargo source so I'm closing the PR and bringing the discussion back here in hopes that someone else will give me a hint on how to fix this or provide a better PR.

@farcaller

This comment has been minimized.

Copy link
Author

@farcaller farcaller commented Jul 11, 2015

For the reference, here's a unit test exposing the problem:

test!(dev_dependencies_dont_leak_features {
    let p = project("foo")
        .file("Cargo.toml", r#"
            [package]
            name = "foo"
            version = "0.1.0"
            authors = []

            [dependencies.a]
            path = "a"

            [dev-dependencies.a]
            path = "a"
            features = ["badfeature"]
        "#)
        .file("src/main.rs", r#"
            extern crate a;
            pub fn main() { a::a(); }
        "#)
        .file("a/Cargo.toml", r#"
            [package]
            name = "a"
            version = "0.1.0"
            authors = []

            [features]
            badfeature = []
        "#)
        .file("a/src/lib.rs", r#"
            #[cfg(feature = "badfeature")]
            pub fn a() { panic!(); }

            #[cfg(not(feature = "badfeature"))]
            pub fn a() { }
        "#);

    assert_that(p.cargo_process("run"), execs().with_status(0));
});
@alexcrichton

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton commented Jul 13, 2015

Hm, upon thinking on this again I don't think there's actually any alternative here. Architecturally Cargo will union all features when compiling a crate, so even if we kept cargo build and cargo test apart in terms of activating the features of dev-dependencies, you'd have a situation like this:

  • For cargo build the feature would not be activated and the library would be compiled as usual.
  • For cargo test the upstream dependency would be recompiled with the new feature, and the library would be compiled against this new copy of the library for integration tests and such.

This means that even if we were to draw the distinction between cargo build and cargo test you'd still have to write a library ready to link against both versions of the upstream dependency.

I'll also clarify that this is a very specific problem, it only shows up when:

  • The exact same dependency is used as both a dev and non-dev dependency.
  • The dev-dependency version activates a feature

All in all I'm starting to think that this is working as intended rather than a bug.

@farcaller

This comment has been minimized.

Copy link
Author

@farcaller farcaller commented Jul 13, 2015

Here's my practical concern.

zinc pulls in volatile_cell in either runtime or test. It also pulls in expectest for testing.

volatile_cell supports mocking if built with replayer feature. If it's enabled, expectest is pulled in too.

I can't change expectest to dev-dependency for volatile_cell as it doesn't propagate when I build zinc crate for tests and I can't use it as non-dev dependency now as building zinc for release pulls in expectest and its dependencies all way down to libstd.

Is it a wrong use of cargo or unsupported usecase? It would be sad for us to drop cargo support again, but we really need to have both cross-builds and unit tests done in a reasonable manner (i.e. without ./configure generating the Cargo.toml).

@alexcrichton

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton commented Jul 13, 2015

Could you put the activation of replayer in volatile_cell behind a feature of the top-level crate itself? That way cargo test would run unit tests and cargo test --feature replayer would just run "more" unit tests.

yberreby added a commit to yberreby/nickel-postgres that referenced this issue Aug 15, 2015
@apoelstra

This comment has been minimized.

Copy link

@apoelstra apoelstra commented Oct 31, 2015

I ran into this bug this week. I've got a feature qa on a library which adds some extra required fields to my program's configuration file. These extra fields are used by the integration tests as configuration for the test harness; they have no purpose for ordinary users of the library.

@alexcrichton

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton commented Nov 2, 2015

@apoelstra the bug here should just be that when building the crate itself the feature leaks into the dependency, but when you crate is itself used as a dependency it shouldn't activate the features activated as part of the dev-dependencies. Does this differ from what you're seeing though?

@apoelstra

This comment has been minimized.

Copy link

@apoelstra apoelstra commented Nov 3, 2015

@alexcrichton The crate that depends on my library is a binary; it's never used as a dependency. It's when I'm building the binary that I'm having problems.

@alexcrichton

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton commented Nov 4, 2015

Ah yes that'd do it. Can you elaborate on the problems you're seeing though? I understand that the dependence is built with the extra features during a normal cargo build, but I'm curious how this manifests itself as a problem down the road

@apoelstra

This comment has been minimized.

Copy link

@apoelstra apoelstra commented Nov 4, 2015

@alexcrichton Sure. My application is an always-on network node which communicates with its peers as well as another application which feeds it information about the outside world. It is controlled by a .toml configuration file which describes the network address of its peers, authentication keys, etc.

Most of the application logic lives in a library separate from the main binary. This includes the logic for parsing the configuration file. (This sounds iffy but we have a few projects which use almost the same configuration file, and I just use cargo features to handle the small parts that differ.)

The library has a feature qa which adds several required fields to the program configuration file. These fields tell it when to terminate (as opposed to running forever) and also give it instructions for obtaining mock external data. I want these fields to be required for integration tests and not present in real life. So under dev-dependencies I have my library included with the qa feature and under dependencies I have it included without.

However, when I build my binary, the qa feature is used, and the result is a program that won't run, instead demanding users add meaningless fields to their configuration file.

@alexcrichton

This comment has been minimized.

Copy link
Member

@alexcrichton alexcrichton commented Nov 4, 2015

Thanks for the info!

FWIW I think this is unlikely to be fixed in the near future. I'm not 100% convinced there's a great way to fix this, so you may want to add a separate "foo-tests" project which pulls in the dev-dep with the feature enabled if that's possible.

@jethrogb

This comment has been minimized.

Copy link
Contributor

@jethrogb jethrogb commented Apr 21, 2016

  • For cargo build the feature would not be activated and the library would be compiled as usual.
  • For cargo test the upstream dependency would be recompiled with the new feature, and the library would be compiled against this new copy of the library for integration tests and such.

This means that even if we were to draw the distinction between cargo build and cargo test you'd still have to write a library ready to link against both versions of the upstream dependency.

Right, this is exactly what I want. My library would ideally set alloc_buddy_simple = { version = "0.1", features = ["use-as-rust-allocator"] }. The problem is that the tests can't run with this allocator because it won't be initialized when running tests. Therefore, to be able to run any tests at all, that feature needs to be disabled. I currently to this instead:

[dependencies]
alloc_buddy_simple = { version = "0.1" }
[features]
default = ["alloc_buddy_simple/use-as-rust-allocator"]

And remember to always run cargo test --no-default-features, but this is quite cumbersome.

I'll also clarify that this is a very specific problem, it only shows up when:

  • The exact same dependency is used as both a dev and non-dev dependency.
  • The dev-dependency version activates a feature

Actually, it also shows up when:

  • The exact same dependency is used as both a dev and non-dev dependency.
  • The dependency does not specify any features.
  • The dev-dependency version says default-features=false.
@newpavlov

This comment has been minimized.

Copy link

@newpavlov newpavlov commented Sep 5, 2016

I think the following problem is related to this issue too, but should be simpler to fix.

Let us have 3 crates with following Cargo.toml files:
some_tests

name = "some_tests"
version = "0.0.0"

[features]
default = [use-std]
use-std = []

foo

name = "foo"
version = "0.0.0"

[dev-dependencies]
some_tests = {path = "../some_tests", default-features = false}

[features]
default = [use-std]
use-std = ["some_tests/use-std"]

bar

name = "bar"
version = "0.0.0"

[dependencies]
foo = {path = "../foo", default-features = false}

[features]
default = [use-std]
use-std = ["foo/use-std"]

Rationale behind such structure is to disable some parts of test suit which are not compatible with no_std. foo gets compiled as intended, but then we try to compile bar we'll get the following error:

error: Package `foo v0.0.0 (file:///path/to/foo)` does not have these features: `some_tests`

As I understand problem is that dev-dependencies do not propagate to higher level crates, so then we try to compile bar cargo can't see optional some_tests dependency for foo thus failing to set necessary feature.

As a quick fix we can simple ignore dev-dependencies listed in [features] when setting up features from higher level crates, but I don't know how this solution will look architecture wise.

@pftbest

This comment has been minimized.

Copy link

@pftbest pftbest commented Sep 22, 2018

I have a problem with cbindgen and serde, I cannot use them together

This is what I have in my Cargo.toml

[dependencies.serde]
version = "*"
default-features = false

[dev-dependencies]
cbindgen = "*"

And I get this error

   Compiling serde v1.0.79                                                                                              
error[E0463]: can't find crate for `std`
  |
  = note: the `thumbv7em-none-eabihf` target may not be installed

error: aborting due to previous error

For more information about this error, try `rustc --explain E0463`.
error: Could not compile `serde`.

Removing cbindgen fixes the build. It looks like cbindgen in [dev-depencies] somehow enables the std feature for serde in [dependencies].

@philipr-za

This comment has been minimized.

Copy link

@philipr-za philipr-za commented May 16, 2019

Just adding another voice to this issue in 2019. Not being able to set different features for a dependency vs dev-dependency makes sharing test utilities from other crates in a project extremely awkward where using the feature functionality would be a very straightforward and neat solution.

@tarcieri

This comment has been minimized.

Copy link

@tarcieri tarcieri commented May 21, 2019

This bug appears to have broken no_std builds of curve25519-dalek.

I've managed to fix it in general, but it's making it quite difficult to do things like no_std + serde (since serde/std is activated by several of dalek's dev-dependencies). See:

dalek-cryptography/curve25519-dalek#247 (comment)

@elichai

This comment has been minimized.

Copy link

@elichai elichai commented Aug 16, 2019

Anyone working to fix this?
It seems like the dependency resolution between regular deps dev-dep and build-deb is quite broken :/

@najamelan

This comment has been minimized.

Copy link

@najamelan najamelan commented Aug 18, 2019

Just discovered that features also leak between targets. You cannot turn on a feature just when compiling for wasm32 and not for other targets. The feature will leak to other targets.

@Recmo

This comment has been minimized.

Copy link

@Recmo Recmo commented Aug 19, 2019

One more example, the following will fail to build on no-std targets because criterion pulls in itertools/std:

[dependencies]
itertools = { version = "0.8.0", default_features = false }

[dev-dependencies]
criterion = "0.2.10"

My 2cts: std is not a feature in the sense that you can take a union of it, rather its opposite no_std would make sense in a union. But none of this makes sense if the union is taken across all build targets. Criterion in particular pulls in a lot of deps and features that have nothing to do with the embedded target for this lib.

@Recmo

This comment has been minimized.

Copy link

@Recmo Recmo commented Aug 20, 2019

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