bazelbuild
nogo: use bazel validations and don't fail compile
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:
- When you flip on a new validation rule, there may be lots of breakages and you would like to see the entire set of violations, as opposed to the only those appearing in the leaf go_libraries in the DAG. This allows go_libraries to propagate their results as to see downstream nogo violations.
- When iterating locally, it is expensive to turn off nogo; you currently have to change the nogo registration for the sdk which then requires recompiling the entire chain of dependencies without the nogo check. With this change, you can simply pass --norun_validations and all the nogo output is silenced.
Which issues(s) does this PR fix?
Fixes #3695
Other notes for review
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?
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.
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 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
I think there is a tradeoff being made here. It's a strict feature improvement, but I think it's a downgrade for open-source maintenance overall. Similar to adding a new feature without tests, we are simply delegating the verification and refactoring burden to future maintainers.
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?
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 https://github.com/bazelbuild/rules_go/issues/3063#issuecomment-1151363481 and some private conversations I have had, folks to maintain downstream patch(es) to nogo.
So this PR will be a permanent until replaced kind of change and will have a downstream impact, which causes churn for users. So it's a no for me until the concerns I have stated above are addressed (or until we have more users demanding this change). If other maintainers are willing to accept this PR, I won't object 🤗.
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
nogo.
@sluongng Could you elaborate on this? The linked patch mostly touches nogo_main.go, not the Starlark or compilepkg logic for nogo. The current PR only makes a tiny change to nogo_main.go, so I wouldn't expect that to cause much churn.
It was simply an example of folks patching nogo code paths in their own system for different purposes.
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 (.x file generation) in more detail before BazelCon.
Sure! Is there anything I could do for you in the meantime?
It would definitely help if anyone could solve:
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.
But I don't want to make that a requirement for clear incremental improvements.
Update: I submitted https://github.com/bazelbuild/rules_go/pull/3789 to extract nogo facts into separate files. I will look into clearing further blockers for the separation of nogo into a separate action.