rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.79k stars 434 forks source link

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

Closed Gankra closed 1 month ago

Gankra commented 1 month ago

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): https://github.com/Gankra/abi-cafe/issues/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 commented 1 month ago

Usage of this PR in abi-cafe: https://github.com/Gankra/abi-cafe/pull/58

Gankra commented 1 month ago

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 commented 1 month ago

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.

thomcc commented 1 month ago

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 commented 1 month ago

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

Gankra commented 1 month ago

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 commented 1 month ago

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

NobodyXu commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Pushed up the new bool-based version

NobodyXu commented 1 month ago
  • 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...

thomcc commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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