Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support nogo with Bzlmod #3529

Closed
Tracked by #140
jmhodges opened this issue Apr 12, 2023 · 5 comments · Fixed by #3782
Closed
Tracked by #140

Support nogo with Bzlmod #3529

jmhodges opened this issue Apr 12, 2023 · 5 comments · Fixed by #3782

Comments

@jmhodges
Copy link
Contributor

jmhodges commented Apr 12, 2023

It would be nice for there to be docs on how to use nogo with the go_sdk bzlmod extension. I'm still a newbie with the new Bazel module system and it's not obvious to me how to translate go_register_toolchains(nogo = ...) into it.

I'd also be happy to hear there's another preferred way of applying vet and such

@fmeum
Copy link
Member

fmeum commented Apr 12, 2023

This is actually more than a docs issue: There is no design for how to represent nogo with Bzlmod yet. It's complex enough that I would like to give this some thought, but I also lack experience with it.

If anyone has ideas, please let me know and we can work on a design together.

@fmeum
Copy link
Member

fmeum commented Apr 12, 2023

One idea would be to add the following module tag:

go_lint.nogo(
    target = "//:my_nogo_target",
    apply_to = ["my_module"],
)

Here apply_to would be optional and default to the root module only. Only the go_lint.nogo tag of the root module would be evaluated and nogo would only be active for Go targets in repos created by modules listed in apply_to.

@fmeum fmeum changed the title docs: nogo with MODULE.bazel docs Support nogo with Bzlmod Apr 21, 2023
@sluongng
Copy link
Contributor

@fmeum asked what do I think about this, so here is my brain dump on the current state of nogo.


I think nogo require a revamp from the ground up. There are several issues with it currently:

  1. Poor UX
  • Currently, it's being run as part of CompilePkg, which on failure, creates errors really hard for humans to parse. I had to feed the error to ChatGPT to explain to me what was going on sometimes.
  • There is no way to suggest a fix and consume that fix suggestion automatically. Most other linters do support auto-fixes and diff mode.
  1. Poor scalability
  • It's being run in CompilePkg also means that in a very BIG package and with a lot of analyzers, Bazel is still only dedicating 1 CPU to execute that action locally. This causes compilation to be quite slow on such packages and discourages folks from adding more analyzers, which is counterintuitive.
  1. Cyclic Dependency
  • Because we first need to compile the nogo linter first with all the analyzer, then add it into the compile action using a transition, it would lead to Bazel wrongfully detecting cyclic dependency. A -> B -> A' where A and A' are the same target being compiled with different transitions.
  1. Poor configurability
  • Linter config comes with poor default today, where we forcefully run nogo on top of external source code that users don't control.
  • Current config is in JSON. This could be encoded from starlark, but users would have to code up such a thing.

With that said, I want to explore redesigning nogo to goval where we should run linter using Bazel Validation Actions. Rename so that we could ship both nogo and goval in rules_go before announcing a deprecation.

This directly helps solve (1) and (2) quite nicely as the validation will be executed in a separate action, and there will be a dedicated OutputGroup for us to pew out fixes and recommendations for downstream consumption (i.e. running a fixer command that would consume _validation output group to apply in-tree fixes).

The design needs to be broken down into a few parts:

a. Preparing the runner binary: do we compile the goval binary like nogo binary? Or should we allow/encourage folks to use a pre-compiled binary for it to simplify the setup?

b. Validation action starlark rules: how do we add validation action into go_library and go_test and go_binary?

c. Linter config: where / how do we add a config? Perhaps we could start with a global config and then start supporting package-level overrides such as apple_rules_lint? We should exclude external modules by default by only enabling goval for the current workspace/module in the config. All other modules would require explicit opt-in.

d. Consider running some Analyzers in parallel and tell Bazel about the accurate CPU consumption of such action.


I think redesigning/rewriting would give us more freedom to make the new lint solution work better with bzlmod.

@zecke
Copy link
Contributor

zecke commented Oct 28, 2023

For me the lack of nogo is among the last blockers of moving to bzlmod. In many cases perfect is the enemy of good and we have the above data point of a team stopping to use the linter in order to move to bzlmod.

What could an incremental change look like?

@sluongng
Copy link
Contributor

I don't think anyone is attempting this currently. Most folks have other priorities with their $day_job. Would be nice if some community member pick this up and create a poc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants