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.

Sign up for GitHub

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

Fix run make tests when it can't find dynamically linked librustc #123100

Closed
wants to merge 4 commits into from
Closed

Fix run make tests when it can't find dynamically linked librustc #123100

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7f3a62b
Fix run make tests when it can't find dynamically linked librustc_dri…
Mar 26, 2024
46e8d59
Merge branch 'master' of github.com:rust-lang/rust into aj/fix-sgx-ld…
Mar 26, 2024
9f9b4c1
Merge branch 'master' of github.com:rust-lang/rust into aj/fix-sgx-ld…
Apr 2, 2024
13224c0
Merge branch 'master' of github.com:rust-lang/rust into aj/fix-sgx-ld…
Apr 5, 2024
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
2 changes: 2 additions & 0 deletions src/tools/compiletest/src/runtest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3784,6 +3784,7 @@ impl<'test> TestCx<'test> {
.env("TMPDIR", &tmpdir)
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
.env("LD_LIBRARY_PATH", cwd.join(&self.config.compile_lib_path))
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I wonder if this is the right place to solve it. The original run-make specifies LD_LIBRARY_PATH via LD_LIB_PATH_ENVVAR (when that is the correct LD_LIB_PATH_ENVVAR for the target):

rust/src/tools/compiletest/src/runtest.rs

Line 3582 in 7f3a62b

.env("LD_LIB_PATH_ENVVAR", dylib_env_var())

In tools.mk, then LD_LIBRARY_PATH (which is the suitable LD_LIB_PATH_ENVVAR) is set to the correct paths

rust/tests/run-make/tools.mk

Lines 3 to 6 in 536606b

HOST_RPATH_ENV = \
$(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(HOST_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))"
TARGET_RPATH_ENV = \
$(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(TARGET_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))"

My guess is that

rust/src/tools/run-make-support/src/lib.rs

Lines 142 to 154 in 536606b

let ld_lib_path_envvar = env::var("LD_LIB_PATH_ENVVAR").unwrap();
let mut cmd = Command::new(bin_path);
cmd.env(&ld_lib_path_envvar, {
let mut paths = vec![];
paths.push(PathBuf::from(env::var("TMPDIR").unwrap()));
for p in env::split_paths(&env::var("TARGET_RPATH_ENV").unwrap()) {
paths.push(p.to_path_buf());
}
for p in env::split_paths(&env::var(&ld_lib_path_envvar).unwrap()) {
paths.push(p.to_path_buf());
}
env::join_paths(paths.iter()).unwrap()
});

is broken, but this should be the correct place to fix it. In particular, I think you'd just need to preserve the semantics of

 HOST_RPATH_ENV = \ 
     $(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(HOST_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))" 
 TARGET_RPATH_ENV = \ 
     $(LD_LIB_PATH_ENVVAR)="$(TMPDIR):$(TARGET_RPATH_DIR):$($(LD_LIB_PATH_ENVVAR))"

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I think "TARGET_RPATH_ENV" should instead be TARGET_RPATH_DIR? Can you check if that fixes the issue?

Copy link
Author

@aditijannu aditijannu Apr 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jieyouxu Updating TARGET_RPATH_ENV with TARGET_RAPTH_DIR doesn't work on sgx.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm nevermind then. I see the original run-make test also had this issue...

Copy link
Member

@jieyouxu jieyouxu Apr 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Actually, could you try changing

- for p in env::split_paths(&env::var("TARGET_RPATH_ENV").unwrap()) { 
-     paths.push(p.to_path_buf()); 
- }
+ for p in env::split_paths(&env::var("HOST_RPATH_DIR").unwrap()) { 
+     paths.push(p.to_path_buf()); 
+ }
+ for p in env::split_paths(&env::var("TARGET_RPATH_DIR").unwrap()) { 
+     paths.push(p.to_path_buf()); 
+ }

Judging from #113889, HOST_RPATH_DIR should have the paths you need (I hope)

Copy link
Member

Choose a reason for hiding this comment

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

Fixing this with something like this makes sense to me.

.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
.env("LLVM_COMPONENTS", &self.config.llvm_components)
// We for sure don't want these tests to run in parallel, so make
Expand Down Expand Up @@ -3838,6 +3839,7 @@ impl<'test> TestCx<'test> {
.env("RUSTC", cwd.join(&self.config.rustc_path))
.env("TMPDIR", &tmpdir)
.env("HOST_RPATH_DIR", cwd.join(&self.config.compile_lib_path))
.env("LD_LIBRARY_PATH", cwd.join(&self.config.compile_lib_path))
.env("TARGET_RPATH_DIR", cwd.join(&self.config.run_lib_path))
.env("LLVM_COMPONENTS", &self.config.llvm_components)
// We for sure don't want these tests to run in parallel, so make
Expand Down
Loading