Skip to content

Commit b478370

Browse files
committed
fix incorrect removal of non-artifact libs
This is also the first step towards cleaning up the filtering logic which is still making some logic harder to understand than needs be. The goal is to get it to be closer to what's currently on master. Another test was added to have more safety regarding the overall library inclusion logic.
1 parent cd25256 commit b478370

File tree

2 files changed

+87
-36
lines changed

2 files changed

+87
-36
lines changed

src/cargo/core/compiler/unit_dependencies.rs

Lines changed: 46 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -271,11 +271,11 @@ fn compute_deps(
271271
let mut dev_deps = Vec::new();
272272
for (dep_pkg_id, deps) in filtered_deps {
273273
let dep_pkg = state.get(dep_pkg_id);
274-
let (has_artifact, artifact_lib) = calc_artifact_deps(
274+
let (could_have_non_artifact_lib, has_artifact_lib) = calc_artifact_deps(
275275
unit, unit_for, dep_pkg_id, deps, state, dep_filter, &mut ret,
276276
)?;
277277

278-
let lib = package_lib(dep_pkg, has_artifact, artifact_lib);
278+
let lib = package_lib(dep_pkg, could_have_non_artifact_lib, has_artifact_lib);
279279
let dep_lib = match lib {
280280
Some(t) => t,
281281
None => continue,
@@ -402,14 +402,15 @@ fn compute_deps(
402402
///
403403
/// `has_artifact` is true if `dep_pkg` has any artifact, and `artifact_lib` is true
404404
/// if any of them wants a library to be available too.
405-
fn package_lib(dep_pkg: &Package, has_artifact: bool, artifact_lib: bool) -> Option<&Target> {
406-
dep_pkg.targets().iter().find(|t| {
407-
if has_artifact {
408-
t.is_lib() && artifact_lib
409-
} else {
410-
t.is_lib()
411-
}
412-
})
405+
fn package_lib(
406+
dep_pkg: &Package,
407+
could_have_non_artifact_lib: bool,
408+
artifact_lib: bool,
409+
) -> Option<&Target> {
410+
dep_pkg
411+
.targets()
412+
.iter()
413+
.find(|t| t.is_lib() && (could_have_non_artifact_lib || artifact_lib))
413414
}
414415

415416
/// Find artifacts for all `deps` of `unit` and add units that build these artifacts
@@ -423,16 +424,24 @@ fn calc_artifact_deps(
423424
filter: &dyn Fn(&Unit, &Dependency) -> bool,
424425
ret: &mut Vec<UnitDep>,
425426
) -> CargoResult<(bool, bool)> {
426-
let mut has_artifact = false;
427-
let mut artifact_lib = false;
427+
let mut has_artifact_lib = false;
428+
let mut num_artifacts = 0;
428429
let artifact_pkg = state.get(dep_id);
430+
let mut deps_past_filter = 0;
429431
for (dep, artifact) in deps
430432
.iter()
431-
.filter(|dep| filter(unit, dep))
433+
.filter(|dep| {
434+
if filter(unit, dep) {
435+
deps_past_filter += 1;
436+
true
437+
} else {
438+
false
439+
}
440+
})
432441
.filter_map(|dep| dep.artifact().map(|a| (dep, a)))
433442
{
434-
has_artifact = true;
435-
artifact_lib |= artifact.is_lib();
443+
has_artifact_lib |= artifact.is_lib();
444+
num_artifacts += 1;
436445
// Custom build scripts (build/compile) never get artifact dependencies,
437446
// but the run-build-script step does (where it is handled).
438447
if !unit.target.is_custom_build() {
@@ -456,7 +465,8 @@ fn calc_artifact_deps(
456465
)?);
457466
}
458467
}
459-
Ok((has_artifact, artifact_lib))
468+
let could_be_non_artifact_lib = deps_past_filter != num_artifacts;
469+
Ok((could_be_non_artifact_lib, has_artifact_lib))
460470
}
461471

462472
/// Returns the dependencies needed to run a build script.
@@ -506,22 +516,25 @@ fn compute_deps_custom_build(
506516
IS_NO_ARTIFACT_DEP,
507517
)?;
508518

519+
let mut result = vec![compile_script_unit];
520+
521+
// Include any artifact dependencies.
522+
//
523+
// This is essentially the same as `calc_artifact_deps`, but there are some
524+
// subtle differences that require this to be implemented differently.
509525
let artifact_build_deps = state.deps(unit, script_unit_for, &|_unit, dep| {
510526
dep.kind() == DepKind::Build && dep.artifact().is_some()
511527
});
512528

513-
if artifact_build_deps.is_empty() {
514-
Ok(vec![compile_script_unit])
515-
} else {
516-
let mut artifact_units: Vec<_> = build_artifact_requirements_to_units(
517-
unit,
518-
unit_for.root_compile_kind(),
519-
artifact_build_deps,
520-
state,
521-
)?;
522-
artifact_units.push(compile_script_unit);
523-
Ok(artifact_units)
524-
}
529+
build_artifact_requirements_to_units(
530+
unit,
531+
unit_for.root_compile_kind(),
532+
artifact_build_deps,
533+
state,
534+
&mut result,
535+
)?;
536+
537+
Ok(result)
525538
}
526539

527540
/// Given a `parent` unit which is a build script with artifact dependencies `artifact_deps`,
@@ -537,8 +550,8 @@ fn build_artifact_requirements_to_units(
537550
root_unit_compile_target: CompileKind,
538551
artifact_deps: Vec<(PackageId, &HashSet<Dependency>)>,
539552
state: &State<'_, '_>,
540-
) -> CargoResult<Vec<UnitDep>> {
541-
let mut ret = Vec::new();
553+
ret: &mut Vec<UnitDep>,
554+
) -> CargoResult<()> {
542555
// This really wants to be true for build dependencies, otherwise resolver = "2"
543556
// will fail. // It means that the host features will be separated from normal
544557
// features, thus won't be unified with them.
@@ -564,7 +577,7 @@ fn build_artifact_requirements_to_units(
564577
)?);
565578
}
566579
}
567-
Ok(ret)
580+
Ok(())
568581
}
569582

570583
/// Given a `parent` unit containing a dependency `dep` whose package is `artifact_pkg`,
@@ -682,11 +695,11 @@ fn compute_deps_doc(
682695
// the documentation of the library being built.
683696
let mut ret = Vec::new();
684697
for (id, deps) in deps {
685-
let (has_artifact, artifact_lib) =
698+
let (could_have_non_artifact_lib, has_artifact_lib) =
686699
calc_artifact_deps(unit, unit_for, id, deps, state, dep_filter, &mut ret)?;
687700

688701
let dep_pkg = state.get(id);
689-
let lib = package_lib(dep_pkg, has_artifact, artifact_lib);
702+
let lib = package_lib(dep_pkg, could_have_non_artifact_lib, has_artifact_lib);
690703
let dep_lib = match lib {
691704
Some(lib) => lib,
692705
None => continue,

tests/testsuite/artifact_dep.rs

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -804,19 +804,57 @@ fn allow_artifact_and_no_artifact_dep_to_same_package_within_different_dep_categ
804804
.file(
805805
"src/lib.rs",
806806
r#"
807+
#[cfg(test)] extern crate bar;
807808
pub fn foo() {
808809
env!("CARGO_BIN_DIR_BAR");
809810
let _b = include_bytes!(env!("CARGO_BIN_FILE_BAR"));
810811
}"#,
811812
)
812813
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
813814
.file("bar/src/main.rs", "fn main() {}")
815+
.file("bar/src/lib.rs", "")
814816
.build();
815-
p.cargo("check -Z bindeps")
817+
p.cargo("test -Z bindeps")
816818
.masquerade_as_nightly_cargo()
817819
.with_stderr_contains("[COMPILING] bar v0.5.0 ([CWD]/bar)")
818-
.with_stderr_contains("[CHECKING] foo [..]")
819-
.with_stderr_contains("[FINISHED] dev [unoptimized + debuginfo] target(s) in [..]")
820+
.with_stderr_contains("[FINISHED] test [unoptimized + debuginfo] target(s) in [..]")
821+
.run();
822+
}
823+
824+
#[cargo_test]
825+
fn normal_build_deps_are_picked_up_in_presence_of_an_artifact_build_dep_to_the_same_package() {
826+
let p = project()
827+
.file(
828+
"Cargo.toml",
829+
r#"
830+
[package]
831+
name = "foo"
832+
version = "0.0.0"
833+
authors = []
834+
resolver = "2"
835+
836+
[dependencies]
837+
bar = { path = "bar", artifact = "bin:bar" }
838+
839+
[build-dependencies]
840+
bar = { path = "bar" }
841+
"#,
842+
)
843+
.file("build.rs", "fn main() { bar::f(); }")
844+
.file(
845+
"src/lib.rs",
846+
r#"
847+
pub fn foo() {
848+
env!("CARGO_BIN_DIR_BAR");
849+
let _b = include_bytes!(env!("CARGO_BIN_FILE_BAR"));
850+
}"#,
851+
)
852+
.file("bar/Cargo.toml", &basic_bin_manifest("bar"))
853+
.file("bar/src/main.rs", "fn main() {}")
854+
.file("bar/src/lib.rs", "pub fn f() {}")
855+
.build();
856+
p.cargo("check -Z bindeps")
857+
.masquerade_as_nightly_cargo()
820858
.run();
821859
}
822860

0 commit comments

Comments
 (0)