-
Notifications
You must be signed in to change notification settings - Fork 433
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
feat: add cargo_output to eliminate last vestiges of stdout pollution #1141
Conversation
Cargo.toml
Outdated
@@ -30,6 +30,8 @@ libc = { version = "0.2.62", default-features = false, optional = true } | |||
|
|||
[features] | |||
parallel = ["dep:libc", "dep:jobserver", "dep:once_cell"] | |||
# enables OutputKind::Stderr which requires rustc 1.74.0 -- currently above msrv |
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.
Hm, maybe it's time to bump the MSRV to 1.75 (RHEL 8.10 & 9.4, Ubuntu 24.04LTS seem to have settled on that version).
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.
This would certainly rock for me personally
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.
Elsewhere I've recommended we switch to stable-5 (which I believe is in line with libc
's stated MSRV... even if it's actual MSRV is preposterously old) which would give us 1.74.
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.
Just to verify, the only reason this is behind a feature is the MSRV situation, right?
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.
correct
context: abi-cafe uses cc-rs as a library, and really wants stdout to be totally clean for its own output report (which may be json, so, very sad if that's messed with). 2 years ago I forked cc-rs to remove all printlns and redirect subcommand stderr to piped (effectively null): Gankra/abi-cafe#49 Since then y'all have wonderfully added the Of course I don't really want to lose those errors, so I included a "pipe stdout to stderr" mode -- unfortunately violating current cc msrv. |
Usage of this PR in abi-cafe: Gankra/abi-cafe#58 |
I will also concede that this PR is still technically a half-measure. What I reaaally want is a way to capture all compiler output (stderr and stdout) to a String(s?) (or |
I also want to make it clear the Stderr mode isn't a blocker for me -- I was already blackholing stdout in my fork, I just know this std trick was added since then and think it's nice to have. |
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.
This looks fine to me. I think we should consider bumping MSRV to 1.74 for this. Removing a cargo feature is considered a breaking change I believe, so I'd prefer to bump the MSRV then to add a cargo feature.
This would be an aggressive enough bump that it might be worth giving a heads up in t-libs/crate-maintainers zulip first tho (edit: done).
Oh, I might prefer to wait on it then. |
Ok so would we be happy landing this with the stderr stuff removed? |
Also if we're cutting down to that I suppose we can make |
(It's forward compat to add enum to the public API later, with an |
Yeah, alternatively, perhaps we can take a |
Yeah i played around with a sufficiently fancy |
Pushed up the new bool-based version |
Aha wait I see the problem with So maybe we could take a function It's a bit hacky though... |
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.
Looks good to me.
I think we should take this for now, and once we have a new enough MSRV, (try to remember to) extend the API later with the more advanced functionality rather than hacking it in. |
To clarify, removing a cargo feature and releasing a version without that feature means that dependees using that feature do not upgrade to newer versions. This is an automatic behavior of the resolver. |
(also to clarify i would have suggested the feature grows to be defaulted on once msrv is achieved but this is all moot now) |
No description provided.