bazel-contrib / rules_go

Go rules for Bazel
Apache License 2.0
1.39k stars 664 forks source link

Generate Exports for stdlib for golangci-lint #3651

Open DolceTriade opened 1 year ago

DolceTriade commented 1 year ago

What version of rules_go are you using?

0.41.0 and master

What version of gazelle are you using?

0.32.0

What version of Bazel are you using?

Build label: 6.0.0- (@non-git)
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Tue Jan 1 00:00:00 1980 (315532800)
Build timestamp: 315532800
Build timestamp as int: 315532800

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

ya

What operating system and processor architecture are you using?

Linux amd64

Any other potentially useful information about your toolchain?

No

What did you do?

export GOPACKAGEDRIVER=$PWD/tools/gopackagedriver.sh
golangci-lint run -v ./src/... --timeout=1h

What did you expect to see?

golangci-lint complete

What did you see instead?

golangci-lint hangs.

After debugging this, this seems to be an issue with gopackagesdriver not providing the correct Export files for the stdlib. golanglint-ci read the export files for all packages it receives from gopackagedriver, including packages in the stdlib. However, since Bazel sets the ExportFile for every stdlib module to the __BAZEL_OUTPUTDIR__, it fails to open. This can be thousands of imports across all the files which leads to

  1. Incorrect results where stdlib modules fail typecheck
  2. Hanging for extremely long times (I lost patience at like 10min) as golangci-lint continously fails to read the export data here: https://github.com/golangci/golangci-lint/blob/9fc1e2075330561fb31888c4f2aff12b652466c2/pkg/golinters/goanalysis/runner_loadingpackage.go#L175

However, we do have the ability to generate the export files for the stdlib, at the cost of some extra build time on the first build:

diff --git a/go/tools/builders/stdliblist.go b/go/tools/builders/stdliblist.go
index 32f217f3..4c5eb295 100644
--- a/go/tools/builders/stdliblist.go
+++ b/go/tools/builders/stdliblist.go
@@ -158,7 +158,7 @@ func flatPackageForStd(cloneBase string, pkg *goListPackage, pathReplaceFn func(
        ID:              stdlibPackageID(pkg.ImportPath),
        Name:            pkg.Name,
        PkgPath:         pkg.ImportPath,
-       ExportFile:      outputBasePath(cloneBase, pkg.Target),
+       ExportFile:      pathReplaceFn(pkg.Export),
        Imports:         map[string]string{},
        Standard:        pkg.Standard,
        GoFiles:         goFiles,
@@ -265,7 +265,7 @@ func stdliblist(args []string) error {
        listArgs = append(listArgs, "-compiled=true")
    }

-   listArgs = append(listArgs, "-json", "builtin", "std", "runtime/cgo")
+   listArgs = append(listArgs, "-export", "-json", "builtin", "std", "runtime/cgo")

    jsonFile, err := os.Create(*out)
    if err != nil {

And this lets golangci-lint finish quickly and provide more accurate results.

nogo works great for analyzer based linters but golangci-lint offers many more linters that aren't written as analyzers which makes moving to bazel mean that you lose a lot of the lints you were accustomed to when using native Go tooling.

It would be great to support this!

fmeum commented 1 year ago

Would you be interested in submitting this as a PR?

Also, do you happen to have some rough numbers on how much this slows down the first as well as subsequent builds? That would allow us to make an informed decision on whether to make this the default or opt-in.

DolceTriade commented 1 year ago

Sure, I can make a PR. Judging from the wallTime for the mnemonic GoStdlibList in the execution_log with and and without the -export flag set:

Type Time
Master 2.184654s
With -export 22.085003s

So there is a significant jump in time for running. I'll make a PR that allows this to be opt-in from the register_toolchains flag maybe.

ghost commented 2 months ago

Hello, any news on this subjet? This fix is really vital/required to load a package through package.Load() (what golangci-lint does).