Skip to content

Commit

Permalink
Make artifact dependencies available in main loop
Browse files Browse the repository at this point in the history 
This is the commit message #2:
------------------------------

rough cut of support for artifact dependencies at build time…

…which unfortunately already shows that the binary it is supposed to
include is reproducibly not ready in time even though the path is
correct and it's present right after the run.

Could it be related to rmeta?

This is the commit message rust-lang#3:
------------------------------

Fix test expectations as failure is typical than the warning we had before…

…and add some tolerance to existing test to avoid occasional failures.

This doesn't change the issue that it also doens't work at all for
libraries, which is nicely reproducable and hopefully helps to fix
this issue.

This is the commit message rust-lang#4:
------------------------------

Probably the fix for the dependency issue in the scheduler

This means that bin() targets are now properly added to the job graph
to cause proper syncing, whereas previously apparently it would
still schedule binaries, but somehow consider them rmeta and thus
start their dependents too early, leading to races.

This is the commit message rust-lang#5:
------------------------------

Don't accidentally include non-gnu windows tests in gnu windows.
  • Loading branch information
@Byron
Byron committed Feb 18, 2022
1 parent 74a0a80 commit a6ebe1a
Show file tree
Hide file tree
Showing 4 changed files with 76 additions and 99 deletions.
2 changes: 1 addition & 1 deletion src/cargo/core/compiler/job_queue.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -395,7 +395,7 @@ impl<'cfg> JobQueue<'cfg> {
.filter(|dep| {
// Binaries aren't actually needed to *compile* tests, just to run
// them, so we don't include this dependency edge in the job graph.
!dep.unit.target.is_test() && !dep.unit.target.is_bin()
!dep.unit.target.is_test() && !dep.unit.target.is_bin() || dep.unit.artifact
})
.map(|dep| {
// Handle the case here where our `unit -> dep` dependency may
Expand Down
63 changes: 48 additions & 15 deletions src/cargo/core/compiler/unit_dependencies.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,14 +263,6 @@ fn compute_deps(
return false;
}

// Artifact dependencies are only counted as standard libraries if they are marked
// as 'library as well'
if let Some(artifact) = dep.artifact() {
if !artifact.is_lib() {
return false;
}
}

// If we've gotten past all that, then this dependency is
// actually used!
true
Expand All @@ -280,7 +272,48 @@ fn compute_deps(
let mut dev_deps = Vec::new();
for (id, deps) in filtered_deps {
let pkg = state.get(id);
let lib = match pkg.targets().iter().find(|t| t.is_lib()) {
// Artifact dependencies are only counted as standard libraries if they are marked
// as 'library as well'. We don't filter in the closure above as we still want to get a chance
// to process them as pure non-lib artifact dependencies.
let mut has_artifact = false;
let mut artifact_lib = false;
// Custom build scripts (build/compile) never get artifact dependencies, but the run-build-script step does.
let artifact_pkg = state.get(id);
for (dep, artifact) in deps
.iter()
.filter_map(|dep| dep.artifact().map(|a| (dep, a)))
{
has_artifact = true;
artifact_lib |= artifact.is_lib();
if !unit.target.is_custom_build() && !unit.mode.is_run_custom_build() {
ret.extend(
match_artifacts_kind_with_targets(unit, dep, artifact_pkg.targets())?
.into_iter()
.map(|target| {
new_unit_dep(
state,
unit,
artifact_pkg,
target,
unit_for, // TODO(ST): definitely check what's best here.
unit.kind, // TODO(ST): handle target="target", and other cases
check_or_build_mode(unit.mode, target),
/*artifact*/ true,
)
})
.collect::<Result<Vec<_>, _>>()?,
);
}
}

let lib = pkg.targets().iter().find(|t| {
if has_artifact {
t.is_lib() && artifact_lib
} else {
t.is_lib()
}
});
let lib = match lib {
Some(t) => t,
None => continue,
};
Expand Down Expand Up @@ -451,7 +484,7 @@ fn compute_deps_custom_build(
if artifact_build_deps.is_empty() {
Ok(vec![compile_script_unit])
} else {
let mut artifact_units: Vec<_> = artifact_requirements_to_units(
let mut artifact_units: Vec<_> = build_artifact_requirements_to_units(
unit,
script_unit_for,
artifact_build_deps,
Expand All @@ -463,18 +496,18 @@ fn compute_deps_custom_build(
}
}

fn artifact_requirements_to_units(
fn build_artifact_requirements_to_units(
parent: &Unit,
parent_unit_for: UnitFor,
artifact_deps: Vec<(PackageId, &HashSet<Dependency>)>,
state: &State<'_, '_>,
kind: CompileKind,
) -> CargoResult<Vec<UnitDep>> {
let mut out = Vec::new();
let mut ret = Vec::new();
for (dep_pkg_id, deps) in artifact_deps {
let artifact_pkg = state.get(dep_pkg_id);
for build_dep in deps.iter().filter(|d| d.is_build()) {
let artifact_pkg = state.get(dep_pkg_id);
out.extend(
ret.extend(
match_artifacts_kind_with_targets(parent, build_dep, artifact_pkg.targets())?
.into_iter()
.map(|target| {
Expand All @@ -494,7 +527,7 @@ fn artifact_requirements_to_units(
);
}
}
Ok(out)
Ok(ret)
}

fn match_artifacts_kind_with_targets<'a>(
Expand Down
30 changes: 0 additions & 30 deletions src/cargo/core/package.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -578,36 +578,6 @@ impl<'cfg> PackageSet<'cfg> {
)
.collect();

// TODO: try to move these checks to later stage where a better error can be produced, and maybe where
// the checks are happening as a side-effect of creating actual build targets.
for dep_ids_and_deps_for_kind in
[DepKind::Normal, DepKind::Build, DepKind::Development]
.iter()
.map(|&dep_kind| dep_pkgs_to_deps.iter().filter_map(move |(_dep_id, deps)| {
let mut iter = deps
.iter()
.filter(move |dep| dep.kind() == dep_kind)
.peekable();
iter.peek().is_some().then(|| iter.collect::<Vec<_>>())
})
)
{
for deps in dep_ids_and_deps_for_kind {
let artifact_name = deps
.iter()
.find_map(|dep| dep.artifact().is_some().then(|| dep.name_in_toml()));
let non_artifact_name = deps
.iter()
.find_map(|dep| dep.artifact().is_none().then(|| dep.name_in_toml()));
if let (Some(artifact_name), Some(non_artifact_name)) =
(artifact_name, non_artifact_name)
{
ws.config().shell().warn(format!(r#"Consider setting 'lib = true' in artifact dependency '{}' instead of declaring '{}' separately."#,
artifact_name, non_artifact_name))?;
}
}
}

let dep_pkgs = dep_pkgs_to_deps
.iter()
.filter(|(_id, deps)| deps.iter().all(|dep| dep.maybe_lib()))
Expand Down
80 changes: 27 additions & 53 deletions tests/testsuite/artifact_dep.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ fn build_without_nightly_shows_warnings_and_ignores_them() {
.file("bar/src/lib.rs", "")
.build();
p.cargo("check")
// TODO(ST): use backticks instead of single quotes for quotation for consistency.
.with_stderr(
"\
[WARNING] 'lib' specifiers need an 'artifact = …' value and would fail the operation when '-Z bindeps' is provided.
Expand All @@ -126,7 +127,7 @@ fn build_without_nightly_shows_warnings_and_ignores_them() {
}

#[cargo_test]
fn warn_about_artifact_and_no_artifact_dep_to_same_package_within_the_same_dep_category() {
fn disallow_artifact_and_no_artifact_dep_to_same_package_within_the_same_dep_category() {
let p = project()
.file(
"Cargo.toml",
Expand All @@ -143,15 +144,13 @@ fn warn_about_artifact_and_no_artifact_dep_to_same_package_within_the_same_dep_c
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
.file("bar/src/main.rs", "")
.file("bar/src/main.rs", "fn main() {}")
.build();
p.cargo("check -Z unstable-options -Z bindeps")
.masquerade_as_nightly_cargo()
.with_status(101)
.with_stderr(
"\
[WARNING] Consider setting 'lib = true' in artifact dependency 'bar' instead of declaring 'bar_stable' separately.
[CHECKING] foo [..]
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
"[ERROR] the crate `foo v0.0.0 ([CWD])` depends on crate `bar v0.5.0 ([CWD]/bar)` multiple times with different names",
)
.run();
}
Expand All @@ -174,30 +173,18 @@ fn build_script_with_bin_artifacts() {
)
.file("src/lib.rs", "")
.file("build.rs", r#"
// TODO(ST): figure out why the file may not be there right away
fn assert_file_with_tolerance(path: &std::path::Path) {
if path.is_file() {
return
}
std::thread::sleep(std::time::Duration::from_millis(100));
if path.is_file() {
return
}
panic!("File at '{}' wasn't present even after retrying", path.display());
}
fn main() {
let baz: std::path::PathBuf = std::env::var("CARGO_BIN_FILE_BAR_baz").expect("CARGO_BIN_FILE_BAR_baz").into();
println!("{}", baz.display());
assert_file_with_tolerance(&baz);
assert!(&baz.is_file());
let dir: std::path::PathBuf = std::env::var("CARGO_BIN_DIR_BAR").expect("CARGO_BIN_DIR_BAR").into();
println!("{}", dir.display());
assert!(dir.is_dir());
let bar: std::path::PathBuf = std::env::var("CARGO_BIN_FILE_BAR").expect("CARGO_BIN_FILE_BAR").into();
println!("{}", bar.display());
assert_file_with_tolerance(&bar);
assert!(&bar.is_file());
let bar2: std::path::PathBuf = std::env::var("CARGO_BIN_FILE_BAR_bar").expect("CARGO_BIN_FILE_BAR_bar").into();
println!("{}", bar2.display());
Expand All @@ -218,12 +205,9 @@ fn build_script_with_bin_artifacts() {
.build();
p.cargo("build -Z unstable-options -Z bindeps")
.masquerade_as_nightly_cargo()
.with_stderr(
"\
[COMPILING] foo [..]
[COMPILING] bar v0.5.0 ([CWD]/bar)
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]",
)
.with_stderr_contains("[COMPILING] foo [..]")
.with_stderr_contains("[COMPILING] bar v0.5.0 ([CWD]/bar)")
.with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
.run();

let build_script_output = build_script_output_string(&p, "foo");
Expand All @@ -242,7 +226,7 @@ fn build_script_with_bin_artifacts() {
)
.unwrap();
}
#[cfg(windows)]
#[cfg(all(windows, not(target_env = "gnu")))]
{
cargo_test_support::compare::match_exact(
&format!(
Expand Down Expand Up @@ -401,7 +385,7 @@ fn build_script_with_selected_dashed_bin_artifact_and_lib_true() {
println!("{}", dir.display());
println!("{}", bin.display());
assert!(dir.is_dir());
assert!(bin.is_file());
assert!(&bin.is_file());
assert!(std::env::var("CARGO_BIN_FILE_BAR_BAZ").is_err(), "CARGO_BIN_FILE_BAR_BAZ isn't set due to name mismatch");
assert!(std::env::var("CARGO_BIN_FILE_BAR_BAZ_bar").is_err(), "CARGO_BIN_FILE_BAR_BAZ_bar isn't set as binary isn't selected");
}
Expand Down Expand Up @@ -432,7 +416,7 @@ fn build_script_with_selected_dashed_bin_artifact_and_lib_true() {
)
.unwrap();
}
#[cfg(windows)]
#[cfg(all(windows, not(target_env = "gnu")))]
{
cargo_test_support::compare::match_exact(
&format!(
Expand All @@ -457,7 +441,6 @@ fn build_script_with_selected_dashed_bin_artifact_and_lib_true() {

// TODO(ST): impl this, and add static and cdylib artifacts, too.
#[cargo_test]
#[ignore]
fn lib_with_selected_dashed_bin_artifact_and_lib_true() {
let p = project()
.file(
Expand Down Expand Up @@ -538,16 +521,13 @@ fn allow_artifact_and_no_artifact_dep_to_same_package_within_different_dep_categ
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
.file("bar/src/main.rs", "")
.file("bar/src/main.rs", "fn main() {}")
.build();
p.cargo("check -Z unstable-options -Z bindeps")
.masquerade_as_nightly_cargo()
.with_stderr(
"\
[CHECKING] foo [..]
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
)
.with_stderr_contains("[CHECKING] foo [..]")
.with_stderr_contains("[CHECKING] bar v0.5.0 ([CWD]/bar)")
.with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
.run();
}

Expand Down Expand Up @@ -740,16 +720,13 @@ fn prevent_no_lib_warning_with_artifact_dependencies() {
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
.file("bar/src/main.rs", "")
.file("bar/src/main.rs", "fn main() {}")
.build();
p.cargo("check -Z unstable-options -Z bindeps")
.masquerade_as_nightly_cargo()
.with_stderr(
"\
[CHECKING] foo [..]
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
)
.with_stderr_contains("[CHECKING] bar v0.5.0 ([CWD]/bar)")
.with_stderr_contains("[CHECKING] foo v0.0.0 ([CWD])")
.with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
.run();
}

Expand All @@ -770,17 +747,14 @@ fn show_no_lib_warning_with_artifact_dependencies_that_have_no_lib_but_lib_true(
)
.file("src/lib.rs", "")
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
.file("bar/src/main.rs", "")
.file("bar/src/main.rs", "fn main() {}")
.build();
p.cargo("check -Z unstable-options -Z bindeps")
.masquerade_as_nightly_cargo()
.with_stderr(
"\
[WARNING] foo v0.0.0 ([CWD]) ignoring invalid dependency `bar` which is missing a lib target
[CHECKING] foo [..]
[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]
",
)
.with_stderr_contains("[WARNING] foo v0.0.0 ([CWD]) ignoring invalid dependency `bar` which is missing a lib target")
.with_stderr_contains("[CHECKING] bar v0.5.0 ([CWD]/bar)")
.with_stderr_contains("[CHECKING] foo [..]")
.with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
.run();
}

Expand Down Expand Up @@ -1123,7 +1097,7 @@ fn assert_artifact_executable_output(
"artifacts are placed into their own output directory to not possibly clash"
);
}
#[cfg(windows)]
#[cfg(all(windows, not(target_env = "gnu")))]
{
assert_eq!(
p.glob(format!(
Expand Down

0 comments on commit a6ebe1a

Please sign in to comment.