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

Tracking issue for -Z features=itarget #7914

Closed
2 tasks
ehuss opened this issue Feb 21, 2020 · 30 comments · Fixed by #8997
Closed
2 tasks

Tracking issue for -Z features=itarget #7914

ehuss opened this issue Feb 21, 2020 · 30 comments · Fixed by #8997
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-tracking-issue Category: A tracking issue for something unstable.

Comments

@ehuss
Copy link
Contributor

ehuss commented Feb 21, 2020

Implementation: #7820
Documentation: https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#features

Summary
-Z features=itarget causes the feature resolver to ignore features for target-specific dependencies for targets that don't match the current compile target. For example:

[dependency.common]
version = "1.0"
features = ["f1"]

[target.'cfg(windows)'.dependencies.common]
version = "1.0"
features = ["f2"]

When building this example for a non-Windows platform, the f2 feature will not be enabled.

Unresolved issues

  • Update cargo metadata
  • Does this cause existing projects to fail?
    • Projects can add missing features as a backwards-compatible workaround. However, if it causes problems for too many projects, we may need to find some way to opt-in.
@ehuss ehuss added C-tracking-issue Category: A tracking issue for something unstable. A-features2 Area: issues specifically related to the v2 feature resolver labels Feb 21, 2020
@est31
Copy link
Member

est31 commented Feb 24, 2020

Does this affect lockfiles too? As in: if this feature is enabled, will a lockfile on Windows look different from a lockfile on Linux? Or is it only affecting behaviour of the actual build?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 24, 2020

It does not affect Cargo.lock.

@johanlindfors
Copy link

This seems to work, at least for my scenario:

[target.'cfg(macos)'.dependencies]
sdl2 = { version = "0.32", features = ["bundled"]}

[target.'cfg(windows)'.dependencies]
sdl2 = { version = "0.32", features = ["bundled", "static-link"]}

Building with: cargo +nightly run -Z features=itarget

But I'm still a bit confused about the proposed solution. This clearly feels like a bug in the current feature resolver, but this fix proposes that we specify a different resolver? Or is this just a temporary solution while this fix gets merged into the "original" resolver?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 25, 2020

@johanlindfors it seems to work for me. What platform are you targeting? What did you see that made you think it isn't working? One thing, cfg(macos) is not a valid expression. It should be cfg(target_os = "macos").

Yes, there is a new resolver, it is not a "bug" per se in the current resolver. The dependency resolver still needs to resolve all platforms for a stable dependency graph. These features also need to be resolved for multiple targets independently (host and target). It may be possible to do it with the current resolver, but I think it would require a multi-pass solution, so I don't think there would be any benefit, and a lot of downsides.

@jdm
Copy link

jdm commented Feb 25, 2020

cfg([some target_os value]) is actually a valid cfg expression in current Rust code as far as I can tell. At least cfg(unix) and cfg(windows) seem to work.

@ehuss
Copy link
Contributor Author

ehuss commented Feb 25, 2020

unix and windows are special cases. You can read more about them here. Those are target families, whereas macos is a target os within the unix family.

@jplatte
Copy link
Contributor

jplatte commented Feb 25, 2020

@johanlindfors Yes, -Z features=itarget is a temporary solution for trying out the new solver while it is still unstable. It will replace the current solver as the default if things go to plan.

@johanlindfors
Copy link

johanlindfors commented Feb 25, 2020

@ehuss You are right, I was using the cfg(target_os = "macos") syntax but when copying the code into the comment on github I manually "shortened" it, thinking I was being smart, turns out, as usual, that was not the case!

:)

@ehuss
Copy link
Contributor Author

ehuss commented Mar 3, 2020

@benmarten
Copy link

I am not able to get this to work:

[target.'cfg(macos)'.dependencies.opencv]
version = "0.32.0"
default-features = false
features = ['opencv-4']

[target.'cfg(linux)'.dependencies.opencv]
version = "0.32.0"
default-features = false
features = ['opencv-32']

cargo build -Z features=itarget

Anything wrong with the syntax?

@ehuss
Copy link
Contributor Author

ehuss commented Mar 14, 2020

@benmarten See above. cfg(macos) is not a valid cfg expression.

@benmarten
Copy link

benmarten commented Mar 14, 2020

@ehuss
Thanks, that works now on my mac and inside docker/linux.

[target.'cfg(target_os = "macos")'.dependencies.opencv]
version = "0.32.0"
default-features = false
features = ['opencv-4']

[target.'cfg(target_os = "linux")'.dependencies.opencv]
version = "0.32.0"
default-features = false
features = ['opencv-32']
rustup override set nightly
cargo +nightly build -Z features=itarget

@gz
Copy link

gz commented Mar 27, 2020

I'm getting a panic from cargo when enabling the itarget feature and trying to build:

