fix: preserve jobserver file descriptors on rustc invocation to get TargetInfo
#12447
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @epage (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
A-cfg-expr
|
What cargo does when it wants to really run rustc (for compilation, with jobserver passed through properly) - changes CLOEXEC flags to false? Creates jobserver-rs client in some other way? Maybe it makes sense to do the same thing here for consistency, even if this rustc run is tiny. |
|
The |
|
Method |
There was a problem hiding this comment.
Thanks for working on jobserver so far!
The intent of this pull request is not immediately clear to me. What does this try to resolve in practice? People in the future may be confused and wonderwhy these envs were removed. I know there are links to discussions, but without a summary it's not easy to follow. Besides, assumed this quite important, I believe a test is pretty much wanted. Thus, I'd like to see something beyond code:
- A clear PR description of why this is bad and how it affects other programs.
- Why the solution is desired (remove envs) but not others (e.g. inherit jobserver).
- A test that verifies the “wrong” behavior, and one follow-up commit fixes it.
|
I've updated PR description.
This PR affects only rustc invocation, so I'm not sure, what do you mean.
I don't know, if this solution is better. It shouldn't change anything, since now jobserver isn't used and it won't be used with this PR. Also, according to @bjorn3 "That rustc call doesn't use jobserver", so passing jobserver won't change anything, so it's may be better to use simplest solution.
My local test is makefile, called One option to write such test is to use some wrapper over rustc, which would verify that passed jobserver-related env vars from cargo don't refer to nonexistent file descriptors, but imho it would be enough to have such check in rustc, which my PR to rustc is intended to add. |
At high level:
(Even if the check in rust-lang/rust#113730 is later made optional or relaxed due to incompatibilities in third-party tools that we cannot fix, with cargo we can indeed fix it so it's better to make it compatible.) I'd personally follow #12447 (comment) and |
belovdv
force-pushed
the
jobserver-remove-env-var
branch
from
8a97ce2
to
adcbca4
Compare
|
To fix CI, rebase onto master. See #12450 |
belovdv
force-pushed
the
jobserver-remove-env-var
branch
from
adcbca4
to
e1a4c19
Compare
|
Branch rebased, thanks.
About test - should I add one, or it'd be enough to have check in rustc? |
belovdv
changed the title
TargetInfoTargetInfo
belovdv
force-pushed
the
jobserver-remove-env-var
branch
from
e1a4c19
to
1e12538
Compare
|
Thanks! @bors r+ |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Update cargo 21 commits in d78bbf4bde3c6b95caca7512f537c6f9721426ff..7e9de3f4ec3708f500bec142317895b96131e47c 2023-08-03 12:58:25 +0000 to 2023-08-13 00:47:32 +0000 - feat: remove `--keep-going` from `cargo test/bench` (rust-lang/cargo#12478) - chore: window-sys should be a platform-specific dependency (rust-lang/cargo#12483) - docs: make the env var source of rerun-if-env-changed clearer (rust-lang/cargo#12482) - doc: note the backward compatible `.cargo/credential` file exists (rust-lang/cargo#12479) - Fix elided lifetime in associated const (rust-lang/cargo#12475) - prompt the use of `--nocapture` flag if `cargo test` process is terminated via a signal. (rust-lang/cargo#12463) - cargo-credential: reset stdin & stdout to the Console (rust-lang/cargo#12469) - Fix cargo remove incorrectly removing used patches (rust-lang/cargo#12454) - chore(gh): Expand update window (rust-lang/cargo#12466) - Fix panic when enabling http.debug for certain strings (rust-lang/cargo#12468) - fix(cli): Make `--help` easier to browse (rust-lang/cargo#11905) - fix: preserve jobserver file descriptors on rustc invocation to get `TargetInfo` (rust-lang/cargo#12447) - refactor: migrate to `tracing` (rust-lang/cargo#12458) - docs: add example for cargo-credential (rust-lang/cargo#12461) - Bail out an error when using cargo:: in custom build script (rust-lang/cargo#12332) - Fix printing multiple warning messages for unused fields in [registries] table (rust-lang/cargo#12439) - Update windows dependencies (rust-lang/cargo#12453) - Rustfmt a let-else statement (rust-lang/cargo#12451) - Add allow(internal_features) (rust-lang/cargo#12450) - Update pretty_env_logger to 0.5 (rust-lang/cargo#12445) - Remove build metadata from libgit2-sys dependency (rust-lang/cargo#12444) r? `@ghost`
fix: preserve jobserver file descriptors on rustc invocation in `fix_exec_rustc` Similar to #12447 Command `cargo fix` invokes `cargo rustc`. It sends environment variable which points to closed file descriptors. This PR makes cargo compatible with rust-lang/rust#113730. It should be enough to have tests in rustc. It seems to be good to have more centralized way to pass jobserver.
Then cargo asks rustc for
TargetInfo, cargo passes environment variables that describe jobserver file descriptors, but doesn't actually passed them, becausejobserver-rssetsFD_CLOEXECby default and expects users to explicitly callClient::configureto pass them further. Rustc respects make jobserver, so this causes error - env vars refers to nonexistent file descriptors, which is ignored now.This PR makes cargo compatible with rust-lang/rust#113730, which attempts to catch such situations.