Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 2 commits into from
May 10, 2025

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented May 9, 2025

The broken-pipe-no-ice run-make test currently fails on rpath-less builds, because host compiler runtime libs are not configured for raw std command usages.

This PR is an alternative approach to #140744. However, instead of duplicating run_make_support::util::set_host_compiler_dylib_path logic, we instead support "ejecting" the "configured" underlying std Command from bare_rustc() and rustdoc(), where host compiler runtime libs are already set.

cc @jchecahi
r? @Kobzol

jieyouxu and others added 2 commits May 9, 2025 19:54
In rare cases, the test may need access to the underlying
`std::process::Command` (e.g. for non-trivial process spawning).

Co-authored-by: Jesus Checa Hidalgo <jchecahi@redhat.com>
Where host compiler runtime libs are properly configured, instead of raw
`RUSTC`/`RUSTDOC` commands.

Co-authored-by: Jesus Checa Hidalgo <jchecahi@redhat.com>
@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 9, 2025
Copy link
Contributor

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

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

Seems like another alternative would be to either implement DerefMut<Command> for our wrapper, or just implement the stdout/stderr/spawn functions? But most tests don't need this anyway, so the having the eject logic is also fine by me. Feel free to r=me.

@jieyouxu
Copy link
Member Author

jieyouxu commented May 9, 2025

Yeah I considered those options, but it seems really rare to need to implement those.

  • DerefMut<Command> I don't necessarily want to implement, because that practically makes the wrapper "transparent" (but we intentionally maintain separation as we have CompletedOutput wrapper types too).
  • stdout/stderr I think already exists, it's just spawn that doesn't, beacuse with spawn, you'll again get "unwrapped" raw std types.

@jieyouxu
Copy link
Member Author

jieyouxu commented May 9, 2025

Waiting a bit to hear back from @jchecahi in case this isn't sufficient for them. Locally, the test passes for me when I set rust.rpath = false.

@jchecahi
Copy link

jchecahi commented May 9, 2025

@jieyouxu yes, this approach fixes the test in my setup too. Thanks for looking into this!

@jieyouxu
Copy link
Member Author

jieyouxu commented May 9, 2025

@bors r=@Kobzol rollup

@bors
Copy link
Collaborator

bors commented May 9, 2025

📌 Commit 84ed40d has been approved by Kobzol

It is now in the queue for this repository.

@bors 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 May 9, 2025
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 9, 2025
Fix `broken-pipe-no-ice` run-make test for rpath-less builds

The `broken-pipe-no-ice` run-make test currently fails on rpath-less builds, because host compiler runtime libs are not configured for raw std command usages.

This PR is an alternative approach to rust-lang#140744. However, instead of duplicating `run_make_support::util::set_host_compiler_dylib_path` logic, we instead support "ejecting" the "configured" underlying std `Command` from `bare_rustc()` and `rustdoc()`, where host compiler runtime libs are already set.

cc ``@jchecahi``
r? ``@Kobzol``
bors added a commit to rust-lang-ci/rust that referenced this pull request May 9, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#139863 (rustdoc: Replace unstable flag `--doctest-compilation-args` with a simpler one: `--doctest-build-arg`)
 - rust-lang#140815 (also export metrics from librustdoc)
 - rust-lang#140819 (Add regression test for 125877)
 - rust-lang#140843 (Fix `broken-pipe-no-ice` run-make test for rpath-less builds)
 - rust-lang#140848 (Improved error message for top-level or-patterns)
 - rust-lang#140852 (Update the edition guide for let chains)
 - rust-lang#140864 (Last minute relnotes fix)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f9003b7 into rust-lang:master May 10, 2025
6 checks passed
@rustbot rustbot added this to the 1.89.0 milestone May 10, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 10, 2025
Rollup merge of rust-lang#140843 - jieyouxu:broken-pipe, r=Kobzol

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

The `broken-pipe-no-ice` run-make test currently fails on rpath-less builds, because host compiler runtime libs are not configured for raw std command usages.

This PR is an alternative approach to rust-lang#140744. However, instead of duplicating `run_make_support::util::set_host_compiler_dylib_path` logic, we instead support "ejecting" the "configured" underlying std `Command` from `bare_rustc()` and `rustdoc()`, where host compiler runtime libs are already set.

cc `@jchecahi`
r? `@Kobzol`
@jieyouxu jieyouxu deleted the broken-pipe branch May 10, 2025 06:47
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants