Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure search paths inside OUT_DIR precede external paths #15221

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
100 changes: 96 additions & 4 deletions src/cargo/core/compiler/custom_build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -46,6 +46,7 @@ use cargo_util::paths;
use cargo_util_schemas::manifest::RustVersion;
use std::collections::hash_map::{Entry, HashMap};
use std::collections::{BTreeSet, HashSet};
use std::ffi::OsStr;
use std::path::{Path, PathBuf};
use std::str::{self, FromStr};
use std::sync::{Arc, Mutex};
Expand Down Expand Up @@ -74,11 +75,96 @@ pub enum Severity {

pub type LogMessage = (Severity, String);

/// This 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.
Comment on lines +78 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: The first line of a doc comment should a summary, followed by a blank linke. And then the detailed doc.

See rustdoc book

Suggested change
/// This 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.
/// 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
/// it's 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) -> std::path::PathBuf {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already imported.

Suggested change
pub fn into_path_buf(self) -> std::path::PathBuf {
pub fn into_path_buf(self) -> PathBuf {

match self {
LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p,
}
}

pub fn display(&self) -> std::path::Display<'_> {
match self {
LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p.display(),
}
}
}

impl AsRef<Path> for LibraryPath {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel like we only need AsRef<PathBuf>?

diff --git a/src/cargo/core/compiler/custom_build.rs b/src/cargo/core/compiler/custom_build.rs
index e1e7cc49977..75d553c1db3 100644
--- a/src/cargo/core/compiler/custom_build.rs
+++ b/src/cargo/core/compiler/custom_build.rs
@@ -128,20 +128,6 @@ impl LibraryPath {
             LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p,
         }
     }
-
-    pub fn display(&self) -> std::path::Display<'_> {
-        match self {
-            LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p.display(),
-        }
-    }
-}
-
-impl AsRef<Path> for LibraryPath {
-    fn as_ref(&self) -> &Path {
-        match self {
-            LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p,
-        }
-    }
 }
 
 impl AsRef<PathBuf> for LibraryPath {
@@ -152,14 +138,6 @@ impl AsRef<PathBuf> for LibraryPath {
     }
 }
 
-impl AsRef<OsStr> for LibraryPath {
-    fn as_ref(&self) -> &OsStr {
-        match self {
-            LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p.as_os_str(),
-        }
-    }
-}
-
 /// Contains the parsed output of a custom build script.
 #[derive(Clone, Debug, Hash, Default, PartialEq, Eq, PartialOrd, Ord)]
 pub struct BuildOutput {
@@ -323,7 +301,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 {
diff --git a/src/cargo/core/compiler/mod.rs b/src/cargo/core/compiler/mod.rs
index 73597978880..74f964c723d 100644
--- a/src/cargo/core/compiler/mod.rs
+++ b/src/cargo/core/compiler/mod.rs
@@ -518,7 +518,7 @@ fn rustc(
         });
 
         for path in library_paths.iter() {
-            rustc.arg("-L").arg(path);
+            rustc.arg("-L").arg(path.as_ref());
         }
 
         for key in build_scripts.to_link.iter() {

fn as_ref(&self) -> &Path {
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,
}
}
}

impl AsRef<OsStr> for LibraryPath {
fn as_ref(&self) -> &OsStr {
match self {
LibraryPath::CargoArtifact(p) | LibraryPath::External(p) => p.as_os_str(),
}
}
}

/// 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>`
Expand Down Expand Up @@ -884,10 +970,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((
Expand Down
45 changes: 36 additions & 9 deletions src/cargo/core/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -495,16 +495,39 @@ 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);
}

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
))
})?;
for path in output.library_paths.iter() {
rustc.arg("-L").arg(path);
}

if key.0 == current_id {
if pass_l_flag {
Expand Down Expand Up @@ -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,
));
}
Expand All @@ -659,6 +682,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
Expand All @@ -670,10 +700,7 @@ 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).to_path_buf();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: to_path_buf can be moved to line 705 so we don't pay allocation cost if not needed.

if dir.starts_with(&root_output) {
search_path.push(dir);
} else {
Expand Down
14 changes: 9 additions & 5 deletions src/cargo/util/context/target.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -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" => {
Expand All @@ -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)?;
Expand Down
37 changes: 37 additions & 0 deletions tests/testsuite/build_script.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6087,3 +6087,40 @@ fn directory_with_leading_underscore() {
.with_status(0)
.run();
}

#[cargo_test]
fn linker_search_path_preference() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks nice. Thank you!

Could we have one more test that: Has some dependencies, and those dependencies also emit cargo::rustc-link-search, and we ensure that the order from the dependencies is maintained?

// 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"
"#,
)
.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() {}")
.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 /usr/lib -L /lib`
...
"#]]).run();
}