Skip to content

Set the host library path in run-make v2 #123763

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

Merged
merged 2 commits into from
Apr 12, 2024
Merged

Set the host library path in run-make v2 #123763

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a5409b
Set the host library path in run-make v2
cuviper Apr 11, 2024
7e171c7
Use `env::split_paths`/`join_paths` in runtest
cuviper Apr 11, 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
6 changes: 6 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 @@ -3785,6 +3785,11 @@ impl<'test> TestCx<'test> {
debug!(?support_lib_deps);
debug!(?support_lib_deps_deps);

let mut host_dylib_env_paths = String::new();
host_dylib_env_paths.push_str(&cwd.join(&self.config.compile_lib_path).to_string_lossy());
host_dylib_env_paths.push(':');
host_dylib_env_paths.push_str(&env::var(dylib_env_var()).unwrap());
Copy link
Member

@jieyouxu jieyouxu Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This might need to use std::env::join_paths because dylib_env_var() expands to PATH on Windows, but Windows uses ; for PATH separator unlike : for everyone else. std::env::join_paths should handle this properly for the host platform at least. Expanding dylib_env_var() into its constituent paths will also need to use std::env::split_paths, and then repiece individual paths together with std::env::join_paths.

Copy link
Member

@jieyouxu jieyouxu Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When I initially added the v2 infra, I was not aware std::env::join_paths existed and that Windows used ; separators for PATH not :, so there may be a couple more places where this bug exists that may have misled you into writing this (sorry 😆)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did wonder about that, but decided to leave it alone for the moment. I'll update these now.

Perhaps the is_windows() bit in run_make_support::run is also related? That seemed odd to me, but I don't want to touch Windows if it's working... 😅

Copy link
Member

Choose a reason for hiding this comment

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

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

Lines 15 to 34 in f13f37f

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()
});
if is_windows() {
let mut paths = vec![];
for p in env::split_paths(&std::env::var("PATH").unwrap_or(String::new())) {
paths.push(p.to_path_buf());
}
paths.push(Path::new(&std::env::var("TARGET_RPATH_DIR").unwrap()).to_path_buf());
cmd.env("PATH", env::join_paths(paths.iter()).unwrap());
}

This bit is almost completely wrong, I did not know better at the time when this was written (I realize this is terribly broken now) 😆

Copy link
Member

@jieyouxu jieyouxu Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'll put up a fix for this lol EDIT: actually, could you propagate the fix for run_make_support::run as well? The logic in there for the ld_lib_path_envvar() should just use set_host_rpath(). All of

 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() 
 }); 
  
 if is_windows() { 
     let mut paths = vec![]; 
     for p in env::split_paths(&std::env::var("PATH").unwrap_or(String::new())) { 
         paths.push(p.to_path_buf()); 
     } 
     paths.push(Path::new(&std::env::var("TARGET_RPATH_DIR").unwrap()).to_path_buf()); 
     cmd.env("PATH", env::join_paths(paths.iter()).unwrap()); 
 }

is just set_host_rpath() but actually wrong I think.

Copy link
Member

@jieyouxu jieyouxu Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh I remember why there was a is_windows(): you're right, but conditionally right...

RUN has different environments depending on platform:

rust/tests/run-make/tools.mk

Lines 85 to 116 in 616a8f8

ifeq ($(UNAME),Darwin)
RUN = $(TARGET_RPATH_ENV) $(EXECUTE)
FAIL = $(TARGET_RPATH_ENV) $(EXECUTE) && exit 1 || exit 0
DYLIB_GLOB = lib$(1)*.dylib
DYLIB = $(TMPDIR)/lib$(1).dylib
STATICLIB = $(TMPDIR)/lib$(1).a
STATICLIB_GLOB = lib$(1)*.a
else
ifdef IS_WINDOWS
RUN = PATH="$(PATH):$(TARGET_RPATH_DIR)" $(EXECUTE)
FAIL = PATH="$(PATH):$(TARGET_RPATH_DIR)" $(EXECUTE) && exit 1 || exit 0
DYLIB_GLOB = $(1)*.dll
DYLIB = $(TMPDIR)/$(1).dll
ifdef IS_MSVC
STATICLIB = $(TMPDIR)/$(1).lib
STATICLIB_GLOB = $(1)*.lib
else
IMPLIB = $(TMPDIR)/lib$(1).dll.a
STATICLIB = $(TMPDIR)/lib$(1).a
STATICLIB_GLOB = lib$(1)*.a
endif
BIN = $(1).exe
LLVM_FILECHECK := $(shell cygpath -u "$(LLVM_FILECHECK)")
else
RUN = $(TARGET_RPATH_ENV) $(EXECUTE)
FAIL = $(TARGET_RPATH_ENV) $(EXECUTE) && exit 1 || exit 0
DYLIB_GLOB = lib$(1)*.so
DYLIB = $(TMPDIR)/lib$(1).so
STATICLIB = $(TMPDIR)/lib$(1).a
STATICLIB_GLOB = lib$(1)*.a
endif
endif