ProcessExecutionError: Command line: ['/home/gz/.cargo/bin/xargo', 'build', '--target', 'x86_64-custom', '--color', 'always', '-Zfeatures=itarget']
Exit code: 101
Stderr:  | warning: profiles for the non root package will be ignored, specify profiles at the workspace root:
         | package:   /home/gz/workspace/besp/lib/node-replication/Cargo.toml
         | workspace: /home/gz/workspace/besp/Cargo.toml
         | thread 'main' panicked at 'features did not find PackageId { name: "libc", version: "0.2.66", source: "registry `https://github.com/rust-lang/crates.io-index`" } false', src/tools/cargo/src/cargo/core/resolver/features.rs:220:17
         | stack backtrace:
         |    0: backtrace::backtrace::libunwind::trace
         |              at /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/libunwind.rs:86
         |    1: backtrace::backtrace::trace_unsynchronized
         |              at /cargo/registry/src/git.luolix.top-1ecc6299db9ec823/backtrace-0.3.44/src/backtrace/mod.rs:66
         |    2: std::sys_common::backtrace::_print_fmt
         |              at src/libstd/sys_common/backtrace.rs:78
         |    3: <std::sys_common::backtrace::_print::DisplayBacktrace as core::fmt::Display>::fmt
         |              at src/libstd/sys_common/backtrace.rs:59
         |    4: core::fmt::write
         |              at src/libcore/fmt/mod.rs:1069
         |    5: std::io::Write::write_fmt
         |              at src/libstd/io/mod.rs:1427
         |    6: std::sys_common::backtrace::_print
         |              at src/libstd/sys_common/backtrace.rs:62
         |    7: std::sys_common::backtrace::print
         |              at src/libstd/sys_common/backtrace.rs:49
         |    8: std::panicking::default_hook::{{closure}}
         |              at src/libstd/panicking.rs:198
         |    9: std::panicking::default_hook
         |              at src/libstd/panicking.rs:218
         |   10: std::panicking::rust_panic_with_hook
         |              at src/libstd/panicking.rs:511
         |   11: rust_begin_unwind
         |              at src/libstd/panicking.rs:419
         |   12: std::panicking::begin_panic_fmt
         |              at src/libstd/panicking.rs:373
         |   13: cargo::core::resolver::features::ResolvedFeatures::activated_features_int
         |   14: cargo::core::compiler::unit_dependencies::new_unit_dep_with_profile
         |   15: cargo::core::compiler::unit_dependencies::compute_deps
         |   16: cargo::core::compiler::unit_dependencies::deps_of
         |   17: cargo::core::compiler::unit_dependencies::deps_of
         |   18: cargo::core::compiler::unit_dependencies::deps_of
         |   19: cargo::core::compiler::unit_dependencies::deps_of
         |   20: cargo::core::compiler::unit_dependencies::deps_of
         |   21: cargo::core::compiler::unit_dependencies::deps_of
         |   22: cargo::core::compiler::unit_dependencies::deps_of_roots
         |   23: cargo::core::compiler::unit_dependencies::build_unit_dependencies
         |   24: cargo::ops::cargo_compile::compile_ws
         |   25: cargo::ops::cargo_compile::compile
         |   26: cargo::commands::build::exec
         |   27: cargo::cli::main
         |   28: cargo::main
         |   29: std::rt::lang_start::{{closure}}
         |   30: std::rt::lang_start_internal::{{closure}}
         |              at src/libstd/rt.rs:52
         |   31: std::panicking::try::do_call
         |              at src/libstd/panicking.rs:331
         |   32: std::panicking::try
         |              at src/libstd/panicking.rs:274
         |   33: std::panic::catch_unwind
         |              at src/libstd/panic.rs:394
         |   34: std::rt::lang_start_internal
         |              at src/libstd/rt.rs:51
         |   35: main
         |   36: __libc_start_main
         |   37: <unknown>
         | note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

There are no target specific dependencies in the particular lib that is being compiled (node-replication) but there are a few libc dependencies in transitive crates that are declared i.e., like this:

[target.'cfg(unix)'.dependencies]
libc = "0.2"

@ehuss
Copy link
Contributor Author

ehuss commented Mar 27, 2020

@gz Which version are you using? Is this a project I can look at? If not, can you create a reproduction? Or maybe show the dependency tree and the relevant dependency declarations.

Also, it looks like you are using a custom JSON target spec, can you include that, too?

@gz
Copy link

gz commented Mar 27, 2020

@ehuss I put a minimal example here https://github.com/gz/itarget-bug that triggers the panic for me. It seems the culprit is when I try to include the hashbrown dependency.

@ehuss
Copy link
Contributor Author

ehuss commented Mar 27, 2020

@gz TYVM for the repro! I posted a fix at #8048. As a workaround, you can pass -Zfeatures=itarget,host_dep or -Zfeatures=all (requires recent nightly such as nightly-2020-03-27).

bors added a commit that referenced this issue Mar 27, 2020
Fix -Zfeatures=itarget with certain host dependencies

In some cases, `-Zfeatures=itarget` can panic because an entry is missing in the activation map. This happens because the way "for_host" was tracked when building the activation map. Previously "for_host" was only set when `-Zfeatures=host_dep` was specified. However, if you don't specify `host_dep`, then "for_host" was always false.

