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

Binary dependency is never built if specified with both optional and target keys #10526

Closed
Tracked by #10061
bstrie opened this issue Mar 31, 2022 · 3 comments · Fixed by #11183, #11331 or #11434
Closed
Tracked by #10061

Binary dependency is never built if specified with both optional and target keys #10526

bstrie opened this issue Mar 31, 2022 · 3 comments · Fixed by #11183, #11331 or #11434
Labels
A-features2 Area: issues specifically related to the v2 feature resolver C-bug Category: bug Z-bindeps Nightly: binary artifact dependencies

Comments

@bstrie
Copy link
Contributor

bstrie commented Mar 31, 2022

Problem

For convenience I have also uploaded the failing code here: https://github.com/bstrie/bindeperror2

For the following code:

# Cargo.toml
[package]
name = "mycrate"
version = "0.0.0"
edition = "2021"

[dependencies]
mybindep = { path = "mybindep", artifact = "bin", optional = true, target = "x86_64-unknown-linux-gnu" }

[features]
default = ["mybindep"]
// src/main.rs
fn main() {
    env!("CARGO_BIN_FILE_MYBINDEP");
}
# mybindep/Cargo.toml
[package]
name = "mybindep"
version = "0.0.0"
edition = "2021"
// mybindep/src/main.rs
fn main() {}

I expected this code to build. However, cargo build produces the following:

 > cargo build
   Compiling mycrate v0.0.0 (/home/ben/code/scrap/bindeperror2)
error: environment variable `CARGO_BIN_FILE_MYBINDEP` not defined
 --> src/main.rs:2:5
  |
2 |     env!("CARGO_BIN_FILE_MYBINDEP");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: this error originates in the macro `env` (in Nightly builds, run with -Z macro-backtrace for more info)

error: could not compile `mycrate` due to previous error

As we can see from the output, it is not merely that the environment variable is missing; only mycrate is ever built, and mybindep is skipped.

Note that mybindep is an optional dependency, but is specified as a default feature.

In addition, neither cargo build --features mybindep nor cargo build --all-features will result in mybindep being built.

The following causes the code to compile:

- mybindep = { path = "mybindep", artifact = "bin", optional = true, target = "x86_64-unknown-linux-gnu" 
+ mybindep = { path = "mybindep", artifact = "bin", optional = true }

Furthermore, with this (along with removing the default feature) we can observe that cargo build --features mybindep and cargo build --all-features now functions as expected. Leaving the target key and removing the optional key also causes the code to build. It is the presence of both these keys simultaneously that results in the issue.

cc @Byron

Version

cargo 1.61.0-nightly (109bfbd 2022-03-17)
release: 1.61.0-nightly
commit-hash: 109bfbd055325ef87a6e7f63d67da7e838f8300b
commit-date: 2022-03-17
host: x86_64-unknown-linux-gnu
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.80.0-DEV (sys:0.4.51+curl-7.80.0 vendored ssl:OpenSSL/1.1.1m)
os: Pop!_OS 21.10 (impish) [64-bit]
@bstrie bstrie added the C-bug Category: bug label Mar 31, 2022
@weihanglo weihanglo added the Z-bindeps Nightly: binary artifact dependencies label Apr 1, 2022
@weihanglo weihanglo added the A-features2 Area: issues specifically related to the v2 feature resolver label Oct 4, 2022
bors added a commit that referenced this issue Nov 3, 2022
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
bors added a commit that referenced this issue Nov 3, 2022
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
bors added a commit that referenced this issue Nov 3, 2022
Revert #11183

This reverts commit d4c38af, reversing changes made to 92d8826.

Fixes #11330

---

The root cause is that [`map_to_features_for`] takes care of `unit_for.host_features` from parent unit, but [here] we only want to know whether the dependency is for host or not.

We could have an ad-hoc `FeaturesFor` construction there, but I'd rather revert it at this moment. The interaction between `UnitFor` and `FeaturesFor` bites me every time 😭.

After this revert, if we want to resolve #10526 again. The fastest way is doing the following:

```diff
diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs
index 9319a19e0..14cc84941 100644
--- a/src/cargo/core/compiler/unit_dependencies.rs
+++ b/src/cargo/core/compiler/unit_dependencies.rs
`@@` -1103,7 +1103,7 `@@` impl<'a, 'cfg> State<'a, 'cfg> {
                         // If this is an optional dependency, and the new feature resolver
                         // did not enable it, don't include it.
                         if dep.is_optional() {
-                            let features_for = unit_for.map_to_features_for(dep.artifact());
+                            let features_for = FeaturesFor::from_for_host(unit_for.is_for_host());
                             if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) {
                                 return false;
                             }
```

[`map_to_features_for`]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/profiles.rs#L1043
[here]: https://github.com/rust-lang/cargo/blob/810cbad9a123ad4ee0a55a96171b8f8478ff1c03/src/cargo/core/compiler/unit_dependencies.rs#L1102
@weihanglo weihanglo reopened this Nov 4, 2022
@bstrie
Copy link
Contributor Author

bstrie commented Nov 7, 2022

@weihanglo Thanks for working on this, even if #11183 didn't quite work out. :)

@weihanglo
Copy link
Member

Oops. Really? Both #11183 (checkout this commit 810cbad) or the patch in the PR description of #11331 work for me with your example. Maybe my assumption was wrong. Could you try it again?

Anyhow, I'll continue investigating a solution like patch from #11331, but need more refactors.

@bstrie
Copy link
Contributor Author

bstrie commented Nov 7, 2022

Er, sorry, I meant to refer to #11131 as the patch to address this that didn't work out and was reverted.

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-bug Category: bug Z-bindeps Nightly: binary artifact dependencies
Projects
None yet
2 participants