nogo: use bazel validations and don't fail compile #3707

joeljeske · 2023-09-22T20:35:08Z

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

fmeum · 2023-09-22T21:08:06Z

Cc @sluongng

go/tools/builders/compilepkg.go

sluongng

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

sluongng · 2023-09-25T09:53:51Z

go/private/actions/archive.bzl

+            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',


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.

joeljeske · 2023-09-25T14:50:59Z

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?

sluongng · 2023-09-26T09:46:38Z

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 #3063 (comment) 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 🤗.

fmeum · 2023-09-26T10:05:18Z

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.

sluongng · 2023-09-26T12:07:30Z

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.

fmeum · 2023-10-17T07:53:32Z

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

joeljeske · 2023-10-17T11:00:21Z

Sure! Is there anything I could do for you in the meantime?

fmeum · 2023-10-17T11:36:44Z

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.

fmeum

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.

fmeum · 2023-12-15T15:05:38Z

go/private/actions/archive.bzl

            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(


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?

fmeum · 2023-12-15T15:07:34Z

go/private/actions/archive.bzl

+                out_nogo_log.path,
+                out_nogo_validation.path,
+            ],
+            progress_message = "Nogo Static Analysis",


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.

fmeum · 2023-12-15T15:09:51Z

go/private/actions/archive.bzl

+                out_nogo_validation.path,
+            ],
+            progress_message = "Nogo Static Analysis",
+            mnemonic = "GoNogo",


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.

fmeum · 2023-12-15T15:11:33Z

go/private/actions/compilepkg.bzl

@@ -112,6 +113,9 @@ def emit_compilepkg(
    if go.nogo:
        args.add("-nogo", go.nogo)
        inputs.append(go.nogo)
+        if out_nogo_log:


Can this ever validly be None? If not, let's fail if it is.

go/tools/builders/compilepkg.go

fmeum · 2023-12-16T13:42:53Z

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.

nogo: use bazel validations and don't fail compile

f621dfa

fmeum self-requested a review September 22, 2023 21:09

fmeum reviewed Sep 25, 2023

View reviewed changes

go/tools/builders/compilepkg.go Show resolved Hide resolved

sluongng suggested changes Sep 25, 2023

View reviewed changes

linzhp mentioned this pull request Dec 6, 2023

nogo nolint comments don't work across dependencies when using nilaway #3774

Open

fmeum reviewed Dec 15, 2023

View reviewed changes

fmeum mentioned this pull request Jul 26, 2024

Run nogo in a separate validation action #3995

Merged

fmeum closed this in 29d4e5d Aug 6, 2024

fmeum closed this in #3995 Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nogo: use bazel validations and don't fail compile #3707

nogo: use bazel validations and don't fail compile #3707

joeljeske commented Sep 22, 2023

fmeum commented Sep 22, 2023

sluongng left a comment

sluongng Sep 25, 2023

joeljeske commented Sep 25, 2023 •

edited

Loading

sluongng commented Sep 26, 2023

fmeum commented Sep 26, 2023

sluongng commented Sep 26, 2023

fmeum commented Oct 17, 2023

joeljeske commented Oct 17, 2023

fmeum commented Oct 17, 2023

fmeum left a comment •

edited

Loading

fmeum Dec 15, 2023

fmeum Dec 15, 2023

fmeum Dec 15, 2023

fmeum Dec 15, 2023

fmeum commented Dec 16, 2023

nogo: use bazel validations and don't fail compile #3707

nogo: use bazel validations and don't fail compile #3707

Conversation

joeljeske commented Sep 22, 2023

fmeum commented Sep 22, 2023

sluongng left a comment

Choose a reason for hiding this comment

sluongng Sep 25, 2023

Choose a reason for hiding this comment

joeljeske commented Sep 25, 2023 • edited Loading

sluongng commented Sep 26, 2023

fmeum commented Sep 26, 2023

sluongng commented Sep 26, 2023

fmeum commented Oct 17, 2023

joeljeske commented Oct 17, 2023

fmeum commented Oct 17, 2023

fmeum left a comment • edited Loading

Choose a reason for hiding this comment

fmeum Dec 15, 2023

Choose a reason for hiding this comment

fmeum Dec 15, 2023

Choose a reason for hiding this comment

fmeum Dec 15, 2023

Choose a reason for hiding this comment

fmeum Dec 15, 2023

Choose a reason for hiding this comment

fmeum commented Dec 16, 2023

joeljeske commented Sep 25, 2023 •

edited

Loading

fmeum left a comment •

edited

Loading