Enforce consistent Go import organization using `goimports` #8198

dan-stowell · 2025-01-16T20:09:09Z

@iain-macdonald (VSCode) and I (Zed) noticed our editors making seemingly-superfluous changes to Go imports on save. Turns out this is due to gopls running import organization logic.

It would be nice to consistently organize imports across the codebase the same way we consistently format Go code using gofmt. Luckily there's a tool that can do just that! goimports

This change adds goimports to buildfix.sh and to checkstyle.sh. Running buildfix.sh on a recent checkout touches 4 files. (Diff at the bottom). I can of course add these changes to this pull request if we want to move forward.

The downside of introducing this change is that people's editors may not adhere to goimports organization or may actively fight it. I don't want to cause headaches!

diff --git a/enterprise/server/sociartifactstore/sociartifactstore_darwin.go b/enterprise/server/sociartifactstore/sociartifactstore_darwin.go
index 8745f9b794..0b9270fb1f 100644
--- a/enterprise/server/sociartifactstore/sociartifactstore_darwin.go
+++ b/enterprise/server/sociartifactstore/sociartifactstore_darwin.go
@@ -8,7 +8,7 @@ import (
 
 	"github.com/buildbuddy-io/buildbuddy/server/environment"
 	"github.com/buildbuddy-io/buildbuddy/server/util/status"
-	"github.com/google/go-containerregistry/pkg/v1"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 
 	repb "github.com/buildbuddy-io/buildbuddy/proto/remote_execution"
 	socipb "github.com/buildbuddy-io/buildbuddy/proto/soci"
diff --git a/enterprise/server/sociartifactstore/sociartifactstore_test.go b/enterprise/server/sociartifactstore/sociartifactstore_test.go
index 84fb56bf67..057153f0e8 100644
--- a/enterprise/server/sociartifactstore/sociartifactstore_test.go
+++ b/enterprise/server/sociartifactstore/sociartifactstore_test.go
@@ -18,7 +18,7 @@ import (
 	"github.com/buildbuddy-io/buildbuddy/server/util/prefix"
 	"github.com/buildbuddy-io/buildbuddy/server/util/proto"
 	"github.com/buildbuddy-io/buildbuddy/server/util/status"
-	"github.com/google/go-containerregistry/pkg/v1"
+	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/empty"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
 	"github.com/google/go-containerregistry/pkg/v1/stream"
diff --git a/enterprise/server/util/cpuset/numcpu_linux.go b/enterprise/server/util/cpuset/numcpu_linux.go
index bd7f3b9cca..c5332c37d3 100644
--- a/enterprise/server/util/cpuset/numcpu_linux.go
+++ b/enterprise/server/util/cpuset/numcpu_linux.go
@@ -3,8 +3,9 @@
 package cpuset
 
 import (
-	"github.com/prometheus/procfs"
 	"strconv"
+
+	"github.com/prometheus/procfs"
 )
 
 func GetCPUs() []cpuInfo {
diff --git a/enterprise/server/util/fieldgetter/fieldgetter_test.go b/enterprise/server/util/fieldgetter/fieldgetter_test.go
index c91cd28df8..bfbd53ec41 100644
--- a/enterprise/server/util/fieldgetter/fieldgetter_test.go
+++ b/enterprise/server/util/fieldgetter/fieldgetter_test.go
@@ -2,9 +2,10 @@ package fieldgetter_test
 
 import (
 	"fmt"
+	"testing"
+
 	"github.com/buildbuddy-io/buildbuddy/enterprise/server/util/fieldgetter"
 	"github.com/stretchr/testify/assert"
-	"testing"
 )
 
 type Parent struct {

bduffany

LGTM - would be good to get some reviews from other folks as well before merging

btw, for vscode, I had to add this to my settings.json to get goimports-on-save to work properly:

  "[go]": {
    "editor.codeActionsOnSave": {
      "source.organizeImports": "explicit"
    }
  },

we could maybe add this to our vscode setup guide in the internal site (https://github.com/buildbuddy-io/buildbuddy-internal/blob/master/website/docs/development-environment.md)

bduffany · 2025-01-16T20:45:39Z

BUILD

+alias(
+    name = "goimports",
+    actual = "@org_golang_x_tools//cmd/goimports",
+    visibility = ["//visibility:public"],
+)
+
+genrule(
+    name = "goimports_bin",
+    outs = ["goimports.sh"],
+    cmd = """
+echo '#!/bin/bash' > $@
+echo 'exec $(location @org_golang_x_tools//cmd/goimports) "$$@"' >> $@
+chmod +x $@
+    """,
+    tools = ["@org_golang_x_tools//cmd/goimports"],
+)


(nit - instead of using $(location) does it work if we run @org_golang_x_tools//cmd/goimports directly, similar to how we run @npm//prettier/bin:prettier which is also an external dep?)

That absolutely works! Thanks. Lol, goimports.sh was failing to find goimports when run in CI.

Scratch that. What pushed me to writing the goimports.sh rule in the first place is that running bazel run //:goimports would execute outside the current working directory. I don't know how to get the absolute path of goimports without calling $(location ...).

…pendency of checkstyle

iain-macdonald

I mostly just care about not having to think about stuff like this too much, so this seems like a good change in that regard. That said, we have historically grouped inputs into three groups: library-level ones (like "fmt"), unaliased imports, and then aliased imports, with each group separated by an empty line, like this:

buildbuddy/tools/cas/cas.go

Lines 4 to 25 in 3567984

    
           "bytes" 
        
           "context" 
        
           "flag" 
        
           "os" 
        
           "strings" 
        
           "time" 
        
           "github.com/buildbuddy-io/buildbuddy/server/remote_cache/cachetools" 
        
           "github.com/buildbuddy-io/buildbuddy/server/remote_cache/digest" 
        
           "github.com/buildbuddy-io/buildbuddy/server/util/grpc_client" 
        
           "github.com/buildbuddy-io/buildbuddy/server/util/log" 
        
           "github.com/buildbuddy-io/buildbuddy/server/util/proto" 
        
           "github.com/buildbuddy-io/buildbuddy/server/util/status" 
        
           "github.com/mattn/go-isatty" 
        
           "google.golang.org/grpc/metadata" 
        
           "google.golang.org/protobuf/encoding/protojson" 
        
           bbspb "github.com/buildbuddy-io/buildbuddy/proto/buildbuddy_service" 
        
           capb "github.com/buildbuddy-io/buildbuddy/proto/cache" 
        
           repb "github.com/buildbuddy-io/buildbuddy/proto/remote_execution" 
        
           rspb "github.com/buildbuddy-io/buildbuddy/proto/resource" 
        
           bspb "google.golang.org/genproto/googleapis/bytestream"

Do you know if there's a way to get goimports to do that automatically?

tylerwilliams · 2025-01-16T21:54:22Z

I'm cool with this if that's the diff!

2 minor questions/nits I have are:

what's with the v1 named import thing it's doing, seems kinda fucky?
I would like to keep the imports grouped the way we have them e.g.:

import (
  builtins
  
  custom packages

  alias ... aliased packages
)

I don't think goimports will break this, but I have seen some editors group aliases and non-aliased stuff together which is 🤮 This change won't break anything here, right?

vadimberezniker · 2025-01-16T22:58:17Z

As mentioned in your other PR, I would manually alias the v1 import to something else since v1 is not a descriptive name. You can't even tell what module it's related to w/o looking at the imports.

iain-macdonald · 2025-01-16T23:40:22Z

As mentioned in your other PR, I would manually alias the v1 import to something else since v1 is not a descriptive name. You can't even tell what module it's related to w/o looking at the imports.

That's what the Google Style Guide recommends: https://google.github.io/styleguide/go/decisions.html#imports so seems reasonable to me that we just adopt that convention.

BUILD

sluongng · 2025-01-17T12:21:57Z

tools/checkstyle/checkstyle.sh

 run GoFormat \
  "$GOFMT_PATH" -d .

+run GoImports \
+  "$GOIMPORTS_PATH" -d .


Nit: https://pkg.go.dev/golang.org/x/tools/cmd/goimports

In addition to fixing imports, goimports also formats your code in the same style as gofmt so it can be used as a replacement for your editor's gofmt-on-save hook.

So we can replace GoFormat with GoImports

I want to test it, but yes, good point!

Co-authored-by: Fabian Meumertzheim <[email protected]>

dan-stowell · 2025-01-22T19:11:44Z

what's with the v1 named import thing it's doing, seems kinda fucky?

Does seem wrong. This blog post mentions packages named after major versions, saying that you should just use the real package name. Not sure why goimports does this.

If I explicitly alias to gcr, though, goimports does not complain or try to change it.

I would like to keep the imports grouped the way we have them e.g.:
import (
  builtins
  
  custom packages

  alias ... aliased packages
)

goimports will not break this grouping. However, it won't group imports this way on its own. There are two other tools, gci and goimports-reviser that promise to have a separate group for aliased packages, but they also seem to promise other stuff.

dan-stowell added 4 commits January 16, 2025 12:00

can just alias goimports, thanks perplexity

6674602

add //:goimports.sh command to organize imports

51187f3

add goimports to checkstyle.sh

0428f79

goimports -d . to actually trip diff in checkstyle.sh

8c62c0a

dan-stowell requested a review from bduffany January 16, 2025 20:09

bduffany approved these changes Jan 16, 2025

View reviewed changes

bduffany reviewed Jan 16, 2025

View reviewed changes

bduffany requested review from tylerwilliams, iain-macdonald and vadimberezniker January 16, 2025 21:09

checkstyle in CI cannot find goimports binary, make it an explicit de…

138b017

…pendency of checkstyle

iain-macdonald reviewed Jan 16, 2025

View reviewed changes

vadimberezniker approved these changes Jan 16, 2025

View reviewed changes

fmeum approved these changes Jan 17, 2025

View reviewed changes

BUILD Outdated Show resolved Hide resolved

sluongng reviewed Jan 17, 2025

View reviewed changes

Update BUILD

b3acbb2

Co-authored-by: Fabian Meumertzheim <[email protected]>

iain-macdonald approved these changes Jan 22, 2025

View reviewed changes

dan-stowell added 5 commits January 22, 2025 16:41

Merge branch 'master' into dan/goimports

e004c1c

Merge branch 'master' into dan/goimports

e68344a

echo -> cat, rootpath -> execpath

52ec107

goimports overwrite

6d4220a

create a goimports.sh similar to prettier.sh

406b733

dan-stowell merged commit d5eef28 into master Jan 23, 2025
15 checks passed

dan-stowell deleted the dan/goimports branch January 23, 2025 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce consistent Go import organization using `goimports` #8198

Enforce consistent Go import organization using `goimports` #8198

dan-stowell commented Jan 16, 2025

bduffany left a comment

bduffany Jan 16, 2025 •

edited

Loading

dan-stowell Jan 16, 2025

dan-stowell Jan 16, 2025

iain-macdonald left a comment

tylerwilliams commented Jan 16, 2025

vadimberezniker commented Jan 16, 2025

iain-macdonald commented Jan 16, 2025

sluongng Jan 17, 2025

dan-stowell Jan 22, 2025

dan-stowell commented Jan 22, 2025

	"bytes"
	"context"
	"flag"
	"os"
	"strings"
	"time"

	"github.com/buildbuddy-io/buildbuddy/server/remote_cache/cachetools"
	"github.com/buildbuddy-io/buildbuddy/server/remote_cache/digest"
	"github.com/buildbuddy-io/buildbuddy/server/util/grpc_client"
	"github.com/buildbuddy-io/buildbuddy/server/util/log"
	"github.com/buildbuddy-io/buildbuddy/server/util/proto"
	"github.com/buildbuddy-io/buildbuddy/server/util/status"
	"github.com/mattn/go-isatty"
	"google.golang.org/grpc/metadata"
	"google.golang.org/protobuf/encoding/protojson"

	bbspb "github.com/buildbuddy-io/buildbuddy/proto/buildbuddy_service"
	capb "github.com/buildbuddy-io/buildbuddy/proto/cache"
	repb "github.com/buildbuddy-io/buildbuddy/proto/remote_execution"
	rspb "github.com/buildbuddy-io/buildbuddy/proto/resource"
	bspb "google.golang.org/genproto/googleapis/bytestream"

Enforce consistent Go import organization using goimports #8198

Enforce consistent Go import organization using goimports #8198

Conversation

dan-stowell commented Jan 16, 2025

bduffany left a comment

Choose a reason for hiding this comment

bduffany Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

dan-stowell Jan 16, 2025

Choose a reason for hiding this comment

dan-stowell Jan 16, 2025

Choose a reason for hiding this comment

iain-macdonald left a comment

Choose a reason for hiding this comment

tylerwilliams commented Jan 16, 2025

vadimberezniker commented Jan 16, 2025

iain-macdonald commented Jan 16, 2025

sluongng Jan 17, 2025

Choose a reason for hiding this comment

dan-stowell Jan 22, 2025

Choose a reason for hiding this comment

dan-stowell commented Jan 22, 2025

Enforce consistent Go import organization using `goimports` #8198

Enforce consistent Go import organization using `goimports` #8198

bduffany Jan 16, 2025 •

edited

Loading