From d288fa9d762d26c0b647aaa2a377bbe0845cad79 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 3 Nov 2021 16:48:39 +0800 Subject: [PATCH] Validate libs also don't see artifact dependencies as libraries with lib=false Also ---- - Add prelimiary test for validating build-time artifacts - Try to fix CI on gnu windows Which apparently generates paths similar to linux, but with .exe suffix. The current linux patterns should match that. - refactor Help sharing code across modules --- src/cargo/core/compiler/artifact.rs | 58 ++++++++++++ src/cargo/core/compiler/custom_build.rs | 50 +---------- src/cargo/core/compiler/mod.rs | 3 +- tests/testsuite/artifact_dep.rs | 112 +++++++++++++++++++++++- 4 files changed, 172 insertions(+), 51 deletions(-) create mode 100644 src/cargo/core/compiler/artifact.rs diff --git a/src/cargo/core/compiler/artifact.rs b/src/cargo/core/compiler/artifact.rs new file mode 100644 index 00000000000..09eac3aa707 --- /dev/null +++ b/src/cargo/core/compiler/artifact.rs @@ -0,0 +1,58 @@ +use crate::core::compiler::unit_graph::UnitDep; +use crate::core::compiler::{Context, CrateType, FileFlavor, Unit}; +use crate::core::TargetKind; +use crate::CargoResult; +use cargo_util::ProcessBuilder; + +pub fn set_env( + cx: &Context<'_, '_>, + dependencies: &[UnitDep], + cmd: &mut ProcessBuilder, +) -> CargoResult<()> { + for unit_dep in dependencies.iter().filter(|d| d.unit.artifact) { + for artifact_path in cx + .outputs(&unit_dep.unit)? + .iter() + .filter_map(|f| (f.flavor == FileFlavor::Normal).then(|| &f.path)) + { + let artifact_type_upper = unit_artifact_type_name_upper(&unit_dep.unit); + let dep_name_upper = unit_dep + .dep_name + .unwrap_or(unit_dep.unit.pkg.name()) + .to_uppercase() + .replace("-", "_"); + cmd.env( + &format!("CARGO_{}_DIR_{}", artifact_type_upper, dep_name_upper), + artifact_path.parent().expect("parent dir for artifacts"), + ); + cmd.env( + &format!( + "CARGO_{}_FILE_{}_{}", + artifact_type_upper, + dep_name_upper, + unit_dep.unit.target.name() + ), + artifact_path, + ); + if unit_dep.unit.target.name().to_uppercase() == dep_name_upper { + cmd.env( + &format!("CARGO_{}_FILE_{}", artifact_type_upper, dep_name_upper,), + artifact_path, + ); + } + } + } + Ok(()) +} + +fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str { + match unit.target.kind() { + TargetKind::Lib(kinds) => match kinds.as_slice() { + &[CrateType::Cdylib] => "CDYLIB", + &[CrateType::Staticlib] => "STATICLIB", + invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid), + }, + TargetKind::Bin => "BIN", + invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid), + } +} diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 53e6dde3037..318a230d2b7 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -1,9 +1,9 @@ use super::job::{Freshness, Job, Work}; use super::{fingerprint, Context, LinkType, Unit}; +use crate::core::compiler::artifact; use crate::core::compiler::context::Metadata; use crate::core::compiler::job_queue::JobState; -use crate::core::compiler::{CrateType, FileFlavor}; -use crate::core::{profiles::ProfileRoot, PackageId, Target, TargetKind}; +use crate::core::{profiles::ProfileRoot, PackageId, Target}; use crate::util::errors::CargoResult; use crate::util::machine_message::{self, Message}; use crate::util::{internal, profile}; @@ -205,39 +205,7 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { .inherit_jobserver(&cx.jobserver); // Find all artifact dependencies and make their file and containing directory discoverable using environment variables. - for unit_dep in dependencies.iter().filter(|d| d.unit.artifact) { - for artifact_path in cx - .outputs(&unit_dep.unit)? - .iter() - .filter_map(|f| (f.flavor == FileFlavor::Normal).then(|| &f.path)) - { - let artifact_type_upper = unit_artifact_type_name_upper(&unit_dep.unit); - let dep_name_upper = unit_dep - .dep_name - .unwrap_or(unit_dep.unit.pkg.name()) - .to_uppercase() - .replace("-", "_"); - cmd.env( - &format!("CARGO_{}_DIR_{}", artifact_type_upper, dep_name_upper), - artifact_path.parent().expect("parent dir for artifacts"), - ); - cmd.env( - &format!( - "CARGO_{}_FILE_{}_{}", - artifact_type_upper, - dep_name_upper, - unit_dep.unit.target.name() - ), - artifact_path, - ); - if unit_dep.unit.target.name().to_uppercase() == dep_name_upper { - cmd.env( - &format!("CARGO_{}_FILE_{}", artifact_type_upper, dep_name_upper,), - artifact_path, - ); - } - } - } + artifact::set_env(cx, dependencies, &mut cmd)?; if let Some(linker) = &bcx.target_data.target_config(unit.kind).linker { cmd.env( @@ -518,18 +486,6 @@ fn build_work(cx: &mut Context<'_, '_>, unit: &Unit) -> CargoResult { Ok(job) } -fn unit_artifact_type_name_upper(unit: &Unit) -> &'static str { - match unit.target.kind() { - TargetKind::Lib(kinds) => match kinds.as_slice() { - &[CrateType::Cdylib] => "CDYLIB", - &[CrateType::Staticlib] => "STATICLIB", - invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid), - }, - TargetKind::Bin => "BIN", - invalid => unreachable!("BUG: artifacts cannot be of type {:?}", invalid), - } -} - fn insert_warnings_in_build_outputs( build_script_outputs: Arc>, id: PackageId, diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index d24e414d962..31883e5bb34 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -1,3 +1,4 @@ +mod artifact; mod build_config; mod build_context; mod build_plan; @@ -273,7 +274,7 @@ fn rustc(cx: &mut Context<'_, '_>, unit: &Unit, exec: &Arc) -> Car // previous build scripts, we include them in the rustc invocation. // Artifacts are in a different location than typical units, hence - // we must assure the crate-dependent directory is present. + // we must assure the crate- and target-dependent directory is present. if is_artifact { paths::create_dir_all(&root)?; } diff --git a/tests/testsuite/artifact_dep.rs b/tests/testsuite/artifact_dep.rs index 669f04812d3..1f0271d243f 100644 --- a/tests/testsuite/artifact_dep.rs +++ b/tests/testsuite/artifact_dep.rs @@ -156,6 +156,7 @@ fn warn_about_artifact_and_no_artifact_dep_to_same_package_within_the_same_dep_c .run(); } +// TODO(ST): add static and cdylib artifacts, too. #[cargo_test] fn build_script_with_bin_artifacts() { let p = project() @@ -227,7 +228,7 @@ fn build_script_with_bin_artifacts() { let build_script_output = build_script_output_string(&p, "foo"); let msg = "we need the binary directory for this artifact along with all binary paths"; - #[cfg(not(windows))] + #[cfg(any(not(windows), target_env = "gnu"))] { cargo_test_support::compare::match_exact( "[..]/artifact/bar-[..]/bin/baz-[..]\n\ @@ -313,6 +314,49 @@ fn build_script_with_bin_artifact_and_lib_false() { .run(); } +#[cargo_test] +fn lib_with_bin_artifact_and_lib_false() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.0" + authors = [] + + [dependencies] + bar = { path = "bar/", artifact = "bin" } + "#, + ) + .file( + "src/lib.rs", + r#" + fn main() { + bar::doit() + }"#, + ) + .file("bar/Cargo.toml", &basic_bin_manifest("bar")) + .file("bar/src/main.rs", "fn main() { bar::doit(); }") + .file( + "bar/src/lib.rs", + r#" + pub fn doit() { + panic!("cannot be called from library due to lib = false"); + } + "#, + ) + .build(); + p.cargo("build -Z unstable-options -Z bindeps") + .masquerade_as_nightly_cargo() + .with_status(101) + .with_stderr_contains( + "error[E0433]: failed to resolve: use of undeclared crate or module `bar`", + ) + .with_stderr_contains(" --> src/lib.rs:3:16") + .run(); +} + #[cargo_test] fn build_script_with_selected_dashed_bin_artifact_and_lib_true() { let p = project() @@ -376,7 +420,7 @@ fn build_script_with_selected_dashed_bin_artifact_and_lib_true() { let build_script_output = build_script_output_string(&p, "foo"); let msg = "we need the binary directory for this artifact and the binary itself"; - #[cfg(not(windows))] + #[cfg(any(not(windows), target_env = "gnu"))] { cargo_test_support::compare::match_exact( "[..]/artifact/bar-baz-[..]/bin\n\ @@ -411,6 +455,68 @@ fn build_script_with_selected_dashed_bin_artifact_and_lib_true() { assert_artifact_executable_output(&p, "debug", "bar", "baz_suffix"); } +// TODO(ST): impl this, and add static and cdylib artifacts, too. +#[cargo_test] +fn lib_with_selected_dashed_bin_artifact_and_lib_true() { + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.0.0" + authors = [] + + [dependencies] + bar-baz = { path = "bar/", artifact = "bin:baz-suffix", lib = true } + "#, + ) + .file( + "src/lib.rs", + r#" + pub fn foo() { + bar_baz::exists(); + + env!("CARGO_BIN_DIR_BAR_BAZ"); + let _b = include_bytes!(env!("CARGO_BIN_FILE_BAR_BAZ_baz-suffix")); + } + "#, + ) + .file( + "bar/Cargo.toml", + r#" + [package] + name = "bar-baz" + version = "0.5.0" + authors = [] + + [[bin]] + name = "bar" + + [[bin]] + name = "baz-suffix" + "#, + ) + .file("bar/src/main.rs", "fn main() {}") + .file("bar/src/lib.rs", "pub fn exists() {}") + .build(); + p.cargo("build -Z unstable-options -Z bindeps") + .masquerade_as_nightly_cargo() + .with_stderr( + "\ +[COMPILING] bar-baz v0.5.0 ([CWD]/bar) +[COMPILING] foo [..] +[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]", + ) + .run(); + + assert!( + !p.bin("bar").is_file(), + "artifacts are located in their own directory, exclusively, and won't be lifted up" + ); + assert_artifact_executable_output(&p, "debug", "bar", "baz_suffix"); +} + #[cargo_test] fn allow_artifact_and_no_artifact_dep_to_same_package_within_different_dep_categories() { let p = project() @@ -999,7 +1105,7 @@ fn assert_artifact_executable_output( dep_name: &str, bin_name: &str, ) { - #[cfg(not(windows))] + #[cfg(any(not(windows), target_env = "gnu"))] { assert_eq!( p.glob(format!(