From c1f7bb92855e5622adcb1b62449b89ef9f3a4d3a Mon Sep 17 00:00:00 2001 From: Vitali Lovich Date: Tue, 4 Mar 2025 05:51:53 -0800 Subject: [PATCH 1/3] Refactor get_dynamic_search_path Will need it in a follow-up PR --- src/cargo/core/compiler/mod.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index 8cdf9310a5b..f193fd8b8dd 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -659,6 +659,13 @@ fn add_plugin_deps( Ok(()) } +fn get_dynamic_search_path(path: &Path) -> &Path { + match path.to_str().and_then(|s| s.split_once("=")) { + Some(("native" | "crate" | "dependency" | "framework" | "all", path)) => Path::new(path), + _ => path, + } +} + // Determine paths to add to the dynamic search path from -L entries // // Strip off prefixes like "native=" or "framework=" and filter out directories @@ -670,12 +677,9 @@ where { let mut search_path = vec![]; for dir in paths { - let dir = match dir.to_str().and_then(|s| s.split_once("=")) { - Some(("native" | "crate" | "dependency" | "framework" | "all", path)) => path.into(), - _ => dir.clone(), - }; + let dir = get_dynamic_search_path(dir); if dir.starts_with(&root_output) { - search_path.push(dir); + search_path.push(dir.to_path_buf()); } else { debug!( "Not including path {} in runtime library search path because it is \ From f94b75aeb602dafff24c3d4fe2766b0f9fc0c9e6 Mon Sep 17 00:00:00 2001 From: Vitali Lovich Date: Tue, 4 Mar 2025 05:56:08 -0800 Subject: [PATCH 2/3] Add test case for current behavior. --- tests/testsuite/build_script.rs | 87 +++++++++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index a072d567f2d..dcb90dca1f5 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -6087,3 +6087,90 @@ fn directory_with_leading_underscore() { .with_status(0) .run(); } + +#[cargo_test] +fn linker_search_path_preference() { + // This isn't strictly the exact scenario that causes the issue, but it's the shortest demonstration + // of the issue. + let p = project() + .file( + "Cargo.toml", + r#" + [package] + name = "foo" + version = "0.1.0" + edition = "2024" + build = "build.rs" + + [dependencies] + a = { path = "a" } + b = { path = "b" } + "#, + ) + .file( + "build.rs", + r#" + fn main() { + let out_dir = std::env::var("OUT_DIR").unwrap(); + println!("cargo::rustc-link-search=/usr/lib"); + println!("cargo::rustc-link-search={}/libs2", out_dir); + println!("cargo::rustc-link-search=/lib"); + println!("cargo::rustc-link-search={}/libs1", out_dir); + } + "#, + ) + .file("src/main.rs", "fn main() {}") + .file( + "a/Cargo.toml", + r#" + [package] + name = "a" + version = "0.1.0" + edition = "2024" + build = "build.rs" + "#, + ) + .file("a/src/lib.rs", "") + .file( + "a/build.rs", + r#" + fn main() { + let out_dir = std::env::var("OUT_DIR").unwrap(); + println!("cargo::rustc-link-search=/usr/lib3"); + println!("cargo::rustc-link-search={}/libsA.2", out_dir); + println!("cargo::rustc-link-search=/lib3"); + println!("cargo::rustc-link-search={}/libsA.1", out_dir); + } + "#, + ) + .file( + "b/Cargo.toml", + r#" + [package] + name = "b" + version = "0.1.0" + edition = "2024" + build = "build.rs" + "#, + ) + .file("b/src/lib.rs", "") + .file( + "b/build.rs", + r#" + fn main() { + let out_dir = std::env::var("OUT_DIR").unwrap(); + println!("cargo::rustc-link-search=/usr/lib2"); + println!("cargo::rustc-link-search={}/libsB.1", out_dir); + println!("cargo::rustc-link-search=/lib2"); + println!("cargo::rustc-link-search={}/libsB.2", out_dir); + } + "#, + ) + .build(); + + p.cargo("build -v").with_stderr_data(str![[r#" +... +[RUNNING] `rustc --crate-name foo [..] -L /usr/lib -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs2 -L /lib -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs1 -L /usr/lib3 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.2 -L /lib3 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.1 -L /usr/lib2 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.1 -L /lib2 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.2` +... +"#]]).run(); +} \ No newline at end of file From 6c6b34ea2704fb4dc4c87afcdc446557ca2a4074 Mon Sep 17 00:00:00 2001 From: Vitali Lovich Date: Tue, 4 Mar 2025 05:52:43 -0800 Subject: [PATCH 3/3] Search cargo build directories before system for libraries Regardless of crate search paths emitted, always prefer searching search paths pointing into the artifacts directory to those pointing outside. This way libraries built by Cargo are preferred even if the same library name exists in the system & a crate earlier in the build process emitted a system library path for searching. --- src/cargo/core/compiler/build_runner/mod.rs | 4 +- src/cargo/core/compiler/custom_build.rs | 81 +++++++++++++++++++-- src/cargo/core/compiler/mod.rs | 33 +++++++-- src/cargo/util/context/target.rs | 14 ++-- tests/testsuite/build_script.rs | 4 +- 5 files changed, 118 insertions(+), 18 deletions(-) diff --git a/src/cargo/core/compiler/build_runner/mod.rs b/src/cargo/core/compiler/build_runner/mod.rs index 16476693784..645ffb2a8fa 100644 --- a/src/cargo/core/compiler/build_runner/mod.rs +++ b/src/cargo/core/compiler/build_runner/mod.rs @@ -301,7 +301,9 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> { .extend(output.env.iter().cloned()); for dir in output.library_paths.iter() { - self.compilation.native_dirs.insert(dir.clone()); + self.compilation + .native_dirs + .insert(dir.clone().into_path_buf()); } } Ok(self.compilation) diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs index 8928c93be10..4629309bc52 100644 --- a/src/cargo/core/compiler/custom_build.rs +++ b/src/cargo/core/compiler/custom_build.rs @@ -31,7 +31,7 @@ //! [`CompileMode::RunCustomBuild`]: super::CompileMode //! [instructions]: https://doc.rust-lang.org/cargo/reference/build-scripts.html#outputs-of-the-build-script -use super::{fingerprint, BuildRunner, Job, Unit, Work}; +use super::{fingerprint, get_dynamic_search_path, BuildRunner, Job, Unit, Work}; use crate::core::compiler::artifact; use crate::core::compiler::build_runner::UnitHash; use crate::core::compiler::fingerprint::DirtyReason; @@ -74,11 +74,76 @@ pub enum Severity { pub type LogMessage = (Severity, String); +/// Represents a path added to the library search path. +/// +/// We need to keep track of requests to add search paths within the cargo build directory +/// separately from paths outside of Cargo. The reason is that we want to give precedence to linking +/// against libraries within the Cargo build directory even if a similar library exists in the +/// system (e.g. crate A adds `/usr/lib` to the search path and then a later build of crate B adds +/// `target/debug/...` to satisfy its request to link against the library B that it built, but B is +/// also found in `/usr/lib`). +/// +/// There's some nuance here because we want to preserve relative order of paths of the same type. +/// For example, if the build process would in declaration order emit the following linker line: +/// ```bash +/// -L/usr/lib -Ltarget/debug/build/crate1/libs -L/lib -Ltarget/debug/build/crate2/libs) +/// ``` +/// +/// we want the linker to actually receive: +/// ```bash +/// -Ltarget/debug/build/crate1/libs -Ltarget/debug/build/crate2/libs) -L/usr/lib -L/lib +/// ``` +/// +/// so that the library search paths within the crate artifacts directory come first but retain +/// relative ordering while the system library paths come after while still retaining relative +/// ordering among them; ordering is the order they are emitted within the build process, +/// not lexicographic order. +/// +/// WARNING: Even though this type implements PartialOrd + Ord, this is a lexicographic ordering. +/// The linker line will require an explicit sorting algorithm. PartialOrd + Ord is derived because +/// BuildOutput requires it but that ordering is different from the one for the linker search path, +/// at least today. It may be worth reconsidering & perhaps it's ok if BuildOutput doesn't have +/// a lexicographic ordering for the library_paths? I'm not sure the consequence of that. +#[derive(Clone, Debug, Hash, PartialEq, Eq, PartialOrd, Ord)] +pub enum LibraryPath { + /// The path is pointing within the output folder of the crate and takes priority over + /// external paths when passed to the linker. + CargoArtifact(PathBuf), + /// The path is pointing outside of the crate's build location. The linker will always + /// receive such paths after `CargoArtifact`. + External(PathBuf), +} + +impl LibraryPath { + fn new(p: PathBuf, script_out_dir: &Path) -> Self { + let search_path = get_dynamic_search_path(&p); + if search_path.starts_with(script_out_dir) { + Self::CargoArtifact(p) + } else { + Self::External(p) + } + } + + pub fn into_path_buf(self) -> PathBuf { + match self { + LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p, + } + } +} + +impl AsRef for LibraryPath { + fn as_ref(&self) -> &PathBuf { + match self { + LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p, + } + } +} + /// Contains the parsed output of a custom build script. #[derive(Clone, Debug, Hash, Default, PartialEq, Eq, PartialOrd, Ord)] pub struct BuildOutput { /// Paths to pass to rustc with the `-L` flag. - pub library_paths: Vec, + pub library_paths: Vec, /// Names and link kinds of libraries, suitable for the `-l` flag. pub library_links: Vec, /// Linker arguments suitable to be passed to `-C link-arg=` @@ -237,7 +302,7 @@ fn emit_build_output( let library_paths = output .library_paths .iter() - .map(|l| l.display().to_string()) + .map(|l| l.as_ref().display().to_string()) .collect::>(); let msg = machine_message::BuildScript { @@ -884,10 +949,16 @@ impl BuildOutput { "rustc-flags" => { let (paths, links) = BuildOutput::parse_rustc_flags(&value, &whence)?; library_links.extend(links.into_iter()); - library_paths.extend(paths.into_iter()); + library_paths.extend( + paths + .into_iter() + .map(|p| LibraryPath::new(p, script_out_dir)), + ); } "rustc-link-lib" => library_links.push(value.to_string()), - "rustc-link-search" => library_paths.push(PathBuf::from(value)), + "rustc-link-search" => { + library_paths.push(LibraryPath::new(PathBuf::from(value), script_out_dir)) + } "rustc-link-arg-cdylib" | "rustc-cdylib-link-arg" => { if !targets.iter().any(|target| target.is_cdylib()) { log_messages.push(( diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs index f193fd8b8dd..59c135240c6 100644 --- a/src/cargo/core/compiler/mod.rs +++ b/src/cargo/core/compiler/mod.rs @@ -79,7 +79,7 @@ pub use self::compilation::{Compilation, Doctest, UnitOutput}; pub use self::compile_kind::{CompileKind, CompileTarget}; pub use self::crate_type::CrateType; pub use self::custom_build::LinkArgTarget; -pub use self::custom_build::{BuildOutput, BuildScriptOutputs, BuildScripts}; +pub use self::custom_build::{BuildOutput, BuildScriptOutputs, BuildScripts, LibraryPath}; pub(crate) use self::fingerprint::DirtyReason; pub use self::job_queue::Freshness; use self::job_queue::{Job, JobQueue, JobState, Work}; @@ -495,6 +495,32 @@ fn rustc( target: &Target, current_id: PackageId, ) -> CargoResult<()> { + let mut library_paths = vec![]; + + for key in build_scripts.to_link.iter() { + let output = build_script_outputs.get(key.1).ok_or_else(|| { + internal(format!( + "couldn't find build script output for {}/{}", + key.0, key.1 + )) + })?; + library_paths.extend(output.library_paths.iter()); + } + + // NOTE: This very intentionally does not use the derived ord from LibraryPath because we need to + // retain relative ordering within the same type (i.e. not lexicographic). The use of a stable sort + // is also important here because it ensures that paths of the same type retain the same relative + // ordering (for an unstable sort to work here, the list would need to retain the idx of each element + // and then sort by that idx when the type is equivalent. + library_paths.sort_by_key(|p| match p { + LibraryPath::CargoArtifact(_) => 0, + LibraryPath::External(_) => 1, + }); + + for path in library_paths.iter() { + rustc.arg("-L").arg(path.as_ref()); + } + for key in build_scripts.to_link.iter() { let output = build_script_outputs.get(key.1).ok_or_else(|| { internal(format!( @@ -502,9 +528,6 @@ fn rustc( key.0, key.1 )) })?; - for path in output.library_paths.iter() { - rustc.arg("-L").arg(path); - } if key.0 == current_id { if pass_l_flag { @@ -650,7 +673,7 @@ fn add_plugin_deps( .get(*metadata) .ok_or_else(|| internal(format!("couldn't find libs for plugin dep {}", pkg_id)))?; search_path.append(&mut filter_dynamic_search_path( - output.library_paths.iter(), + output.library_paths.iter().map(AsRef::as_ref), root_output, )); } diff --git a/src/cargo/util/context/target.rs b/src/cargo/util/context/target.rs index 3de3d826a59..0dd5cfc4ce9 100644 --- a/src/cargo/util/context/target.rs +++ b/src/cargo/util/context/target.rs @@ -1,5 +1,5 @@ use super::{ConfigKey, ConfigRelativePath, GlobalContext, OptValue, PathAndArgs, StringList, CV}; -use crate::core::compiler::{BuildOutput, LinkArgTarget}; +use crate::core::compiler::{BuildOutput, LibraryPath, LinkArgTarget}; use crate::util::CargoResult; use serde::Deserialize; use std::collections::{BTreeMap, HashMap}; @@ -167,7 +167,9 @@ fn parse_links_overrides( let flags = value.string(key)?; let whence = format!("target config `{}.{}` (in {})", target_key, key, flags.1); let (paths, links) = BuildOutput::parse_rustc_flags(flags.0, &whence)?; - output.library_paths.extend(paths); + output + .library_paths + .extend(paths.into_iter().map(LibraryPath::External)); output.library_links.extend(links); } "rustc-link-lib" => { @@ -178,9 +180,11 @@ fn parse_links_overrides( } "rustc-link-search" => { let list = value.list(key)?; - output - .library_paths - .extend(list.iter().map(|v| PathBuf::from(&v.0))); + output.library_paths.extend( + list.iter() + .map(|v| PathBuf::from(&v.0)) + .map(LibraryPath::External), + ); } "rustc-link-arg-cdylib" | "rustc-cdylib-link-arg" => { let args = extra_link_args(LinkArgTarget::Cdylib, key, value)?; diff --git a/tests/testsuite/build_script.rs b/tests/testsuite/build_script.rs index dcb90dca1f5..12b1b66632f 100644 --- a/tests/testsuite/build_script.rs +++ b/tests/testsuite/build_script.rs @@ -6170,7 +6170,7 @@ fn linker_search_path_preference() { p.cargo("build -v").with_stderr_data(str![[r#" ... -[RUNNING] `rustc --crate-name foo [..] -L /usr/lib -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs2 -L /lib -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs1 -L /usr/lib3 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.2 -L /lib3 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.1 -L /usr/lib2 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.1 -L /lib2 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.2` +[RUNNING] `rustc --crate-name foo [..] -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs2 -L [ROOT]/foo/target/debug/build/foo-[HASH]/out/libs1 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.2 -L [ROOT]/foo/target/debug/build/a-[HASH]/out/libsA.1 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.1 -L [ROOT]/foo/target/debug/build/b-[HASH]/out/libsB.2 -L /usr/lib -L /lib -L /usr/lib3 -L /lib3 -L /usr/lib2 -L /lib2` ... "#]]).run(); -} \ No newline at end of file +}