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

nogo: Allow not running analyzers for certain packages #3838

Closed
DolceTriade opened this issue Jan 20, 2024 · 7 comments · Fixed by #3842
Closed

nogo: Allow not running analyzers for certain packages #3838

DolceTriade opened this issue Jan 20, 2024 · 7 comments · Fixed by #3842

Comments

@DolceTriade
Copy link
Contributor

Currently, nogo is run unconditionally for all go code. The exclude/include escape hatches only ignore diagnostic errors from running analyzers, however, the analyzers are still run.

This is problematic because some external code like AWS's EC2 go client are huge and take a lot of memory to compile. This causes our build to OOM. It would be nice to have a mechanism to not run an analyzer at all.

This would allow builds to run must faster since you aren't linting external code that you don't care to lint.

I propose adding two new fields to the nogo config:

"exclude_pkg": {
  "github.com/DolceTriade/generated": "this is generated code"
},
"only_pkg": {
  "github.com/DolceTriade": "only lint my code"
}

The semantics of exclude_pkg and only_pkg would be the same as only_files and exclude_files, except they operate at the package level and instead of ignoring errors from an analyzer, they don't run that analyzer all together.

DolceTriade added a commit to DolceTriade/rules_go that referenced this issue Jan 20, 2024
Currently, nogo is run unconditionally for all go code. The exclude/include escape hatches only ignore diagnostic errors from running analyzers, however, the analyzers are still run.

This is problematic because some external code like AWS's EC2 go client are huge and take a lot of memory to compile. This causes our build to OOM. It would be nice to have a mechanism to not run an analyzer at all.

This would allow builds to run must faster since you aren't linting external code that you don't care to lint.

This PR adds two new fields to the nogo config: "exclude_pkg" and
"only_pkg". These semantics are the same as "only_file" and
"exclude_file"

Fixes bazel-contrib#3838
DolceTriade added a commit to DolceTriade/rules_go that referenced this issue Jan 20, 2024
Currently, nogo is run unconditionally for all go code. The exclude/include escape hatches only ignore diagnostic errors from running analyzers, however, the analyzers are still run.

This is problematic because some external code like AWS's EC2 go client are huge and take a lot of memory to compile. This causes our build to OOM. It would be nice to have a mechanism to not run an analyzer at all.

This would allow builds to run must faster since you aren't linting external code that you don't care to lint.

This PR adds two new fields to the nogo config: "exclude_pkg" and
"only_pkg". These semantics are the same as "only_file" and
"exclude_file"

Fixes bazel-contrib#3838
@fmeum
Copy link
Member

fmeum commented Jan 20, 2024

Have you seen https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/bzlmod.md#configuring-nogo? It looks like this would cover your use case, it just uses Bazel instead of Go packages to identify the inclusion/exclusion scope.

@DolceTriade
Copy link
Contributor Author

Is this exclusively using bzlmod? Unfortunately it will take some time for us migrate to bzlmod.

@fmeum
Copy link
Member

fmeum commented Jan 20, 2024

Is this exclusively using bzlmod? Unfortunately it will take some time for us migrate to bzlmod.

For now yes, but if we can find a way to realize the same configuration option for WORKSPACE, we could definitely open it up. The general setup is there.

@DolceTriade
Copy link
Contributor Author

I'll look into that approach.

@DolceTriade
Copy link
Contributor Author

DolceTriade commented Jan 20, 2024

Do you think the approach should be to expose includes/excludes into go_register_toolchains or just to expose go_register_nogo to workspace users?

It sounds like the latter is the right thing to do. Maybe make a wrapper that automatically sets the name...

As a workaround:

This works until we can migrate to bzlmod.

load("@io_bazel_rules_go//go/private:nogo.bzl", "go_register_nogo")

# Add lint checking
go_register_nogo(
    name = "io_bazel_rules_nogo",
    includes = ["@@//:__subpackages__"],
    nogo = "@//bazel/go:lint",
)

@fmeum
Copy link
Member

fmeum commented Jan 20, 2024

Yes, the approach of providing a simple macro wrapper around your workaround sounds best to me.

Could you add a test along the lines of the existing Bzlmod test? This would help us clarify how the WORKSPACE way of configuring nogo interacts with apparent vs. canonical repository names.

@DolceTriade
Copy link
Contributor Author

Took another stab at this here: #3842

DolceTriade added a commit to DolceTriade/rules_go that referenced this issue Jan 28, 2024
Currently, nogo is run unconditionally for all go code. The exclude/include escape hatches only ignore diagnostic errors from running analyzers, however, the analyzers are still run.

This is problematic because some external code like AWS's EC2 go client are huge and take a lot of memory to compile. This causes our build to OOM. It would be nice to have a mechanism to not run an analyzer at all.

This would allow builds to run must faster since you aren't linting external code that you don't care to lint.

This PR adds two new fields to the nogo config: "exclude_pkg" and
"only_pkg". These semantics are the same as "only_file" and
"exclude_file"

Fixes bazel-contrib#3838
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants