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.

Sign up for GitHub

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

Jump to bottom

bootstrap: Forward cargo JSON output to stdout, not stderr #134123

Merged
merged 1 commit into from
Dec 10, 2024
Merged

bootstrap: Forward cargo JSON output to stdout, not stderr #134123

merged 1 commit into from
Dec 10, 2024

Conversation

Zalathar
Copy link
Contributor

This fixes the RA errors I've been seeing on proc-macros after the re-landing of #134040.

r? clubby789

@rustbot rustbot added 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 Dec 10, 2024
@Zalathar
Copy link
Contributor Author

Unlike the previous revert, this fix is simple enough that I think it's worth trying a fix-forward. We can always revert the whole thing later if we discover more serious issues.

jieyouxu
jieyouxu approved these changes Dec 10, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

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

Thanks, r=me and clubby

@jieyouxu
Copy link
Member

@bors r=jieyouxu,clubby789 rollup

@bors
Copy link
Contributor

bors commented Dec 10, 2024

📌 Commit 604ba92 has been approved by jieyouxu,clubby789

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 Dec 10, 2024
@jieyouxu
Copy link
Member

@bors p=6 (contributor friction)

@jieyouxu
Copy link
Member

(If anyone is doing rollups feel free to include this in a rollup)

@fmease fmease changed the title bootstrap: Forward cargo JSON output to stout, not stderr bootstrap: Forward cargo JSON output to stdout, not stderr Dec 10, 2024
@fmease fmease mentioned this pull request Dec 10, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2024
@bors
Auto merge of rust-lang#134125 - fmease:rollup-u38o3ob, r=fmease
33c245b 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#133478 (jsondocck: Parse, don't validate commands.)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#134008 (Make `Copy` unsafe to implement for ADTs with `unsafe` fields)
 - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction)
 - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`)
 - rust-lang#134041 (Use SourceMap to load debugger visualizer files)
 - rust-lang#134065 (Move `write_graphviz_results`)
 - rust-lang#134106 (Add compiler-maintainers who requested to be on review rotation)
 - rust-lang#134123 (bootstrap: Forward cargo JSON output to stdout, not stderr)

Failed merges:

 - rust-lang#134120 (Remove Felix from ping groups and review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c42c248 into rust-lang:master Dec 10, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 10, 2024
@rust-timer
Unrolled build for rust-lang#134123
85717cd 
Rollup merge of rust-lang#134123 - Zalathar:json-output, r=jieyouxu,clubby789

bootstrap: Forward cargo JSON output to stdout, not stderr

This fixes the RA errors I've been seeing on proc-macros after the re-landing of rust-lang#134040.

r? clubby789
@rustbot rustbot added this to the 1.85.0 milestone Dec 10, 2024
@bors
Copy link
Contributor

bors commented Dec 10, 2024

⌛ Testing commit 604ba92 with merge 33c245b...

@Zalathar Zalathar deleted the json-output branch December 10, 2024 22:17
@Zalathar Zalathar mentioned this pull request Dec 11, 2024
Merged
@Zalathar
Copy link
Contributor Author

For some additional context:

Our suggested rust-analyzer configuration sets "rust-analyzer.cargo.buildScripts.overrideCommand" to an invocation of python3 x.py check --json-output. After #134040, bootstrap was incorrectly forwarding cargo's JSON output to stderr instead of stdout, so RA ignored that output and therefore didn't understand that proc-macros had been built successfully, leading to mysterious errors reported by RA.

@jieyouxu jieyouxu mentioned this pull request Dec 12, 2024
Merged
jieyouxu added a commit to jieyouxu/rust that referenced this pull request Dec 12, 2024
@jieyouxu
Revert "Rollup merge of rust-lang#134123 - Zalathar:json-output, r=ji…
50fac07 
…eyouxu,clubby789"

This reverts commit c42c248, reversing
changes made to 0f1b827.
@jieyouxu jieyouxu mentioned this pull request Dec 12, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 12, 2024
@bors
Auto merge of rust-lang#134207 - jieyouxu:revert-134040, r=lqd
279c65f 
Revert "bootstrap: print{ln}! -> eprint{ln}! (take 2) rust-lang#134040"

Unfortunately, rust-lang#134040 is proving to have caused more output interleaving problems that are tricky to diagnose and fix, and I think we probably should leave these untouched as bootstrap and compiletest has a bunch of interconnecting parts, and the commands and tools that they exercise do not consistently use stderr/stdout either. This causes hard-to-diagnose output interleaving bugs, which unfortunately degrades contributor experience.

This PR reverts two PRs in order to cleanly revert rust-lang#134040:

1. Revert rust-lang#134123 which is a fix-forward after rust-lang#134040.
2. Revert rust-lang#134040 itself.

I don't regret the initial effort `@clubby789,` and thank you for making the attempts, but I think we need to refrain from touching too many of these at once because some of the interleaving are very non-obvious and we don't have test coverage for.

r? `@clubby789`
cc `@Zalathar`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 12, 2024
@matthiaskrgr
Rollup merge of rust-lang#134207 - jieyouxu:revert-134040, r=lqd
8d2759b 
Revert "bootstrap: print{ln}! -> eprint{ln}! (take 2) rust-lang#134040"

Unfortunately, rust-lang#134040 is proving to have caused more output interleaving problems that are tricky to diagnose and fix, and I think we probably should leave these untouched as bootstrap and compiletest has a bunch of interconnecting parts, and the commands and tools that they exercise do not consistently use stderr/stdout either. This causes hard-to-diagnose output interleaving bugs, which unfortunately degrades contributor experience.

This PR reverts two PRs in order to cleanly revert rust-lang#134040:

1. Revert rust-lang#134123 which is a fix-forward after rust-lang#134040.
2. Revert rust-lang#134040 itself.

I don't regret the initial effort `@clubby789,` and thank you for making the attempts, but I think we need to refrain from touching too many of these at once because some of the interleaving are very non-obvious and we don't have test coverage for.

r? `@clubby789`
cc `@Zalathar`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 12, 2024
@rust-timer
Unrolled build for rust-lang#134207
bbf741a 
Rollup merge of rust-lang#134207 - jieyouxu:revert-134040, r=lqd

Revert "bootstrap: print{ln}! -> eprint{ln}! (take 2) rust-lang#134040"

Unfortunately, rust-lang#134040 is proving to have caused more output interleaving problems that are tricky to diagnose and fix, and I think we probably should leave these untouched as bootstrap and compiletest has a bunch of interconnecting parts, and the commands and tools that they exercise do not consistently use stderr/stdout either. This causes hard-to-diagnose output interleaving bugs, which unfortunately degrades contributor experience.

This PR reverts two PRs in order to cleanly revert rust-lang#134040:

1. Revert rust-lang#134123 which is a fix-forward after rust-lang#134040.
2. Revert rust-lang#134040 itself.

I don't regret the initial effort `@clubby789,` and thank you for making the attempts, but I think we need to refrain from touching too many of these at once because some of the interleaving are very non-obvious and we don't have test coverage for.

r? `@clubby789`
cc `@Zalathar`
@matthiaskrgr
Copy link
Member

@bors r- retry

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 12, 2024
@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 12, 2024
@matthiaskrgr
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 12, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Dec 15, 2024
@bors
Auto merge of rust-lang#134125 - fmease:rollup-u38o3ob, r=fmease
1857833 
Rollup of 11 pull requests

Successful merges:

 - rust-lang#133478 (jsondocck: Parse, don't validate commands.)
 - rust-lang#133967 ([AIX] Pass -bnoipath when adding rust upstream dynamic crates)
 - rust-lang#133970 ([AIX] Replace sa_sigaction with sa_union.__su_sigaction for AIX)
 - rust-lang#133980 ([AIX] Remove option "-n" from AIX "ln" command)
 - rust-lang#134008 (Make `Copy` unsafe to implement for ADTs with `unsafe` fields)
 - rust-lang#134017 (Don't use `AsyncFnOnce::CallOnceFuture` bounds for signature deduction)
 - rust-lang#134023 (handle cygwin environment in `install::sanitize_sh`)
 - rust-lang#134041 (Use SourceMap to load debugger visualizer files)
 - rust-lang#134065 (Move `write_graphviz_results`)
 - rust-lang#134106 (Add compiler-maintainers who requested to be on review rotation)
 - rust-lang#134123 (bootstrap: Forward cargo JSON output to stdout, not stderr)

Failed merges:

 - rust-lang#134120 (Remove Felix from ping groups and review rotation)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@clubby789 clubby789

Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.85.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Zalathar @jieyouxu @bors @matthiaskrgr @clubby789 @rustbot