This is a problem because `itarget` needs to also be able to detect if something is enabled for the host or target. If you have a proc-macro ("for_host"), and it has a dependency with a `[target]` specification that matched the host, then the activation map would fail to include it (because the "for_host" flag was not "sticky" and didn't get passed down).

The solution is to always carry the "for_host" setting around while building the activation map. Only when inserting a feature into the map will it check if `opts.decouple_host_deps` is set. This allows it to handle the "for_host" setting correctly, even if the `host_dep` option isn't enabled.

This was discovered at #7914 (comment) where a dependency on `hashbrown` would fail if you pass `--target something_not_unix` because it has a unix-only dependency for `libc`.

(Sorry, long-winded explanation. Please ask if that is confusing.)
@gz
Copy link

gz commented Mar 27, 2020

Thanks for the fix (and the feature, it simplifies my cargo dependencies quite a lot). I can compile my project now with -Zfeatures=all.

sunshowers added a commit to sunshowers/cargo-guppy that referenced this issue Mar 28, 2020
This is a complete overhaul of the way platform-specific dependencies are
handled. Based on my experimentation and reading the Cargo source code, I
believe that this is now correct.

Doing so also required feature graph construction to be updated, so the
feature graph construction is ready for the new feature resolver, and is now
platform-sensitive as well. The APIs for the feature graph are still to
come, but making the data model be full-fidelity allows for both the current
and new feature resolvers to be implemented.

For more about the new feature resolver, see:

* https://doc.rust-lang.org/nightly/cargo/reference/unstable.html#features
* rust-lang/cargo#7914
* rust-lang/cargo#7915
* rust-lang/cargo#7916
@CleanCut
Copy link

I'd love to get this on stable. Is this on the path to stabilization?

@zicklag
Copy link

zicklag commented Nov 23, 2020

Definitely agree this would be great to have, but in case this helps anybody, here's a description of how I got around not having this for gfx:

https://community.amethyst.rs/t/we-need-to-figure-out-a-way-to-adapt-the-features-of-the-amethyst-dependency-to-the-platform-the-game-is-being-compiled-on/1596/6?u=zicklag

GrygrFlzr added a commit to GrygrFlzr/glsl-to-spirv that referenced this issue Jan 5, 2021
Properly skips optional dependencies pending rust-lang/cargo#7914
More extensible for addition of new platforms
@bors bors closed this as completed in 4aa5223 Jan 5, 2021
bors bot pushed a commit to amethyst/amethyst that referenced this issue Jan 7, 2021
2525: Autoselect gfx (only works on nightly) r=ezpuzz a=CleanCut

A better than #2522 attempt to autoselect the graphics backend BUT only works by using nightly with the command `cargo +nightly build -Zfeatures=itarget` until rust-lang/cargo#7914 becomes stable.

Mostly just making this PR so it's easy to find the attempt later.

~I intend to make yet another attempt via intermediate crates that might work on stable.~ Nope, this really seems like the better approach, once rust-lang/cargo#7914 is stabilized, so I'll just wait.





Co-authored-by: Nathan Stocks <cleancut@github.com>
Co-authored-by: Emory Petermann <emory@radium.io>
@thomwiggers
Copy link
Contributor

It does not seem possible to do

[target.'cfg(not(windows))'.features]
default = ["openssl"]

Is this correct?

@simon-an
Copy link

Took me quite some time to find out, that I need to set resolver = "2" in my root Cargo.toml to activate this.
https://doc.rust-lang.org/cargo/reference/features.html

@1sra3l
Copy link

1sra3l commented Nov 14, 2021

Is this the right place to ask about "feature" specific dependencies?

[target.'cfg(features = "fltk")'.dependencies]
optional = true
# specific fltk git repos 

I want a library to expose GUI elements if those features are requested by the end user

@Razican
Copy link

Razican commented Nov 14, 2021

Is this the right place to ask about "feature" specific dependencies?

[target.'cfg(features = "fltk")'.dependencies]
optional = true
# specific fltk git repos 

I want a library to expose GUI elements if those features are requested by the end user

You can already do this by adding the dependency in the feature definition, right?

[features]
my_feature = ["my_dependency"]

[dependencies]
my_dependency = { version = "1.0", optional = true }

@1sra3l
Copy link

1sra3l commented Nov 14, 2021

Sorry I was not clear enough, I mean to ONLY included it for the features derived. Similar to compiling with OS specific crates, but only if features are requested. If this is already possible I missed it.
FWIW I thought doing something like

[dependencies]
# things the library needs
[dependencies.my_feature]
# stuff to include ONLY when they want the GUI aspect

There seems like something like this should be possible to me, but it may not be.

@1sra3l
Copy link

1sra3l commented Nov 14, 2021

@Razican I think I misunderstood what I was doing, I think I untangled what I was trying to do... sorry for the clutter 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-tracking-issue Category: A tracking issue for something unstable.
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.