mirror of
https://github.com/rust-lang/cargo.git
synced 2025-09-28 11:20:36 +00:00
Make sure search paths inside OUT_DIR precede external paths (#15221)
If a library exists both in an added folder inside OUT_DIR and in the OS, prefer to use the one within OUT_DIR. Folders within OUT_DIR and folders outside OUT_DIR do not change their relative order between themselves. This is accomplished by sorting by whether we think the path is inside the search path or outside. ### What does this PR try to resolve? Fixes #15220. If a Rust crates builds a dynamic library & that same dynamic library is installed in the host OS, the result of the build's success & consistent behavior of executed tools depends on whether or not the user has the conflicting dynamic library in the external search path. If they do, then the host OS library will always be used which is unexpected - updates to your Rust dependency will still have you linking & running against an old host OS library (i.e. someone who doesn't have that library has a different silent behavior). ### How should we test and review this PR? This is what I did to verify my issue got resolved but I'm sure there's a simpler example one could construct. * Make sure Alsa and libllama.so are installed (on Arch I installed alsa-lib and llama.cpp-cuda). * Clone llama-cpp-2 & init llama.cpp submodule & update the submodule to point to https://github.com/ggml-org/llama.cpp/pull/11997 instead. * Add plumbing to expose the new method within llama-cpp-2 as a public facing function on the LlamaModel struct (it's basically the same code as for n_head, just calling n_head_kv from llama.cpp). * Add cpal as a dependency in crate "foo" * Add llama-cpp-2 via path as a dependency in crate "foo" and enable the `dynamic-link` feature. * Add code using the newly expose n_head_kv method in crate "foo" in main.rs. NOTE: Code just needs to compile & be exported, doesn't have to be correct (fn main is probably easiest. * Add some basic code that tries to initialize cpal in crate "foo" in fn main. * Try to build / run crate "foo" Before my change, it fails with a linker error saying it can't find `llama_model_n_head_kv` because /usr/lib appears in the search path before the directory that contains the libllama.so that was built internally by the crate. This is because cpal depends on alsa-sys which uses pkg-config which adds /usr/lib to the search path before the llama-cpp-sys-2 build.rs is run. ### Additional information I'm not sure how to add tests so open to some help on that. I wanted to make sure that this approach is even correct. I coded this to change Cargo minimally and defensively since I don't know the internals of Cargo very well (e.g. I don't know if I have to compare against both `script_out_dir` / `script_out_dir_when_generated` since I don't know the difference & there's not really any explanation on what they are). It's possible this over-complicates the implementation so open to any feedback. Additionally, the sort that happens prior to each build up of the rustc environment is not where I'd ideally place it. I think it would be more efficient to have the list of search paths be free-floating and not tied to a BuildOutput so that they could be kept updated live & resorted only on insertion (since it's changed less frequently than rustc is invoked). Additionally, the generalized sort is correct but pessimistic - maintaining the list sorted could be done efficiently with some minor book keeping (i.e. you'd only need to sort the new paths & then could quickly inject into the middle of a VecDeque). And of course in terms of correctness, I didn't do a thorough job testing across all possible platforms. From first principles this seems directionally correct but it's always possible this breaks someone else's workflow. I'm also uneasy that the relative position of `-L` / `-l` arguments changes in this PR & I'm not sure if that's observable behavior or not (i.e. it used to be -L for a crate followed by `-l` for a crate), but now it's `-L` for all crates, still grouped by crated internally, followed by `-l` by crate).
This commit is contained in:
Weihang Lo
GitHub
commit
ef7d315894
@ -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)
|
||||
|
||||
@ -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;
|
||||
@ -75,11 +75,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<PathBuf> 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<PathBuf>,
|
||||
pub library_paths: Vec<LibraryPath>,
|
||||
/// Names and link kinds of libraries, suitable for the `-l` flag.
|
||||
pub library_links: Vec<String>,
|
||||
/// Linker arguments suitable to be passed to `-C link-arg=<args>`
|
||||
@ -239,7 +304,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::<Vec<_>>();
|
||||
|
||||
let msg = machine_message::BuildScript {
|
||||
@ -886,10 +951,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((
|
||||
|
||||
@ -79,7 +79,7 @@ pub use self::compilation::{Compilation, Doctest, UnitOutput};
|
||||
pub use self::compile_kind::{CompileKind, CompileKindFallback, 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};
|
||||
@ -497,6 +497,32 @@ fn rustc(
|
||||
current_id: PackageId,
|
||||
mode: CompileMode,
|
||||
) -> 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!(
|
||||
@ -504,9 +530,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 {
|
||||
@ -654,7 +677,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,
|
||||
));
|
||||
}
|
||||
@ -663,6 +686,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
|
||||
@ -674,12 +704,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 \
|
||||
|
||||
@ -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)?;
|
||||
|
||||
@ -6084,3 +6084,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 [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();
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user