using gofmt and go vet in sh_test rules

[ixdy]

We have a few lint-style checks for gofmt and go vet which are implemented as sh_tests. We had a data dependency on @io_bazel_rules_go_toolchain//:toolchain, which brought external/io_bazel_rules_go_toolchain/bin into the runfiles for the test, but with the recent refactoring, the :toolchain target no longer exists.

I tried updating this to :go_toolchain, but that doesn't seem to include any of the Go toolchain in the test runfiles anymore.

Is there a way we can depend on the Go toolchain from outside the rules_go repository? I've tried searching through the changes, but I'm having trouble figuring out if this is supposed to be possible.

cc @spxtr

[jayconrod]

I don't think this was supposed to be possible, but it seems like a good use case. Maybe we could expose some stable aliases to tools like gofmt. @ianthehat WDYT?

[ianthehat]

This is possible, but it depends exactly what you want.

There are now two major parts, the go sdk (per platform and version), and the go toolchain (for various combinations of options and cross compiles and underlying sdk).

Toolchain selection logic is currently a temporary hack while we wait for true toolchain selection based on platform constraints to appear in Bazel, which is why some of the rules are a bit obtuse right now, it also means that some of the ways of getting to the toolchain and sdk that I might suggest will also be broken again as soon as that feature arrives.

It sounds like what you really want is the sdk not the toolchain, but without the toolchain there is no way to know which sdk is in use, so it is probably best to go through the toolchain to get to the sdk. For now, you use an attribute "_go_toolchain": attr.label(default = Label("@io_bazel_rules_go_toolchain//:go_toolchain")), to bind your dependancy on the toolchain selector, but that will give you something that is not directly useful in a data dependancy. Also, we no longer capture the entire bin directory from the sdk, only the go binary and the pkg/tools binaries, so if you are using gofmt, we would have to add that as well.

Maybe the right thing to do here is for us to talk about how to provide this functionality directly, it seems like something most users of the rules_go might want anyway, and their might be nicer ways to do it. Could you share your current approach?

[ixdy]

Sure. We use gofmt and go vet as lint checks on our repositories. We have pretty simple shell wrappers around them (verify-gofmt.sh, verify-govet.sh) and then turn them into Bazel tests by depending on all sources in the tree (BUILD).

It's a little strange, since this isn't exactly something Bazel is designed to do, but it works well for us.

We want to follow the Go version used to build the code, since formatting/vet rules tend to change between Go versions. Thus using the sdk/toolchain from go_rules makes sense for us.

If we could depend on the selected gofmt and go binaries directly, that would solve the problem for us. (I'm not familiar with all of the toolchain changes, so I don't now how that plays in to everything.)

[ixdy]

Another possibility is exposing go_vet and go_fmt test rules. I'm not sure how one would best manage the source dependencies, though: users could do something like our all-srcs rules, or they could be automanaged by gazelle?

I think @spxtr and @mikedanese have other ideas, too.

[ash2k]

In addition to go_fmt test rule it'd be useful to have a rule that formats the source files. It should probably format only files/packages that were modified. Or this is possible already (without manually editing each BUILD file)? I'm very new to bazel/rules_go.

[ianthehat]

The thing preventing me from getting this fully done right now is the inability to know what directory bazel run was invoked from. I have started a discussion about this, as soon as we have that I can add the rest of the functionality trivially (based on the work I already did for the gazelle rule) Only changed files or parts of files is much harder because it requires knowledge of what you mean by changed, which needs understanding of all the possible vcs systems, probably better and easier to always apply it to the whole file/files/targets specified.

[drigz]

@ianthehat As of bazel v0.15, bazel run has changed behaviour to the new "direct run", and defines BUILD_WORKSPACE_DIRECTORY and BUILD_WORKING_DIRECTORY (not documented outside of this release note, AFAICT). Does that unblock this?

[jayconrod]

@drigz Right, there's no longer anything blocking us from running a tool invoked by bazel run that writes files in the source directory.

[ash2k]

Related to this issue - if anyone is interested, we have a rule to run goimports. I'm interested in any feedback.

[charlesoconor]

If gofmt was exposed from the rules here it could be integrated into a tool like https://github.com/thundergolfer/bazel-linting-system which currently relies on go to be installed on the system which isn't very hermtic.

[SlyMarbo]

If there's any interest in adding gofmt checking to rules_go, I'd be happy to have a go at implementing it. I've just written a naive implementation for another project here. I've used the gofmt binary from the Go SDK produced by rules_go by referencing @go_sdk//:bin/gofmt. I then use a simple builder tool to invoke gofmt -s -l on a set of source files, extracted from one or more GoSource providers. Using the rule looks like this:

go_library(
    name = "lib",
    srcs = [...],
)

gofmt(
    name = "gofmt",
    embed = [":lib"],
)

If the code is formatted, the 'build' succeeds, producing an empty file. If the code is not formatted, then the build fails, printing the list of filenames that require formatting.

One thing to consider is whether the rule should behave as a build or as a test. Running as a build (as described above) doesn't feel quite right logically, but it's simple to implement. Running as a test makes more sense, but it's a little more fiddly to implement and has the extra downside of making things like bazel test //... more noisy, as most Go packages will now have an extra test target. Not a big problem, but something to consider.

[sluongng]

I am quite convinced that the correct solution here is to build go formatting check into rules_go itself using Validation Action https://docs.bazel.build/versions/main/skylark/rules.html#validation-actions

The idea is to attached this to go_library (for normal source files) and go_test (for test source files) and make sure that these 2 rules will output the correct OutputGroupInfo(_validation = depset([validation_output])),

https://github.com/bazelbuild/rules_go/blob/787d76aee90fea2ffef11c39560f4d330754a4ac/go/private/rules/library.bzl#L47-L50 https://github.com/bazelbuild/rules_go/blob/787d76aee90fea2ffef11c39560f4d330754a4ac/go/private/rules/test.bzl#L176-L178

Not yet sure how to make these approach configurable though:

• swapped out gofmt 🤔 with goimports or gofumpt
• config files?

[derekperkins]

This is somewhat out of scope, but fwiw, we haven't run those as individual tools in years, they're just a part of a single golangci-lint configuration. Getting that to run would require some more work, but then any tool moving forward would just have to integrate once with golangci-lint, and bazel would come for free. There was some progress made a while back, but never pushed over the finish line.

• https://github.com/bazelbuild/rules_go/issues/2695
• https://github.com/golangci/golangci-lint/issues/1473

[Zemnmez]

I've been porting my repo over to MODULE.bazel, and in the walkthrough it states about using the SDK:

If you have a use case that would require this, please explain it in an issue.

I'm trying to do the above. I'm using aspect_rules_lint, which requires a gofmt binary I previously got from go_sdk. I would appreciate a supported way to run the stdlib/cmd tools in the Go SDK.

edit: looks like BuildBuddy has an example of doing this: https://github.com/buildbuddy-io/buildbuddy/blob/0e46543c957fa3fc2341151b5ca632408a3c06b2/rules/go/index.bzl