Make sure search paths inside OUT_DIR precede external paths #15221
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ehuss (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
|
Not sure what the CI failure for "check-version-bump" is but I'm assuming it's some minor book keeping that's validating a version number somewhere needs to be bumped because I made a consequential change? But the output when running locally doesn't make sense to me: I haven't touched |
|
That CI job failure is completely unrelated to this PR. See #15222. |
|
Just note that it is generally recommended an issue to be discussed and labeled as https://doc.crates.io/contrib/process/working-on-cargo.html#before-hacking-on-cargo |
|
Good note but I also wanted to see how difficult such a change would be to help guide any questions that might come up in the issue. |
Will need it in a follow-up PR
9bdda46 to
c8d12c5
Compare
|
I think I've resolved all the outstanding review requests. Please let me know if there's any other concerns. I did add a comment to the LibraryPath documentation that may be worth discussing (but also OK just leaving this for future consideration): @rustbot review |
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.
c8d12c5 to
97bc22f
Compare
| } | ||
| } | ||
|
|
||
| pub fn into_path_buf(self) -> std::path::PathBuf { |
There was a problem hiding this comment.
Already imported.
| pub fn into_path_buf(self) -> std::path::PathBuf { | |
| pub fn into_path_buf(self) -> PathBuf { |
| Some(("native" | "crate" | "dependency" | "framework" | "all", path)) => path.into(), | ||
| _ => dir.clone(), | ||
| }; | ||
| let dir = get_dynamic_search_path(dir).to_path_buf(); |
There was a problem hiding this comment.
nit: to_path_buf can be moved to line 705 so we don't pay allocation cost if not needed.
| /// 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. |
There was a problem hiding this comment.
nit: The first line of a doc comment should a summary, followed by a blank linke. And then the detailed doc.
See rustdoc book
| /// 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. |
| } | ||
| } | ||
|
|
||
| impl AsRef<Path> for LibraryPath { |
There was a problem hiding this comment.
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() {
| @@ -6087,3 +6087,40 @@ fn directory_with_leading_underscore() { | |||
| .with_status(0) | |||
| .run(); | |||
| } | |||
|
|
|||
| #[cargo_test] | |||
| fn linker_search_path_preference() { | |||
There was a problem hiding this comment.
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?
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.
dynamic-linkfeature.Before my change, it fails with a linker error saying it can't find
llama_model_n_head_kvbecause /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_generatedsince 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/-larguments 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-lfor a crate), but now it's-Lfor all crates, still grouped by crated internally, followed by-lby crate).