feat: Allow process_wrapper to capture exit codes instead of forwarding them

[blorente]

Draft implementation of #3771
I'll add tests once the maintainers have confirmed that this behaviour is wanted.

[alexeagle]

simpler: would a user ever need to set one option but not the other? If there's an exit code file, then capture, otherwise forward

[blorente]

I thought about it, but I'm not convinced it's the right approach. Because this is the process wrapper so it can be used for anything, I can see someone wanting to ignore non-zero exit values but not wanting to declare a file, e.g. if we 'bazel run'.

I agree that this is not a real use case right now, so I could definitely be convinced to collapse the arguments.

[illicitonion]

Works for me! I'd be tempted to name the flag --swallow-exit-code or --exit-0 so that it defaults false rather than defaulting true, but I'm not super picky about it...

[blorente]

@illicitonion I've renamed the flag as --swallow-subprocess-exit-code, as the parsing semantics are more clear (it follows the convention of "Only exactly true is allowed"), but kept the internal option as forward_exit_code, because "forward something" seems easier to read in the implementation than "don't not forward something".

Splitting hairs? Probably. Now to make CI pass.

[blorente]

I'm not sure what's up with CI, but the remaining failures don't seem to be related to this PR. Locally, bazel test //test/... passes.

[UebelAndre]

I was wondering. Why can't aspect_rules_lint transitively collect all rust_clippy_check output groups from deps and return that in a top level target? Why is the need to spawn actions like this necessary?

[blorente]

Why can't aspect_rules_lint transitively collect all rust_clippy_check output groups from deps and return that in a top level target?

I think the main reason might be the same as this PR honestly. Without this PR, I'm not sure how we could capture the errors from clippy for post-processing without failing the entire action. If I understand correctly, right now rules_rust accomplishes this by demoting all errors to warnings, which is not ideal.

Why is the need to spawn actions like this necessary?

The other reason is less important and related to rules_lint and supporting --fix. rules_lint has a custom binary that it uses to run the linters, generate diffs, and apply patch files. It would be extremely handy if rules_rust could tell that binary how to run clippy to generate patches. I think this is a secondary reason, but a reason nonetheless.

[UebelAndre]

I think the main reason might be the same as this PR honestly. Without this PR, I'm not sure how we could capture the errors from clippy for post-processing without failing the entire action. If I understand correctly, right now rules_rust accomplishes this by demoting all errors to warnings, which is not ideal.

Why do you need to swallow the error code? I'm also not sure I follow the rationale for why rust_clippy_action is needed vs collecting outputs from existing actions. When this was introduced I thought it was kinda ok because the interface hasn't changed by it feels like now there's immediate changes to it and am concerned around the scope of our public interface for Rustc actions.

[blorente]

it feels like now there's immediate changes to it and am concerned around the scope of our public interface for Rustc actions.

@UebelAndre That's fair, I'm happy to step back and discuss the core problem to see if there's a better solution :)

In fact, let's ignore "spawning an action from a different ruleset" for now, as I think this PR still has purpose outside of it. I can open a separate issue to discuss how to work with rules_lint in the grand scheme of things.

The main problem this PR tries to solve: There is no way for me to, at the same time:

• Capture clippy's output, and
• Preserve warnings as warnings and errors as errors.

This is because rules_rust enforces that "if you're capturing output, you must pass --cap-lints=warn to clippy. Otherwise, if you have any errors, the action will fail and the output file will not be written".

blorente/bzl-repros#1 is a minimal repro of the problem. If I run bazel build //:all --config=clippy, I only get warnings in the output even though I'd expect one error and one warning.

I'm not sold on this exact interface being the right solution, but I do think that preserving the warnings and errors does require teaching the process wrapper to not fail when the child process fails.

---

A secondary problem that this PR tries to solve is "there is no way for me to know the exit code of clippy". Right now, <output>.ok only gets created if clippy succeeds, which is not enough information. Plus, there is no way to both capture the output of clippy and the exit code.

Having this information would be useful but not vital, so this problem has lower priority than the one above. I'm happy to split the solution to this problem to another PR.