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.

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

Migrate jobserver-error run-make test to rmake #128789

Closed
wants to merge 3 commits into from

Conversation

Oneirical
Copy link
Contributor

Part of #121876 and the associated Google Summer of Code project.

In a similar fashion to #128460, this test contains peculiar interactions with the /dev/ directory and piping to file descriptors. Note the massive commented section, which requires figuring out how to pipe /dev/null/ to file descriptor 3 to get this to work.

Will need try jobs, but not yet.

@rustbot
Copy link
Collaborator

rustbot commented Aug 7, 2024

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 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 Aug 7, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@Oneirical Oneirical marked this pull request as ready for review September 15, 2024 16:45
@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

The run-make-support library was changed

cc @jieyouxu

@bors
Copy link
Contributor

bors commented Oct 25, 2024

☔ The latest upstream changes (presumably #131917) made this pull request unmergeable. Please resolve the merge conflicts.

@jieyouxu
Copy link
Member

jieyouxu commented Jan 3, 2025

FWIW, we might actually just delete the disabled part of this test cc #110321.

The problem is that the jobserver helper thread launched here gets starved out and doesn't run, while the coordinator thread continually processes work using the implicit jobserver token, never yielding long enough for the jobserver helper to do its work (and process the error).

Honestly, I'm not sure this is really worth fixing. It would be nice to have a test to validate the jobserver behavior, but I'm not sure it is worth the expense of changing the coordinator's behavior in some way (since there could be performance losses, for example if it explicitly yielded or waited for the jobserver helper, or more aggressively tried to acquire tokens). I'm not familiar enough with Linux thread scheduling to know why the thread could get blocked for so long, or if there are different approaches to fix the scheduling.

Another thought I just had would be to change the test to generate a binary with enough code to split across a very large number of CGUs (and use -C codegen-units=…). That would ensure that the coordinator does a lot of work, giving it more opportunity to yield. Again, I'm not too familiar with Linux thread scheduling, so I don't know if that would help, but might be worth a shot.

@jieyouxu
Copy link
Member

Thanks for the PR, this is superseded by #135461 (with attribution to both authors).

@jieyouxu jieyouxu closed this Jan 13, 2025
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 13, 2025
…=<try>

tests: Port `jobserver-error.rs` to rmake.rs

Part of rust-lang#121876.

This PR ports `jobserver-error.rs` to rmake.rs, and is basically rust-lang#128789 slightly adjusted. Namely, `set_aux_fd` is made `unsafe`, alongside some doc updates.

The complexity involved here is mostly how to get `/dev/null/` piping to fd 3 working with std `Command`, whereas with a shell this is much easier (as is evident with the `Makefile` version).

Supersedes rust-lang#128789.
This PR is co-authored with `@Oneirical` and `@coolreader18.`

r? `@ghost`

try-job: aarch64-gnu
try-job: i686-gnu-1
try-job: x86_64-gnu-debug
try-job: x86_64-gnu-llvm-18-1
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 14, 2025
…=<try>

tests: Port `jobserver-error` to rmake.rs

Part of rust-lang#121876.

This PR ports `tests/run-make/jobserver-error` to rmake.rs, and is basically rust-lang#128789 slightly adjusted. Namely, `set_aux_fd` is made `unsafe`, alongside some doc updates.

The complexity involved here is mostly how to get `/dev/null/` piping to fd 3 working with std `Command`, whereas with a shell this is much easier (as is evident with the `Makefile` version).

Supersedes rust-lang#128789.
This PR is co-authored with `@Oneirical` and `@coolreader18.`

r? `@ghost`

try-job: aarch64-gnu
try-job: i686-gnu-1
try-job: x86_64-gnu-debug
try-job: x86_64-gnu-llvm-18-1
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 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)
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants