nogo: use bazel validations and don't fail compile #3707
Conversation
|
Cc @sluongng |
There was a problem hiding this comment.
Another downside of the current approach will increase the maintenance complexity over time, by adding more features into compilepkg.go. I think it would also help you refactor CI tests much easier if you separate nogo logic to a new subcommand.
The idea of writing out an output file is good. However, I don't think it should be solely used to output errors. Instead, we should consider formats such as https://github.com/reviewdog/reviewdog?tab=readme-ov-file#reviewdog-diagnostic-format-rdformat so that we could let other tools consume these validation outputs and help the user fix their listing mistakes automatically (where possible).
| inputs = [out_nogo_log], | ||
| outputs = [out_nogo_validation], | ||
| # Create the expected output file if there are no errors, otherwise print the errors to stderr. | ||
| command = '[ -s "$1" ] && cat <(echo) "$1" <(echo) 1>&2 || touch $2', |
There was a problem hiding this comment.
I don't like this approach. The actual nogo is still running as part of GoCompilePkg action, producing an output that is consumed by this no-ops command.
I would prefer if you start with separating nogo part of the go code into a separate builder sub-command, or ever a separate binary. Then in the validation action, we instrument that new subcommand/binary instead.
Not quite sure if it's a thing, but ideally user should be able to request for a --output_group=_validation and only trigger the validation actions to run linter, without much dependency on the compilation action.
|
Hey @sluongng thanks for the review! I considered moving this out of the compilepkg action altogether, and I do think your are right; that is the best strategy long term. One issue right now is it seems though that we combine the nogo facts and the export data into a single .x archive that is used by downstream compilepkg actions, and thus the actions are still a bit coupled together. I do not have the bandwidth right now to fully split these actions into its ideal long-term state. However, I would argue that this is still a net benefit over the current state, and has the proper abstractions in place we could improve behind in the future: a CompilePkg action that only fails on compile errors, and a separate Nogo validation action that fails on static analysis errors but skipped with --norun_validations Would you be willing to accept this patch that still provides many improvements over the current state and allow it to be improved later in the future? |
Ah good point. There are a few other clean-ups to the .x and .a dependencies that folks do not have the bandwidth to act on right now.
I think there is a tradeoff being made here.
I thought about accepting this PR if we could somehow "feature flag" this new behavior and make it opt-in instead of opt-out. However, in cases such as #3063 (comment) and some private conversations I have had, folks to maintain downstream patch(es) to So this PR will be a |
@sluongng Could you elaborate on this? The linked patch mostly touches |
|
It was simply an example of folks patching I don't have a public example, but this is more common in enterprises that use Go and rules_go and want to have tight integration between the linter and code review system to make automatic changes suggestions from CI runs. |
|
@joeljeske I want to get back to this, but won't be able to look into the foundations ( |
|
Sure! Is there anything I could do for you in the meantime? |
It would definitely help if anyone could solve:
But I don't want to make that a requirement for clear incremental improvements. |
There was a problem hiding this comment.
Sorry for the radio silence. Now that we have figured out how to make nogo work with Bzlmod, I have the capacity to look into this.
I don't want to block this nice incremental improvement on fully removing nogo from the compilation path, I just have some comments and concerns regarding the current implementation of the validation action.
| gc_goopts = source.gc_goopts, | ||
| cgo = False, | ||
| testfilter = testfilter, | ||
| recompile_internal_deps = recompile_internal_deps, | ||
| ) | ||
|
|
||
| if go.nogo: | ||
| validations.append(out_nogo_validation) | ||
| go.actions.run_shell( |
There was a problem hiding this comment.
We try to avoid relying on run_shell on Windows. Can you think of a way to replace this with a run?
For example, could this logic be moved into https://github.com/bazelbuild/rules_go/blob/master/go/tools/builders/generate_nogo_main.go?
| out_nogo_log.path, | ||
| out_nogo_validation.path, | ||
| ], | ||
| progress_message = "Nogo Static Analysis", |
There was a problem hiding this comment.
This is slightly misleading as the action just processes the results, it doesn't analyze anything.
Also, purely from a stylistic point of view, progress messages typically start with a verb in progressive form, e.g. Post-processing nogo findings.
| out_nogo_validation.path, | ||
| ], | ||
| progress_message = "Nogo Static Analysis", | ||
| mnemonic = "GoNogo", |
There was a problem hiding this comment.
In the future in which we have moved out the nogo analysis into its own action, this is exactly the mnemonic we would like to use for that. Since mnemonics are public API, please use something else here.
| @@ -112,6 +113,9 @@ def emit_compilepkg( | |||
| if go.nogo: | |||
| args.add("-nogo", go.nogo) | |||
| inputs.append(go.nogo) | |||
| if out_nogo_log: | |||
There was a problem hiding this comment.
Can this ever validly be None? If not, let's fail if it is.
|
Update: I submitted #3789 to extract nogo facts into separate files. I will look into clearing further blockers for the separation of nogo into a separate action. |
What type of PR is this?
Feature
What does this PR do? Why is it needed?
This PR allows failing nogo findings to be captured as a Bazel validation action and allow the compilepkg actions to exit 0 even when there are nogo failures.
This is needed for a couple reasons:
Which issues(s) does this PR fix?
Fixes #3695
Other notes for review