From 437bed069a8369460fba205b6f36576902374559 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 28 Nov 2022 13:56:27 +0000 Subject: [PATCH 1/5] test(bindeps): target field specified and `optional = true` coexist --- tests/testsuite/artifact_dep.rs | 52 +++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 0665df649fd..db1a15beefd 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2342,3 +2342,55 @@ fn calc_bin_artifact_fingerprint() { ) .run(); } + +#[cargo_test] +fn with_target_and_optional() { + // This is a incorrect behaviour got to be fixed. + // See rust-lang/cargo#10526 + if cross_compile::disabled() { + return; + } + let target = cross_compile::alternate(); + let p = project() + .file( + "Cargo.toml", + &r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2021" + [dependencies] + d1 = { path = "d1", artifact = "bin", optional = true, target = "$TARGET" } + "# + .replace("$TARGET", target), + ) + .file( + "src/main.rs", + r#" + fn main() { + let _b = include_bytes!(env!("CARGO_BIN_FILE_D1")); + } + "#, + ) + .file( + "d1/Cargo.toml", + r#" + [package] + name = "d1" + version = "0.0.1" + edition = "2021" + "#, + ) + .file("d1/src/main.rs", "fn main() {}") + .build(); + + p.cargo("check -Z bindeps -F d1 -v") + .masquerade_as_nightly_cargo(&["bindeps"]) + .with_stderr_contains( + "\ +[ERROR] environment variable `CARGO_BIN_FILE_D1` not defined +", + ) + .with_status(101) + .run(); +} From 7dcb6daa7d2145e6da9513bb62f676445de049e5 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 28 Nov 2022 15:32:31 +0000 Subject: [PATCH 2/5] test(bindeps): assumed host target and `optional = true` coexist --- tests/testsuite/artifact_dep.rs | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index db1a15beefd..58e44cb6422 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2394,3 +2394,47 @@ fn with_target_and_optional() { .with_status(101) .run(); } + +#[cargo_test] +fn with_assumed_host_target_and_optional_build_dep() { + // This exercises an incorrect behaviour got to be fixed by the following commit. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.1" + edition = "2021" + [build-dependencies] + d1 = { path = "d1", artifact = "bin", optional = true, target = "target" } + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "build.rs", + r#" + fn main() { + std::env::var("CARGO_BIN_FILE_D1").unwrap(); + } + "#, + ) + .file( + "d1/Cargo.toml", + r#" + [package] + name = "d1" + version = "0.0.1" + edition = "2021" + "#, + ) + .file("d1/src/main.rs", "fn main() {}") + .build(); + + p.cargo("check -Z bindeps -F d1 -v") + .masquerade_as_nightly_cargo(&["bindeps"]) + .with_status(101) + .with_stderr_contains("[ERROR] failed to run custom build command for `foo v0.0.1 ([..])`") + .with_stderr_contains("[..]thread '[..]' panicked at 'called `Result::unwrap()`[..]") + .run(); +} From bf56587e2617cac5f8045447911e67c4e84df60a Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 28 Nov 2022 13:59:57 +0000 Subject: [PATCH 3/5] fix(bindeps): target field specified and `optional = true` coexist > Adapted from #11183 Previously, `is_dep_activated` depends on `activated_dependencies`, which is a map of `PackageFeaturesKey` to its own activated `DepFeature`s. `PackageFeaturesKey` in feature resolver is always comprised of * A `PackageId` of a given dependency, and * A enum value that helps decoupling activated features for that dependency. Currently depending on the type of given dependency. Looking into issue 10526, it has an `activated_dependencies` of ``` { (mycrate, NormalOrDev) -> [dep:mybindep] } ``` However, this [line][1] accidentally use a parent's `pkgid` and a dependency's `FeaturesFor` to compose a key: ``` (mycrate, ArtifactDep("x86_64-unknown-linux-gnu")) ``` That is never a valid key to query features for artifacts dependency. A valid key should be comprised of components both from the parent: ``` (mycrate, NormalOrDev) ``` Or both from the dependency itself: ``` (mybindep, ArtifactDep("x86_64-unknown-linux-gnu")) ``` This `unit_for` is from parent and should already contain its own artifact dep information inside `artifact_target_for_features`, so we don't need to map any dependency artifact from `dep.artifact()`. [1]: https://github.com/rust-lang/cargo/blob/a2ea66bea6fe8156444144e98911d073d56c2c0c/src/cargo/core/compiler/unit_dependencies.rs#L1106-L1107 --- src/cargo/core/compiler/unit_dependencies.rs | 2 +- tests/testsuite/artifact_dep.rs | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index 69ac801ff51..d95b4cc3ef2 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 = unit_for.map_to_features_for(None); if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) { return false; } diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 58e44cb6422..1bbdd39b14e 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2345,7 +2345,6 @@ fn calc_bin_artifact_fingerprint() { #[cargo_test] fn with_target_and_optional() { - // This is a incorrect behaviour got to be fixed. // See rust-lang/cargo#10526 if cross_compile::disabled() { return; @@ -2386,12 +2385,15 @@ fn with_target_and_optional() { p.cargo("check -Z bindeps -F d1 -v") .masquerade_as_nightly_cargo(&["bindeps"]) - .with_stderr_contains( + .with_stderr( "\ -[ERROR] environment variable `CARGO_BIN_FILE_D1` not defined +[COMPILING] d1 v0.0.1 [..] +[RUNNING] `rustc --crate-name d1 [..]--crate-type bin[..] +[CHECKING] foo v0.0.1 [..] +[RUNNING] `rustc --crate-name foo [..]--cfg[..]d1[..] +[FINISHED] dev [..] ", ) - .with_status(101) .run(); } From 5b8985a470368f10cbc616a3611c894e2a914fe7 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Mon, 28 Nov 2022 15:49:06 +0000 Subject: [PATCH 4/5] fix(bindeps): assumed host target and `optional = true` coexist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `{ …, artifact = "bin", target = "target" }` should also be considered to being built for `FeaturesFor::ArtifactDep` instead of `NormalOrDev`. [This line][1] requests for `ArtifactDep` but [here][2] Cargo ignore assumed host target of artifact dep. Change it to explicit set host target triple when - `{ …, target = "target" }`, and - `--target` is not presented, meaning it would be build on host platform. [1]: https://github.com/rust-lang/cargo/blob/1382b44e431f58199afdffc2b757962b0e163602/src/cargo/core/compiler/unit_dependencies.rs#L887 [2]: https://github.com/rust-lang/cargo/blob/1382b44e431f58199afdffc2b757962b0e163602/src/cargo/core/resolver/features.rs#L857 --- src/cargo/core/resolver/features.rs | 10 ++++++---- tests/testsuite/artifact_dep.rs | 15 +++++++++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/cargo/core/resolver/features.rs b/src/cargo/core/resolver/features.rs index a2dba2869cf..e2f2bd5c50c 100644 --- a/src/cargo/core/resolver/features.rs +++ b/src/cargo/core/resolver/features.rs @@ -853,12 +853,14 @@ impl<'a, 'cfg> FeatureResolver<'a, 'cfg> { ArtifactTarget::BuildDependencyAssumeTarget => self .requested_targets .iter() - .filter_map(|kind| match kind { - CompileKind::Host => None, - CompileKind::Target(target) => { - Some(FeaturesFor::ArtifactDep(*target)) + .map(|kind| match kind { + CompileKind::Host => { + let host_triple = self.target_data.rustc.host; + CompileTarget::new(&host_triple).unwrap() } + CompileKind::Target(target) => *target, }) + .map(FeaturesFor::ArtifactDep) .collect(), }), ) diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 1bbdd39b14e..0a39af05b23 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -2399,7 +2399,6 @@ fn with_target_and_optional() { #[cargo_test] fn with_assumed_host_target_and_optional_build_dep() { - // This exercises an incorrect behaviour got to be fixed by the following commit. let p = project() .file( "Cargo.toml", @@ -2435,8 +2434,16 @@ fn with_assumed_host_target_and_optional_build_dep() { p.cargo("check -Z bindeps -F d1 -v") .masquerade_as_nightly_cargo(&["bindeps"]) - .with_status(101) - .with_stderr_contains("[ERROR] failed to run custom build command for `foo v0.0.1 ([..])`") - .with_stderr_contains("[..]thread '[..]' panicked at 'called `Result::unwrap()`[..]") + .with_stderr_unordered( + "\ +[COMPILING] foo v0.0.1 ([CWD]) +[COMPILING] d1 v0.0.1 ([CWD]/d1) +[RUNNING] `rustc --crate-name build_script_build [..]--crate-type bin[..] +[RUNNING] `rustc --crate-name d1 [..]--crate-type bin[..] +[RUNNING] `[CWD]/target/debug/build/foo-[..]/build-script-build` +[RUNNING] `rustc --crate-name foo [..]--cfg[..]d1[..] +[FINISHED] dev [..] +", + ) .run(); } From eca3e23d6dcb389c457204e97bfb872c75c028e6 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Tue, 13 Dec 2022 20:25:02 +0000 Subject: [PATCH 5/5] Explain why we don't need to pass `dep.artifact()` --- src/cargo/core/compiler/unit_dependencies.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/unit_dependencies.rs b/src/cargo/core/compiler/unit_dependencies.rs index d95b4cc3ef2..28a47b0dee8 100644 --- a/src/cargo/core/compiler/unit_dependencies.rs +++ b/src/cargo/core/compiler/unit_dependencies.rs @@ -1103,7 +1103,10 @@ 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(None); + // This `unit_for` is from parent dep and *SHOULD* contains its own + // artifact dep infomration inside `artifact_target_for_features`. + // So, no need to map any artifact info from an incorrect `dep.artifact()`. + let features_for = unit_for.map_to_features_for(IS_NO_ARTIFACT_DEP); if !self.is_dep_activated(pkg_id, features_for, dep.name_in_toml()) { return false; }