Copy link
Member

Choose a reason for hiding this comment

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

e.g. on windows it's RUN = PATH="$(PATH):$(TARGET_RPATH_DIR)" $(EXECUTE)...

Copy link
Member Author

Choose a reason for hiding this comment

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

Safe to say, there's a lot of weird legacy there...

Copy link
Member

@jieyouxu jieyouxu Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think then it's fine to leave run() as is, I'll go back to fix run() in a follow up PR. I don't want to block this PR for a different bug.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #123832 to make sure I double check run_make_support::run


let mut cmd = Command::new(&self.config.rustc_path);
cmd.arg("-o")
.arg(&recipe_bin)
Expand All @@ -3801,6 +3806,7 @@ impl<'test> TestCx<'test> {
.env("RUSTC", cwd.join(&self.config.rustc_path))
.env("TMPDIR", &tmpdir)
.env("LD_LIB_PATH_ENVVAR", dylib_env_var())
.env(dylib_env_var(), &host_dylib_env_paths)
.env("HOST_RPATH_DIR", 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)
Expand Down
14 changes: 14 additions & 0 deletions src/tools/run-make-support/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,17 @@ fn handle_failed_output(cmd: &str, output: Output, caller_line_number: u32) -> !
eprintln!("=== STDERR ===\n{}\n\n", String::from_utf8(output.stderr).unwrap());
std::process::exit(1)
}

/// Set the runtime library path as needed for running the host rustc/rustdoc/etc.
pub fn set_host_rpath(cmd: &mut Command) {
let ld_lib_path_envvar = env::var("LD_LIB_PATH_ENVVAR").unwrap();
cmd.env(&ld_lib_path_envvar, {
let mut paths = vec![];
paths.push(PathBuf::from(env::var("TMPDIR").unwrap()));
paths.push(PathBuf::from(env::var("HOST_RPATH_DIR").unwrap()));
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()
});
}
3 changes: 2 additions & 1 deletion src/tools/run-make-support/src/rustc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::ffi::{OsStr, OsString};
use std::path::Path;
use std::process::{Command, Output};

use crate::{handle_failed_output, tmp_dir};
use crate::{handle_failed_output, set_host_rpath, tmp_dir};

/// Construct a new `rustc` invocation.
pub fn rustc() -> Rustc {
Expand All @@ -24,6 +24,7 @@ pub struct Rustc {
fn setup_common() -> Command {
let rustc = env::var("RUSTC").unwrap();
let mut cmd = Command::new(rustc);
set_host_rpath(&mut cmd);
cmd.arg("--out-dir").arg(tmp_dir()).arg("-L").arg(tmp_dir());
cmd
}
Expand Down
6 changes: 4 additions & 2 deletions src/tools/run-make-support/src/rustdoc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::env;
use std::path::Path;
use std::process::{Command, Output};

use crate::handle_failed_output;
use crate::{handle_failed_output, set_host_rpath};

/// Construct a plain `rustdoc` invocation with no flags set.
pub fn bare_rustdoc() -> Rustdoc {
Expand All @@ -21,7 +21,9 @@ pub struct Rustdoc {

fn setup_common() -> Command {
let rustdoc = env::var("RUSTDOC").unwrap();
Command::new(rustdoc)
let mut cmd = Command::new(rustdoc);
set_host_rpath(&mut cmd);
cmd
}

impl Rustdoc {
Expand Down
11 changes: 6 additions & 5 deletions tests/run-make/compiler-builtins/rmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ use run_make_support::object::read::Object;
use run_make_support::object::ObjectSection;
use run_make_support::object::ObjectSymbol;
use run_make_support::object::RelocationTarget;
use run_make_support::set_host_rpath;
use run_make_support::tmp_dir;
use std::collections::HashSet;

Expand All @@ -48,8 +49,8 @@ fn main() {
let path = std::env::var("PATH").unwrap();
let rustc = std::env::var("RUSTC").unwrap();
let bootstrap_cargo = std::env::var("BOOTSTRAP_CARGO").unwrap();
let status = std::process::Command::new(bootstrap_cargo)
.args([
let mut cmd = std::process::Command::new(bootstrap_cargo);
cmd.args([
"build",
"--manifest-path",
manifest_path.to_str().unwrap(),
Expand All @@ -62,10 +63,10 @@ fn main() {
.env("RUSTC", rustc)
.env("RUSTFLAGS", "-Copt-level=0 -Cdebug-assertions=yes")
.env("CARGO_TARGET_DIR", &target_dir)
.env("RUSTC_BOOTSTRAP", "1")
.status()
.unwrap();
.env("RUSTC_BOOTSTRAP", "1");
set_host_rpath(&mut cmd);

let status = cmd.status().unwrap();
assert!(status.success());

let rlibs_path = target_dir.join(target).join("debug").join("deps");
Expand Down