-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Fix rustdoc --test-builder argument parsing #80924
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jyn514 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
Can you add a test for the fixed behavior so this doesn't regress again? It would go in |
3973140
to
8180a10
Compare
There you go. I hope I did that right, I don't rebase very often. |
@teryror I think you forgot to add the regression test to git. |
8180a10
to
0401a99
Compare
Co-authored-by: Joshua Nelson <[email protected]>
r=me with CI passing (feel free to ping me if I forget). Thanks for fixing this! |
@bors r+ |
📌 Commit f3c8f29 has been approved by |
Hooray! Glad to be of service. Also, sorry about the brain farts earlier :D |
No problem! That's what review is for anyway :) |
Rollup of 10 pull requests Successful merges: - rust-lang#78901 (diagnostics: Note capturing closures can't be coerced to fns) - rust-lang#79588 (Provide more information for HRTB lifetime errors involving closures) - rust-lang#80232 (Remove redundant def_id lookups) - rust-lang#80662 (Added support for i386-unknown-linux-gnu and i486-unknown-linux-gnu) - rust-lang#80736 (use Once instead of Mutex to manage capture resolution) - rust-lang#80796 (Update to LLVM 11.0.1) - rust-lang#80859 (Fix --pretty=expanded with --remap-path-prefix) - rust-lang#80922 (Revert "Auto merge of rust-lang#76896 - spastorino:codegen-inline-fns2) - rust-lang#80924 (Fix rustdoc --test-builder argument parsing) - rust-lang#80935 (Rename `rustc_middle::lint::LevelSource` to `LevelAndSource`) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
@@ -0,0 +1,6 @@ | |||
// compile-flags: --test -Z unstable-options --test-builder true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rustdoc passes the source code the the builder through stdin. If true
happens to terminate before that, the rustdoc will be terminated by SIGPIPE. This makes the test quite flaky, e.g., #79705 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use cat
instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe wc
if the output would end up in the test log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cat: unrecognized option '--crate-type'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in shell :
will work, but I think this goes through process::Command
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can always just use --test-builder rustc
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, in shell
:
will work, but I think this goes throughprocess::Command
.
No, :
, if it works, is just an alias for true
. It would have the same problem. We need something that will actually read the input.
You can always just use
--test-builder rustc
.
Haha lol, why didn't I think of that. Of course that wouldn't test that the argument actually has any effect but the current test doesn't check that anyway. So +1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #81197 fixing this.
My suggested fix to issue #80893. I can actually hook Miri in there now.
I also fixed what I believe to be a typo in the option's help text.