Multiple go_binary rules are not fixed/updated

[tldr-darren]

What version of gazelle are you using?

v0.23.0

What version of rules_go are you using?

v0.27.0

What version of Bazel are you using?

4.0.0

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

Yes

What operating system and processor architecture are you using?

Ubuntu 20.04

What did you do?

Start with the following minimal directory structure and files:
base_dir
├── foo
│ ├── BUILD
│ └── foo.go
├── BUILD
├── bar.go
├── baz.go
└── WORKSPACE

foo/BUILD

load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
    name = "foo",
    srcs = ["foo.go"],
    importpath = "foo/foo",
    visibility = ["//visibility:public"],
)

foo/foo.go

package foo

import (
	"fmt"
)

func Foo() {
	fmt.Println("foo")
}

BUILD

load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
load("@bazel_gazelle//:def.bzl", "gazelle")

# gazelle:build_file_name BUILD
# gazelle:prefix foo
gazelle(
    name = "gazelle",
    command = "fix",
)

go_binary(
    name = "bar",
    embed = [":bar_lib"],
    visibility = ["//visibility:public"],
)

go_library(
    name = "bar_lib",
    srcs = ["bar.go"],
    importpath = "foo",
    visibility = ["//visibility:private"],
    deps = ["//foo"],
)

go_binary(
    name = "baz",
    embed = [":baz_lib"],
    visibility = ["//visibility:public"],
)

go_library(
    name = "baz_lib",
    srcs = ["baz.go"],
    importpath = "foo",
    visibility = ["//visibility:private"],
    deps = ["//foo"],
)

bar.go & baz.go

package main

import (
	"foo/foo"
)

func main() {
	foo.Foo()
}

WORKSPACE

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

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

load("@io_bazel_rules_go//go:deps.bzl", "go_register_toolchains", "go_rules_dependencies")

go_rules_dependencies()

go_register_toolchains(version = "1.16")

http_archive(
    name = "com_google_protobuf",
    sha256 = "dd513a79c7d7e45cbaeaf7655289f78fd6b806e52dbbd7018ef4e3cf5cff697a",
    strip_prefix = "protobuf-3.15.8",
    urls = ["https://github.com/protocolbuffers/protobuf/archive/v3.15.8.zip"],
)

load("@com_google_protobuf//:protobuf_deps.bzl", "protobuf_deps")

protobuf_deps()

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

load("@bazel_gazelle//:deps.bzl", "gazelle_dependencies")

gazelle_dependencies()

Reproduction steps:

1. Run bazel build ..., all builds fine
2. Delete the two deps = ["//foo"], from rules //bar_lib and baz_lib in BUILD
3. Run bazel run //:gazelle

What did you expect to see?

The 2 deps = ["//foo"], lines reinserted into rules //bar_lib and baz_lib.

What did you see instead?

Nothing, no file change. Although Gazelle will fix formatting errors, so it is running. It just doesn't do any meaningful changes.

Proof of homework

I see Jay's comment in #826 referencing "Go already had a strong set of conventions", but if this is the case it is unclear what convention I am running afoul of. Gazelle is not printing a warning or error of any kind. The output is:

$ bazel run //:gazelle
INFO: Analyzed target //:gazelle (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
Target //:gazelle up-to-date:
  bazel-bin/gazelle-runner.bash
  bazel-bin/gazelle
INFO: Elapsed time: 0.045s, Critical Path: 0.00s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Build completed successfully, 1 total action

Questions

1. Are multiple go_binaries in the same directory simply not allowed?
2. If the answer to the above is "no, they are allowed", do you have any insight on why Gazelle is not adding the deps = ["//foo"] attributes appropriately?

[tldr-darren]

Too late I realized the convention of 1 binary per directory. It makes sense Gazelle enforces this.

Is https://github.com/golang-standards/project-layout the source of truth for the canonical project layout Gazelle supports, as much as there is one? If so, would it be ok to explicitly state that in the docs? I think it would save time for newcomers.

[achew22]

Funny you should mention that project. See golang-standards/project-layout#117 with the knowledge that rsc (intentionally not tagged) is in charge of go.

As a reference I would look to https://eli.thegreenplace.net/2019/simple-go-project-layout-with-modules/ Eli Bendersky. He is a Google employee, but I don't think he works on go proper. This is the repo layout that I use for everything.

[peterbourgon]

the convention of 1 binary per directory

Not merely a convention, but a requirement of the language. A directory may contain only a single package; package main may define only a single func main.

[tldr-darren]

@achew22 - Thanks for the pointer! I feel like the README should explicitly call out the conventions it supports(?) This is obvious to devs familiar with Go, but probably a gotcha for new comers? Explicitly stating the requirement could save some folks time.

@peterbourgon - Unpopular opinion: I kinda like how Bazel lets you break this convention if you really want to. Not saying Gazelle should officially support doing so.

[achew22]

@tldr-darren, I'm sorry you had a bad time with gazelle in go mode. That's obviously not ideal for the repo. That said, this tool has been in a usable state since 2017 and this is the first time I've heard this complaint. My rule of thumb is that I like to see things three times before I take action. That said, if you are really convinced this is a problem, we love documentation PRs and I'd be happy to work through that with you.

Just as a data point, are you a current/former user of glaze? Is this behavior expected from another tool that exists in the ecosystem?

[peterbourgon]

I kinda like how Bazel lets you break this convention if you really want to.

Again, it is not a convention, it is a requirement of the go toolchain and the language. If Bazel lets you build Go projects where one directory contains multiple package declarations, or is a package main that includes more than one func main, it is in error.

(I suppose there could be go run-esque exceptions, where one directory could hold different sets of .go files that were meant to be built as discrete units. But in that case the directory doesn't represent a coherent Go package.)

[tldr-darren]

That said, this tool has been in a usable state since 2017 and this is the first time I've heard this complaint.

No worries at all! Its all a learning process. I hope I didn't come off like I was knocking Gazelle, it is awesome. I was just trying to make it easier on the next person who comes along. I am quiet new to Golang/Bazel/Gazelle outside of Google's ecosystem. Maybe this is just a me problem :)

Just as a data point, are you a current/former user of glaze? Is this behavior expected from another tool that exists in the ecosystem?

Assuming you are referring to Google's internal tool that is analogous to Gazelle? Yeah, long time user :) But the only references I could find to glaze outside Google were bazelbuild/rules_go#15 and https://github.com/adamsiwiec/glaze. Is glaze well known in the Bazel/Gazelle ecosystem, or just referenced as the Google-internal analog?

Perhaps that prior expectation is what tripped me up, and why this problem might be limited to me. I don't mind closing this issue for that reason.

If Bazel lets you build Go projects where one directory contains multiple package declarations, or is a package main that includes more than one func main, it is in error.

Hypothesis: I believe what it is doing under the hood is treating each go_binary rule as if it were a unique in its directory, allowing for multiple files with package main and func main() to be in 1 directory as long as they are referenced by distinct go_binary rules. If there are multiple definitions of func main() within the same go_binary rule, I have to assume it throws an error.

I agree this is not a coherent go package. I believe it is just made possible by the Bazel ecosystem.

Going to close the issue because I have the answers I was looking for, thank you!

[jayconrod]

To chime in an "well actually" a bit about rules and conventions: the language spec says that:

An implementation may require that all source files for a package inhabit the same directory.

That leaves it ambiguous whether it's really necessary. go build certainly requires this, but it enforces many conventions outside of the language spec (everything related to modules or GOPATH). Bazel does not require this, nor does Blaze within Google (which predates go build). It's common within Google to have multiple go_binary and even go_library targets within a directory. Bazel rules_go had to support this to make the transition from google3 to open source easier for some projects.

That said, I'd encourage developers not to put multiple packages in the same directory, whether they are main package or not. The closer everyone stays to conventions, the easier life is for users, other developers, tools, and IDEs.

Gazelle translates go build conventions into Bazel build files, so most things that are outside go build conventions are out of scope for Gazelle (at least for the Go extension). Gazelle does try to respect manual changes though, which is why it won't complain about multiple go_binary targets; it will only update one.