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: include and exclude targets in configuration #2513

Closed
nikunjy opened this issue May 22, 2020 · 14 comments
Closed

nogo: include and exclude targets in configuration #2513

nikunjy opened this issue May 22, 2020 · 14 comments

Comments

@nikunjy
Copy link

nikunjy commented May 22, 2020

What version of rules_go are you using?

 http_archive(
        name = "io_bazel_rules_go",
        sha256 = "87f0fb9747854cb76a0a82430adccb6269f7d394237104a4523b51061c469171",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/rules_go/releases/download/v0.23.1/rules_go-v0.23.1.tar.gz",
            "https://github.com/bazelbuild/rules_go/releases/download/v0.23.1/rules_go-v0.23.1.tar.gz",
        ],
    )

What version of gazelle are you using?

    http_archive(
        name = "bazel_gazelle",
        sha256 = "bfd86b3cbe855d6c16c6fce60d76bd51f5c8dbc9cfcaef7a2bb5c1aafd0710e8",
        urls = [
            "https://mirror.bazel.build/github.com/bazelbuild/bazel-gazelle/releases/download/v0.21.0/bazel-gazelle-v0.21.0.tar.gz",
            "https://github.com/bazelbuild/bazel-gazelle/releases/download/v0.21.0/bazel-gazelle-v0.21.0.tar.gz",
        ],
    )

What version of Bazel are you using?

bazel 3.1.0-homebrew

Does this issue reproduce with the latest releases of all the above?

Yes

What operating system and processor architecture are you using?

Macos

Any other potentially useful information about your toolchain?

I am using go modules.

What did you do?

go_register_toolchains(nogo = "@io_bazel_rules_go//:tools_nogo")
I want to use nogo. I followed the instructions ( I know it mentions this) and it lines external packages which I have no control over. Is there a way I can limit it my code ?

What did you expect to see?

Lint only my code

What did you see instead?

Lints everything in the external.

external/org_golang_x_tools/go/vcs/vcs.go:532:2: declaration of "metaImport" shadows declaration at line 599
@jayconrod
Copy link
Contributor

This is kind of hard to set up. The Configuring analyzers section in the nogo documentation has some information on this. However, the configuration language leaves a lot to be desired, and there's no simple way to exclude external packages. I'll retitle the issue to reflect that.

Nogo definitely needs to be able to analyze packages in external repos though, and that should remain the default.

@jayconrod jayconrod changed the title Nogo shouldn't lint external packages nogo: need a simple way to exclude packages in external repos May 26, 2020
@blico
Copy link
Contributor

blico commented Jul 24, 2020

Our repo ran into an issue where we were unintentionally linting an external library.

As an attempt to prevent linting external libraries within our nogo config all of our linters have the line:

    "only_files": {
      "src/": ""
    }

However, if an external repo contains src/ within its path it will still get linted because the regexp matches (e.g. @github_foo//src/bar.go)

At first, I tried to fix by adding an anchor (e.g. ^src/) to only_files, but I discovered the entire bazel outputRoot is included in the file path matched against the regex. What I ended up doing is adding the following to every linter:

    "exclude_files": {
      "__main__/external": ""
    }

@sluongng
Copy link
Contributor

sluongng commented Aug 6, 2020

Its a bit confusing how nogo works... even with provided documentation.

I am running into this

analyzer "cgocall" failed: can't parse raw cgo file: open /tmp/rules_go_work-906102496/backup.go: no such file or directory

As we have an indirect dependency on github.com/mattn/go-sqlite3 which is cgo, I have added

     "cgocall": {
       "exclude_files": {
         "external/": "external doesn't pass vet"
       }
     },
     "unsafeptr": {
       "exclude_files": {
         "external/": "external doesn't pass vet"
       }
     },

to our config... but it still failing as its scanning from rules_go_work-* for whatever reason

So adding another exclude on rules_go_work-.*/ works for unsafeptr lint error, but the cgocall just straight up failing...

@jayconrod
Copy link
Contributor

@sluongng That's #2396.

@tommyknows
Copy link

tommyknows commented Sep 29, 2020

I'm hitting the same issue; tried to setup nogo but I get a ton of build-time code analysis errors for external projects (maybe also for internal code, I'm not getting that far).

Is it not possible to have some kind of option in the register-toolchain and disable nogo at runtime / analysis phase even?

Edit: I just gave this a shot and "solved" this here: master...tommyknows:nogo-exclude

It's not really pretty, but maybe a good starting point for a proper PR?
I'd love to work on this, but have just started using Bazel and don't really know about any conventions or anything.
My commit would probably need to be cleaned up, variables named clearer etc...

@tian000
Copy link
Contributor

tian000 commented Oct 12, 2020

@tommyknows That solution looks reasonable, and would definitely increase the usability of nogo for people running into the same issue. I think you should open a PR!, I am happy to open one based on your commit too.

@brianwolfe
Copy link

Would it make sense to just add an extra _all field to nogo configuration that has some merge semantics with the individual analysis fields?

@jayconrod
Copy link
Contributor

Copying a comment from #2675:

I think it would be reasonable to add fields like only_targets and exclude_targets, like the only_files and exclude_files fields but the keys would be Bazel label patterns (like @my_repo//...) instead file regular expressions matching file names.

@jayconrod
Copy link
Contributor

+1 to @brianwolfe's suggestion: we could reserve the analyzer name all to match all analyzers with some reasonable merge semantics. I prefer the name all over _all though.

@brianwolfe
Copy link

brianwolfe commented Oct 15, 2020

As long as we are confident there won't be an analyzer called all, I'm fine without the _ prefix 😛.

@jayconrod jayconrod changed the title nogo: need a simple way to exclude packages in external repos nogo: include and exclude targets in configuration Dec 16, 2020
@rmohr
Copy link
Contributor

rmohr commented May 11, 2021

So adding another exclude on rules_go_work-.*/ works for unsafeptr lint error, but the cgocall just straight up failing...

We experience this too when udating to 0.27. It looks exactly like in this old issue: #2172

@rmohr
Copy link
Contributor

rmohr commented May 11, 2021

This may also be related: #2862

@DolceTriade
Copy link
Contributor

DolceTriade commented Nov 16, 2022

I'm interested in picking this up: I put together a PoC here: #3351

I took the suggestion in #2513 (comment) and tried to apply it., but called it _default since it provides a default config for any analyzer whose config is not explicitly specified.. Hopefully we can start a conversation around such an approach (or modify it if we do want the _all behavior where _all is merged into every specified analyzer's config).

@fmeum
Copy link
Member

fmeum commented Aug 2, 2024

Nogo can now be restricted to on a per-package level: https://github.com/bazelbuild/rules_go/blob/master/docs/go/core/bzlmod.md#configuring-nogo

@fmeum fmeum closed this as completed Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants