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.

Sign up for GitHub

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

Jump to bottom

[Bug]: golangci-lint doesn't pull in dependencies #129

Closed
jelmer opened this issue Jan 29, 2024 · 6 comments · Fixed by #207
Closed

[Bug]: golangci-lint doesn't pull in dependencies #129

jelmer opened this issue Jan 29, 2024 · 6 comments · Fixed by #207
Labels
bug Something isn't working

Comments

@jelmer
Copy link
Contributor

jelmer commented Jan 29, 2024

What happened?

golangci-lint doesn't just look at the .go file specified on the command-line, it also needs to pull in metadata about e.g. types from other files that are referenced from the file that is being linted. Without this, it complains about missing files.

The lint target should inherit all dependencies of the original target, as well as dependencies on any generated files in srcs as those are currently being filtered out (since they don't newed to be linted).

Version

Development (host) and target OS/architectures:

Output of bazel --version:

bazel 7.0.2

Version of the Aspect rules, or other relevant rules from your
WORKSPACE or MODULE.bazel file:

N/A, reproducible using example from rules_lint

Language(s) and/or frameworks involved:

Go / golang-ci

How to reproduce

I'll attach a PR to this bug that demonstrates the issue with the example in the rules_lint repo.

Any other information?

No response
@jelmer jelmer added the bug Something isn't working label Jan 29, 2024
jelmer added a commit to jelmer/rules_lint that referenced this issue Jan 29, 2024
@jelmer
Add test showing that golangci-lint lint targets created by golangci_…
f4267c6 
…lint_aspect() fail to pull in dependencies.

Demonstrates aspect-build#129. Run "./lint.sh src:hello2_go" in example/ and you
should see something like this:

INFO: Invocation ID: ac22e950-5606-4161-8987-0e288bd0d046
INFO: Analyzed target //src:hello2_go (1 packages loaded, 4 targets configured).
INFO: From golangcilint src/golangcilint.hello2_go.aspect_rules_lint.report:
level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"github.com/aspect-build/rules_lint/example/src/hello_helper\""
level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"github.com/aspect-build/rules_lint/example/src/hello_helper\"\n\n"
INFO: Found 1 target...
Aspect //tools:lint.bzl%golangci_lint of //src:hello2_go up-to-date:
  bazel-bin/src/golangcilint.hello2_go.aspect_rules_lint.report
INFO: Elapsed time: 2.526s, Critical Path: 2.24s
INFO: 2 processes: 1 internal, 1 linux-sandbox.
INFO: Build completed successfully, 2 total actions
INFO: Build Event Protocol files produced successfully.
srabraham added a commit to srabraham/rules_lint that referenced this issue Feb 9, 2024
@srabraham
chore: demonstrate golangci-lint not working with deps
ecacb26 
I get an error like below when I run
bazel lint src:hello_go
as a result of the go_binary depending on another go target

this is a demo of aspect-build#129

INFO: Analyzed target //src:hello_go (0 packages loaded, 0 targets configured).
INFO: From golangcilint src/golangcilint.hello_go.aspect_rules_lint.report:
level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"gopher\""
level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"gopher\"\n\n"
@srabraham srabraham mentioned this issue Feb 9, 2024
Draft
@srabraham
Copy link
Contributor

ah, I missed the fact that @jelmer already made a PR that demonstrated this... here's my unnecessary demonstration of this as well. #145
FYI @alexeagle. This bug makes rules_lint effectively unusable for golangci-lint

alexeagle pushed a commit to srabraham/rules_lint that referenced this issue Apr 7, 2024
@srabraham @alexeagle
chore: demonstrate golangci-lint not working with deps
6c633ef 
I get an error like below when I run
bazel lint src:hello_go
as a result of the go_binary depending on another go target

this is a demo of aspect-build#129

INFO: Analyzed target //src:hello_go (0 packages loaded, 0 targets configured).
INFO: From golangcilint src/golangcilint.hello_go.aspect_rules_lint.report:
level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"gopher\""
level=error msg="Running error: 1 error occurred:\n\t* can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for \"gopher\"\n\n"
@alexeagle
Copy link
Member

Taking a look at this finally. I added a commit on #145 which adds these missing files to the inputs of the action - but that's not enough because Go expects the files to be in $GOROOT/pkg/gopher or something. It seems like rules_go does some shenanigans to make these resolve, will require some more study.

In the meantime I'm tempted to remove golangci-lint from rules_lint until we have something that works for this case, WDYT?

@jelmer
Copy link
Contributor Author

jelmer commented Apr 7, 2024 •
edited
Loading

I agree that's probably better than having it half-work and having people try it until they stumble over this bug.

@psalaberria002 might have thoughts as well, since IIRC he originally added this support and might have opinion on whether it's useful to keep with this bug present.

@psalaberria002
Copy link
Contributor

I agree that's probably better than having it half-work and having people try it until they stumble over this bug.

@psalaberria002 might have thoughts as well, since IIRC he originally added this support and might have opinion on whether it's useful to keep with this bug present.

Ok with reverting it until it's fully supported. We are currently using nogo instead.

@alexeagle
Copy link
Member

@mgechev I know you've got bigger fish to fry, but in case you'd like to come play in Bazel-land, I'd love to have golang supported here!

alexeagle added a commit that referenced this issue Apr 11, 2024
@alexeagle
feat: remove golangci-lint
fea7b91 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.
alexeagle added a commit that referenced this issue Apr 11, 2024
@alexeagle
feat: remove golangci-lint
f2709ec 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes #129
@alexeagle alexeagle mentioned this issue Apr 11, 2024
Merged
alexeagle added a commit that referenced this issue Apr 11, 2024
@alexeagle
feat: remove golangci-lint (#207)
ee24ede 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes #129
@alexeagle alexeagle mentioned this issue Apr 11, 2024
Open
@hunshcn
Copy link
Contributor

hunshcn commented Apr 12, 2024

Creating an empty repository rule and using this directory as a cache may be a solution, although it is dirty.

@alexeagle

mrmeku pushed a commit to mrmeku/rules_lint that referenced this issue Apr 15, 2024
@alexeagle @mrmeku
feat: remove golangci-lint (aspect-build#207)
9cfc08a 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for aspect-build#129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes aspect-build#129
mrmeku pushed a commit to mrmeku/rules_lint that referenced this issue Apr 15, 2024
@alexeagle @mrmeku
feat: remove golangci-lint (aspect-build#207)
ada8aa2 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for aspect-build#129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes aspect-build#129
alexeagle added a commit that referenced this issue Apr 22, 2024
@mrmeku @alexeagle
feat(format): Add ability to ignore files (#210)
291f888 
* feat: remove golangci-lint (#207)

In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes #129

* Move changes from linting.md to formatting.md

* Address review comments

---------

Co-authored-by: Alex Eagle <[email protected]>
alexeagle added a commit that referenced this issue May 31, 2024
@alexeagle
feat: remove golangci-lint (#207)
758b8a1 
In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes #129
alexeagle added a commit that referenced this issue May 31, 2024
@mrmeku @alexeagle
feat(format): Add ability to ignore files (#210)
bca3379 
* feat: remove golangci-lint (#207)

In preparation for 1.0, we want all linters to work correctly.
In the discussion for #129 we decided that is a fatal bug, and no one is able to fix it in the short term.

We're happy to add Go linting back to rules_lint when someone has a correct implementation that handles transitive srcs.
The code will still be here in the git history when it's time to revive it.

Closes #129

* Move changes from linting.md to formatting.md

* Address review comments

---------

Co-authored-by: Alex Eagle <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: remove golangci-lint aspect-build/rules_lint
5 participants
@alexeagle @jelmer @psalaberria002 @hunshcn @srabraham