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

cuviper · 2024-04-11T00:36:20Z

When the build is configured with [rust] rpath = false, we need to set
LD_LIBRARY_PATH (or equivalent) to what would have been the RPATH,
so the compiler can find its own libraries. The old tools.mk code has
this environment prefixed in the $(BARE_RUSTC) variable, so we just
need to wire up something similar for run-make v2.

This is now set while building each rmake.rs itself, as well as in the
rust-make-support helpers for rustc and rustdoc commands. This is
also available in a set_host_rpath function for manual commands, like
in the compiler-builtins test.

When the build is configured with `[rust] rpath = false`, we need to set `LD_LIBRARY_PATH` (or equivalent) to what would have been the `RPATH`, so the compiler can find its own libraries. The old `tools.mk` code has this environment prefixed in the `$(BARE_RUSTC)` variable, so we just need to wire up something similar for run-make v2. This is now set while building each `rmake.rs` itself, as well as in the `rust-make-support` helpers for `rustc` and `rustdoc` commands. This is also available in a `set_host_rpath` function for manual commands, like in the `compiler-builtins` test.

rustbot · 2024-04-11T00:36:28Z

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-04-11T00:36:30Z

Some changes occurred in src/tools/compiletest

cc @jieyouxu

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in run-make tests.

cc @jieyouxu

jieyouxu

Thanks for the fix, I believe this might also be the more general fix for run-make v2 tests breaking on sgx (cf. #123100) 💚

Feel free to r=me after fixing the PATH joining and splitting issue (which is more than likely entirely my fault for misleading 😆)

jieyouxu · 2024-04-11T15:47:41Z

src/tools/compiletest/src/runtest.rs

+        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());


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.

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 😆)

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... 😅

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) 😆

~~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.

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

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

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

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.

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

jieyouxu · 2024-04-11T16:08:11Z

Also maybe edit PR CI to run i686-msvc and test-various after the PATH splitting/joining fix, so we can check if the changes might cause failures on Windows / WASM (it shouldn't after fixing the PATH splitting/joining), but it's better to bounce in PR CI than in full build CI.

cuviper · 2024-04-12T00:17:06Z

Also maybe edit PR CI to run i686-msvc and test-various after the PATH splitting/joining fix, so we can check if the changes might cause failures on Windows / WASM (it shouldn't after fixing the PATH splitting/joining), but it's better to bounce in PR CI than in full build CI.

All clear!
https://github.com/rust-lang/rust/actions/runs/8654154411

jieyouxu · 2024-04-12T00:26:02Z

Thanks!
@bors r+ rollup

bors · 2024-04-12T00:26:04Z

📌 Commit 7e171c7 has been approved by jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#123599 (remove some things that do not need to be) - rust-lang#123763 (Set the host library path in run-make v2) - rust-lang#123775 (Make `PlaceRef` and `OperandValue::Ref` share a common `PlaceValue` type) - rust-lang#123789 (move QueryKeyStringCache from rustc_middle to rustc_query_impl, where it actually used) - rust-lang#123826 (Move rare overflow error to a cold function) - rust-lang#123827 (linker: Avoid some allocations in search directory iteration) - rust-lang#123829 (Fix revisions syntax in cfg(ub_checks) test) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#123763 - cuviper:host-rpath-run-make-v2, r=jieyouxu Set the host library path in run-make v2 When the build is configured with `[rust] rpath = false`, we need to set `LD_LIBRARY_PATH` (or equivalent) to what would have been the `RPATH`, so the compiler can find its own libraries. The old `tools.mk` code has this environment prefixed in the `$(BARE_RUSTC)` variable, so we just need to wire up something similar for run-make v2. This is now set while building each `rmake.rs` itself, as well as in the `rust-make-support` helpers for `rustc` and `rustdoc` commands. This is also available in a `set_host_rpath` function for manual commands, like in the `compiler-builtins` test.

rustbot assigned jieyouxu Apr 11, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Apr 11, 2024

jieyouxu approved these changes Apr 11, 2024

View reviewed changes

Use env::split_paths/join_paths in runtest

7e171c7

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Apr 11, 2024

cuviper force-pushed the host-rpath-run-make-v2 branch from 28b7bdf to 7e171c7 Compare April 12, 2024 00:17

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 12, 2024

matthiaskrgr mentioned this pull request Apr 12, 2024

Rollup of 7 pull requests #123838

Merged

bors merged commit a510cbd into rust-lang:master Apr 12, 2024

rustbot added this to the 1.79.0 milestone Apr 12, 2024

aditijannu mentioned this pull request Apr 15, 2024

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

Closed

cuviper deleted the host-rpath-run-make-v2 branch April 21, 2024 18:28

	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());
	}

	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

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

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

Uh oh!

Conversation

cuviper commented Apr 11, 2024

Uh oh!

rustbot commented Apr 11, 2024

Uh oh!

rustbot commented Apr 11, 2024

Uh oh!

jieyouxu left a comment

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

cuviper Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jieyouxu Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jieyouxu commented Apr 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Apr 12, 2024

Uh oh!

jieyouxu commented Apr 12, 2024

Uh oh!

bors commented Apr 12, 2024

Uh oh!

Uh oh!

jieyouxu Apr 11, 2024 •

edited

Loading

jieyouxu Apr 11, 2024 •

edited

Loading

jieyouxu Apr 11, 2024 •

edited

Loading

jieyouxu Apr 11, 2024 •

edited

Loading

jieyouxu Apr 11, 2024 •

edited

Loading

jieyouxu commented Apr 11, 2024 •

edited

Loading