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

feat: add cargo_output to eliminate last vestiges of stdout pollution #1141

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Jul 7, 2024

No description provided.

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
Copy link
Member

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).

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

@Gankra
Copy link
Contributor Author

Gankra commented Jul 7, 2024

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 cargo_* APIs to address this exact problem, however the "stderr to null" case is still outstanding. On the happy path compilers don't write to stdout, but when the source code has errors they like spewing them to stdout!

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.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 7, 2024

Usage of this PR in abi-cafe: Gankra/abi-cafe#58

@Gankra
Copy link
Contributor Author

Gankra commented Jul 7, 2024

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 Vec<u8>, whatevs) when invoking try_compile. I briefly experimented with what this would look like and it was a bit too messy for my inexperience with this codebase.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 7, 2024

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.

Copy link
Member

@thomcc thomcc left a 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).

@thomcc
Copy link
Member

thomcc commented Jul 8, 2024

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.

Oh, I might prefer to wait on it then.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

Ok so would we be happy landing this with the stderr stuff removed?

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

Also if we're cutting down to that I suppose we can make cargo_output take a bool and internally desugar to these options?

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

(It's forward compat to add enum to the public API later, with an impl Into<OutputKind> for bool kinda thing)

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jul 8, 2024

Ok so would we be happy landing this with the stderr stuff removed?

Yeah, alternatively, perhaps we can take a Box<dyn (Into<Stdio> + Clone + Send + Sync) + 'static> instead?

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

Yeah i played around with a sufficiently fancy dyn but you can't have + Clone or + Debug in there (error[E0225]: only auto traits can be used as additional traits in a trait object)

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

Pushed up the new bool-based version

@NobodyXu
Copy link
Collaborator

NobodyXu commented Jul 8, 2024

  • Clone

Aha wait I see the problem with Clone, the Self type is the problem...

So maybe we could take a function Box<dyn Fn() -> Stdio + Send + Sync + 'static>?

It's a bit hacky though...

Copy link
Member

@thomcc thomcc left a 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.

@thomcc
Copy link
Member

thomcc commented Jul 8, 2024

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.

@workingjubilee
Copy link
Member

workingjubilee commented Jul 8, 2024

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.

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.

@Gankra
Copy link
Contributor Author

Gankra commented Jul 8, 2024

(also to clarify i would have suggested the feature grows to be defaulted on once msrv is achieved but this is all moot now)

@thomcc thomcc merged commit ba08d63 into rust-lang:main Jul 8, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants