From b93c19e5ccf2ed31c9fdc393f5659a15a24cd952 Mon Sep 17 00:00:00 2001 From: Sage Griffin Date: Wed, 9 Mar 2022 11:53:04 -0700 Subject: [PATCH 1/5] Properly set dylib search path for doctests Fixes #8531. This issue was introduced by 3fd28143de72d2d0e63186273d68aa890d8297e7. Prior to that commit, the `rustdoc_process` function took other branch of the if statement being modified by this commit. The flag was previously called `is_host`, and `rustdoc` was run reporting `false`. In that commit, the flag was changed to `is_rustc_tool`, and rustdoc began reporting `true`, which resulted in the native directories added by build scripts no longer being appended to LD_LIBRARY_PATH when running doctests. This commit changes the behavior so that we are always appending the same set of paths, and the only variance is if we are cross compiling or not. An alternative would be to change rustdoc to always pass `kind: CompileKind::Host, is_rustc_tool: false`, but as rustdoc is in fact a rustc tool, that feels wrong. The commit which introduced the bug did not include context on why the flag was changed to `is_rustc_tool`, so I don't have a sense for if we should just change that flag to mean something else. While the test suite previously had an explicit test for the library path behavior of `cargo run`, it did not test this for various test forms. This commit modifies the test to ensure the behavior is consistent across `run`, normal tests, and doctests. --- src/cargo/core/compiler/compilation.rs | 4 ++++ tests/testsuite/run.rs | 21 ++++++++++++++++++++- 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index e03a08d3947..e141f82fd42 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -275,6 +275,10 @@ impl<'cfg> Compilation<'cfg> { ) -> CargoResult { let mut search_path = Vec::new(); if is_rustc_tool { + search_path.extend(super::filter_dynamic_search_path( + self.native_dirs.iter(), + &self.root_output[&CompileKind::Host], + )); search_path.push(self.deps_output[&CompileKind::Host].clone()); search_path.push(self.sysroot_host_libdir.clone()); } else { diff --git a/tests/testsuite/run.rs b/tests/testsuite/run.rs index 64cf4e16c6b..307b6eb5d95 100644 --- a/tests/testsuite/run.rs +++ b/tests/testsuite/run.rs @@ -1175,9 +1175,27 @@ fn run_with_library_paths() { ) .file( "src/main.rs", + r##" + extern crate foo; + + fn main() { + foo::assert_search_path(); + } + "##, + ) + .file( + "src/lib.rs", &format!( r##" - fn main() {{ + #[test] + fn test_search_path() {{ + assert_search_path(); + }} + + /// ``` + /// foo::assert_search_path(); + /// ``` + pub fn assert_search_path() {{ let search_path = std::env::var_os("{}").unwrap(); let paths = std::env::split_paths(&search_path).collect::>(); println!("{{:#?}}", paths); @@ -1193,6 +1211,7 @@ fn run_with_library_paths() { .build(); p.cargo("run").run(); + p.cargo("test").run(); } #[cargo_test] From e7f7058c42efaa1b84b0f7c7b97ea6c45c4a016a Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Tue, 19 Sep 2023 22:06:03 -0400 Subject: [PATCH 2/5] test for doctest path problem #8531, and solution * add test reproducing #8531 * re-use solution from #10469 * solution fixes test --- src/cargo/core/compiler/compilation.rs | 1 + tests/testsuite/test.rs | 32 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index e141f82fd42..946c31cccf4 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -274,6 +274,7 @@ impl<'cfg> Compilation<'cfg> { is_rustc_tool: bool, ) -> CargoResult { let mut search_path = Vec::new(); + // this divides our path based on CompileKind: host vs target. if is_rustc_tool { search_path.extend(super::filter_dynamic_search_path( self.native_dirs.iter(), diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index 5f6528109be..f38faf8bde9 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -2,6 +2,7 @@ use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::Package; +use cargo_test_support::Project; use cargo_test_support::{ basic_bin_manifest, basic_lib_manifest, basic_manifest, cargo_exe, project, }; @@ -48,6 +49,37 @@ fn cargo_test_simple() { .run(); } +#[cargo_test] +// download a known broken doctests code base and run cargo test, should fail +fn cargo_test_tensorflow_not_failing_due_to_shared_library() { + use git2::build::RepoBuilder; + + // git repo for cargo project containing doctests that require shared libraries + let url = "https://github.com/tensorflow/rust"; + + // safe place to git clone into + let path = tempfile::tempdir().unwrap(); + + // do the git clone + let _repo = RepoBuilder::new().clone(&url, &path.path()).unwrap(); + + // pathbuf to the resulting local cloned repo + let path_copy = path.path().to_path_buf(); + + // removing the .git directory, because it causes errors when copying in from_template() + std::fs::remove_dir_all(path_copy.clone().join(".git")).unwrap(); + + // now we have a handle on a project we can use + let project = Project::from_template(path_copy.clone()); + + // doctests are failing.. + project + .cargo("test --doc") + .with_stdout_does_not_contain("error while loading shared libraries") + .with_status(101) // has a single failing test, different problem. + .run(); +} + #[cargo_test] fn cargo_test_release() { let p = project() From 7f4704da55fda28d68eb219d4b3d0469675351fb Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Tue, 19 Sep 2023 22:44:38 -0400 Subject: [PATCH 3/5] fmt --- tests/testsuite/test.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index f38faf8bde9..4a63747dd45 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -56,13 +56,13 @@ fn cargo_test_tensorflow_not_failing_due_to_shared_library() { // git repo for cargo project containing doctests that require shared libraries let url = "https://github.com/tensorflow/rust"; - + // safe place to git clone into let path = tempfile::tempdir().unwrap(); // do the git clone let _repo = RepoBuilder::new().clone(&url, &path.path()).unwrap(); - + // pathbuf to the resulting local cloned repo let path_copy = path.path().to_path_buf(); @@ -72,7 +72,7 @@ fn cargo_test_tensorflow_not_failing_due_to_shared_library() { // now we have a handle on a project we can use let project = Project::from_template(path_copy.clone()); - // doctests are failing.. + // doctests are failing.. project .cargo("test --doc") .with_stdout_does_not_contain("error while loading shared libraries") From 3c2557e5d6acd7b804877dda5643ca0fc626088e Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Sun, 24 Sep 2023 14:49:02 -0400 Subject: [PATCH 4/5] remove test replicating error --- tests/testsuite/test.rs | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/tests/testsuite/test.rs b/tests/testsuite/test.rs index 4a63747dd45..5f6528109be 100644 --- a/tests/testsuite/test.rs +++ b/tests/testsuite/test.rs @@ -2,7 +2,6 @@ use cargo_test_support::paths::CargoPathExt; use cargo_test_support::registry::Package; -use cargo_test_support::Project; use cargo_test_support::{ basic_bin_manifest, basic_lib_manifest, basic_manifest, cargo_exe, project, }; @@ -49,37 +48,6 @@ fn cargo_test_simple() { .run(); } -#[cargo_test] -// download a known broken doctests code base and run cargo test, should fail -fn cargo_test_tensorflow_not_failing_due_to_shared_library() { - use git2::build::RepoBuilder; - - // git repo for cargo project containing doctests that require shared libraries - let url = "https://github.com/tensorflow/rust"; - - // safe place to git clone into - let path = tempfile::tempdir().unwrap(); - - // do the git clone - let _repo = RepoBuilder::new().clone(&url, &path.path()).unwrap(); - - // pathbuf to the resulting local cloned repo - let path_copy = path.path().to_path_buf(); - - // removing the .git directory, because it causes errors when copying in from_template() - std::fs::remove_dir_all(path_copy.clone().join(".git")).unwrap(); - - // now we have a handle on a project we can use - let project = Project::from_template(path_copy.clone()); - - // doctests are failing.. - project - .cargo("test --doc") - .with_stdout_does_not_contain("error while loading shared libraries") - .with_status(101) // has a single failing test, different problem. - .run(); -} - #[cargo_test] fn cargo_test_release() { let p = project() From 394aa9d11a971cb52cd1683705215be194a1335f Mon Sep 17 00:00:00 2001 From: Tom Scanlan Date: Thu, 8 Feb 2024 07:32:42 -0500 Subject: [PATCH 5/5] update comment with input --- src/cargo/core/compiler/compilation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cargo/core/compiler/compilation.rs b/src/cargo/core/compiler/compilation.rs index 946c31cccf4..59f583c348a 100644 --- a/src/cargo/core/compiler/compilation.rs +++ b/src/cargo/core/compiler/compilation.rs @@ -274,7 +274,7 @@ impl<'cfg> Compilation<'cfg> { is_rustc_tool: bool, ) -> CargoResult { let mut search_path = Vec::new(); - // this divides our path based on CompileKind: host vs target. + // this divides rustc/rustdoc invocations and other executions of build artifacts if is_rustc_tool { search_path.extend(super::filter_dynamic_search_path( self.native_dirs.iter(),