Skip to content

Fix broken-pipe-no-ice run-make test for rpath-less builds #140744

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jchecahi
Copy link

@jchecahi jchecahi commented May 7, 2025

tests/run-make/broken-pipe-no-ice currently fails to run when rpath is disabled in bootstrap config, as the rustc and rustdoc binaries invoked cannot find the required shared libs (librustc_driver.so).

This commit manually sets the dylib search path for both commands. The logic mirrors what's implemented in run_make_support::util::set_host_compiler_dylib_path, ensuring that the host compiler’s shared libraries are available at runtime even when rpath is disabled.

This is necessary because the test manually constructs the rustc/rustdoc Command instances instead of using the run_make_support wrappers, and would otherwise lack the dynamic linker environment setup.

Fixes part of #140738

tests/run-make/broken-pipe-no-ice currently fails to run when rpath is disabled
in bootstrap config, as the rustc and rustdoc binaries invoked cannot find the
required shared libs (librustc_driver.so).

This commit manually sets the dylib search path for both commands. The logic
mirrors what's implemented in run_make_support::util::set_host_compiler_dylib_path,
ensuring that the host compiler’s shared libraries are available at runtime even
when rpath is disabled.

This is necessary because the test manually constructs the rustc/rustdoc Command
instances instead of using the run_make_support wrappers, and would otherwise lack
the dynamic linker environment setup.

Fixes part of rust-lang#140738
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2025

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 rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2025

This PR modifies run-make tests.

cc @jieyouxu

@jieyouxu
Copy link
Member

jieyouxu commented May 7, 2025

I feel like I wrote the initial version of this test 🤔 AFAIK, this should just use the rustc() wrapper, but at the time I don't know why I didn't use rustc() 🤔 Can you try instead just using rustc() from r-m-s that already does the host path adjustments?

@jieyouxu
Copy link
Member

jieyouxu commented May 7, 2025

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 7, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 7, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@jchecahi
Copy link
Author

jchecahi commented May 7, 2025

I feel like I wrote the initial version of this test 🤔 AFAIK, this should just use the rustc() wrapper, but at the time I don't know why I didn't use rustc() 🤔 Can you try instead just using rustc() from r-m-s that already does the host path adjustments?

Hi @jieyouxu, thanks for reviewing the PR!

Actually, that was the first thing I tried 🙂 — I initially attempted to use the rustc() and rustdoc() wrappers from run-make-support directly. However, the test needs to manipulate the Command (e.g., to add arguments or pipe stdout), and since the Command field in those wrappers is private, it's not accessible from the test.

Because of that, I figured the test was intentionally written using a bare Command::new(...), and I mirrored the logic of set_host_compiler_dylib_path manually instead.

Happy to explore alternatives if there's a clean way to use the wrappers here!

@jieyouxu
Copy link
Member

jieyouxu commented May 7, 2025

D'oh right, that's why 😆

Can you try instead of reimplementing set_host_compiler_dylib_path:

  • Expose the set_host_compiler_dylib_path helper and make it available for run-make tests? That way, for the tests that need to set the host dylib manually (I expect this to be very rare), they don't have to duplicate that logic?
  • Add a comment to set_host_compiler_dylib_path that reminds the test writer that rustc()/rustdoc()/cargo() (after run-make-support: set rustc dylib path for cargo wrapper #140745 merges) will set this automatically, and in the general case it should not be used directly.

@jchecahi
Copy link
Author

jchecahi commented May 7, 2025

Right, I went ahead and tried exposing set_host_compiler_dylib_path, but ran into a type mismatch: the function expects a run_make_support::command::Command, whereas this test uses std::process::Command directly, since it needs access to stdout/stderr and to spawn() for the child process. I also tried using the run_make_support::run::cmd() wrapper, but that just moved the type mismatch error to the function, which still expects a std::process::Command. Moreover, the run_make_support::command module is private, so I couldn’t access it easily.

Maybe adapting set_host_compiler_dylib_path to work with both types could solve the issue, but honestly, it feels a bit messy just to fix this single test. I believe the manual logic, although not ideal, might still be the cleanest option in this case.

That being said, I’m happy to further discuss or explore any other suggestions you may have. Let me know what you think!

@jieyouxu
Copy link
Member

jieyouxu commented May 7, 2025

Let me play around a bit with this locally, I'll get back to u tmrw.

@jieyouxu
Copy link
Member

jieyouxu commented May 7, 2025

@rustbot review

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants