bazel-contrib / bazel-gazelle

Gazelle is a Bazel build file generator for Bazel projects. It natively supports Go and protobuf, and it may be extended to support new languages and custom rule sets.
Apache License 2.0
1.21k stars 380 forks source link

Wrong version of go dependency being built into the binary #1977

Open fishy opened 3 days ago

fishy commented 3 days ago

What version of gazelle are you using?

0.40.0

What version of rules_go are you using?

0.50.1

What version of Bazel are you using?

7.4.1

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

Those are latest releases.

What operating system and processor architecture are you using?

linux/amd64

What did you do?

fsnotify v1.8.0 has a bug that would cause panic (https://github.com/fsnotify/fsnotify/issues/652)

We are seeing this bug in our system after we upgraded gazelle from 0.39.0 to 0.40.0:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0xa9caed]
goroutine 2672 [running]:
github.com/fsnotify/fsnotify.(*watches).removePath(0xc00085c720, {0xc00057eb80, 0x33})
        external/gazelle~~go_deps~com_github_fsnotify_fsnotify/backend_inotify.go:119 +0x22d
github.com/fsnotify/fsnotify.(*inotify).remove(0xc00059b400, {0xc00057eb80?, 0xc00085c6f0?})
        external/gazelle~~go_deps~com_github_fsnotify_fsnotify/backend_inotify.go:372 +0x28
github.com/fsnotify/fsnotify.(*inotify).Remove(0xc00059b400, {0xc00057eb80, 0x33})
        external/gazelle~~go_deps~com_github_fsnotify_fsnotify/backend_inotify.go:368 +0xea
github.com/fsnotify/fsnotify.(*Watcher).Remove(...)
        external/gazelle~~go_deps~com_github_fsnotify_fsnotify/fsnotify.go:332
github.com/reddit/baseplate.go/filewatcher/v2.(*Result[...]).watcherLoop.func1()
        external/gazelle~~go_deps~com_github_reddit_baseplate_go/filewatcher/v2/filewatcher.go:142 +0x2db
created by time.goFunc
        GOROOT/src/time/sleep.go:215 +0x2d

Despite that we actually should be using v1.7.0 (which does not have this bug):

$ git grep "fsnotify" go.mod
go.mod: github.com/fsnotify/fsnotify v1.7.0 // indirect

We know it's fsnotify v1.8.0 being built into the binary because the line number (backend_inotify.go:368) in the call stack only makes sense in v1.8.0: https://github.com/fsnotify/fsnotify/blob/v1.8.0/backend_inotify.go#L368

in v1.7.0 that line is a comment: https://github.com/fsnotify/fsnotify/blob/v1.7.0/backend_inotify.go#L368

Reverting gazelle back to 0.39.0 fixed the issue (we are no longer seeing the panics).

What did you expect to see?

What did you see instead?

fmeum commented 3 days ago

Could you try to make a reproducer? If that's too difficult, could you perhaps try Gazelle commits between 0.39.0 and 0.40.0 so that we get a better understanding of what's happening here?

fishy commented 3 days ago

@fmeum sure. what's the best way to use an untagged version (a version between 0.39.0 and 0.40.0) with bzlmod?

fmeum commented 3 days ago

You can check out the Gazelle repo locally and use a local_path_override to override the bazel_dep, then run a git bisect on your checkout.

fishy commented 3 days ago

git bisect shows that https://github.com/bazel-contrib/bazel-gazelle/commit/bf8c993756d01e8e45bde50ba22a78e5edbe64b4 is the first bad commit, which is probably not that useful 😥

fishy commented 3 days ago

Actually fsnotify is also used by gazelle and that's the commit bumped it to 1.8.0: https://github.com/bazel-contrib/bazel-gazelle/commit/bf8c993756d01e8e45bde50ba22a78e5edbe64b4#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R11

I wonder if that's the issue.

fishy commented 3 days ago

Here's a minimal reproducer:

$ cat BUILD.bazel 
load("@gazelle//:def.bzl", "gazelle")
load("@rules_go//go:def.bzl", "go_binary", "go_library")

gazelle(name = "gazelle")

go_library(
    name = "test-gazelle_lib",
    srcs = ["main.go"],
    importpath = "github.com/fishy/test-gazelle",
    visibility = ["//visibility:private"],
    deps = ["@com_github_fsnotify_fsnotify//:fsnotify"],
)

go_binary(
    name = "test-gazelle",
    embed = [":test-gazelle_lib"],
    pure = "on",
    visibility = ["//visibility:public"],
)

$ cat MODULE.bazel
bazel_dep(name = "rules_go", version = "0.50.1")
bazel_dep(name = "gazelle", version = "0.39.0")

go_sdk = use_extension("@rules_go//go:extensions.bzl", "go_sdk")
go_sdk.download(
    name = "go_sdk",
    version = "1.23.3",
)

go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")
use_repo(
    go_deps,
    "com_github_fsnotify_fsnotify",
)

$ cat WORKSPACE 
workspace(name = "test_gazelle")

$ cat go.mod 
module github.com/fishy/test-gazelle

go 1.23

require github.com/fsnotify/fsnotify v1.7.0

require golang.org/x/sys v0.4.0 // indirect

$ cat main.go 
package main

import (
        "fmt"

        "github.com/fsnotify/fsnotify"
)

func main() {
        _, err := fsnotify.NewWatcher()
        fmt.Println(err)
}

with gazelle at 0.39.0 or other versions before bf8c993, we got one sha256 of the binary:

$ bazel build :test-gazelle && sha256sum bazel-bin/test-gazelle_/test-gazelle
INFO: Analyzed target //:test-gazelle (9 packages loaded, 11617 targets configured).
INFO: Found 1 target...
Target //:test-gazelle up-to-date:
  bazel-bin/test-gazelle_/test-gazelle
INFO: Elapsed time: 2.017s, Critical Path: 0.51s
INFO: 6 processes: 2 internal, 4 linux-sandbox.
INFO: Build completed successfully, 6 total actions
99dac7994fcb2a47f0e37613635561a2540a8af4c1751ad05967cfdd3afdb620  bazel-bin/test-gazelle_/test-gazelle

with 0.40.0 or versions after bf8c993, we got a different sha256 of the binary (and also a debug warning):

$ bazel build :test-gazelle && sha256sum bazel-bin/test-gazelle_/test-gazelle
DEBUG: /home/fishy/.cache/bazel/_bazel_fishy/4f294d4db30625c66c976c79db594938/external/gazelle~/internal/bzlmod/go_deps.bzl:590:40: For Go module "github.com/fsnotify/fsnotify", the root module requires module version v1.7.0, but got v1.8.0 in the resolved dependency graph.
INFO: Analyzed target //:test-gazelle (47 packages loaded, 11985 targets configured).
INFO: Found 1 target...
Target //:test-gazelle up-to-date:
  bazel-bin/test-gazelle_/test-gazelle
INFO: Elapsed time: 2.039s, Critical Path: 0.53s
INFO: 6 processes: 2 internal, 4 linux-sandbox.
INFO: Build completed successfully, 6 total actions
cfbb7d70fe466936454f4697b7d0419e694633b4a3b7f73b3daa9f87d6967ef1  bazel-bin/test-gazelle_/test-gazelle
fmeum commented 2 days ago

Thanks for providing the reproducer. This does look like it's working as intended: Gazelle performs minimum version selection among all Bazel modules that declare Go deps and happens to be one of those. It does even show you a warning that you can optionally turn into an error with go_deps.config.

If you need a specific version, you can try using a replace directive in go.mod to force fs-notify to 1.7.0 or depend on a fixed version once released.

fishy commented 2 days ago

gazelle is not in the dependency tree of the go project (it's used by the build system), so I'm not sure why mixing gazelle's dependency to go project's dependency is working as intended. This at least makes go.mod of the go project no longer the source of truth: we specified 1.7.0 of fsnotify in go.mod but got 1.8.0 built into the go project.

fmeum commented 2 days ago

I can understand that this is somewhat unexpected, but Gazelle needs to be in the same dep tree so that you can safely build language plugins for it. We try to keep Gazelle's deps minimal and can also forego updating them in the future unless required for new functionality.

You can almost have go.mod be the source of truth (ignoring the exception of Gazelle) if you don't use any other Bazel modules that need Go deps and set go_deps.config(check_direct_dependencies = "error") to fail the build if go.mod doesn't reflect the view of your direct deps. But with Bazel as a build system, its dependency management will always trump that of the individual languages.

fishy commented 2 days ago

hmm go_deps.config(check_direct_dependencies = "error") works for the minimal reproducer (it fails when I use gazelle 0.40.0 and don't use go mod edit --replace to pin fsnotify to 1.7.0), but it doesn't produce error on the bigger project we originally encounter this issue. is there other gazelle options that would interfere with it? These are all the options we use:

go_deps = use_extension("@gazelle//:extensions.bzl", "go_deps")
go_deps.from_file(go_mod = "//:go.mod")
go_deps.config(check_direct_dependencies = "error")
go_deps.gazelle_default_attributes(
    directives = [
        "gazelle:proto disable",
    ],
)

use_repo(
    go_deps,
    ...
)
fishy commented 2 days ago

or maybe that only works for direct dependencies, and won't work for indirect dependencies? (in minimal reproducer it's direct dependency, for the bigger project it's an indirect one)

fishy commented 2 days ago

After changing the minimal reproducer to make fsnotify an indirect dependency, confirmed that go_deps.config(check_direct_dependencies = "error") no longer causes it to fail. I think that's a bug?

fmeum commented 2 days ago

After changing the minimal reproducer to make fsnotify an indirect dependency, confirmed that go_deps.config(check_direct_dependencies = "error") no longer causes it to fail. I think that's a bug?

The check only triggers for direct dependencies. The underlying idea is that if you care about the version of a particular dep, then it should be a direct dep (or replaced).

I have a PR out that downgrades Go deps and disables the automatic update for future releases. I will cut a patch release.

fishy commented 2 days ago

I was hoping to rely on go_deps.config(check_direct_dependencies = "error") to notify us that there's an unexpected version of a (direct or indirect) dependency pulled in, and we need to pin it with replace, otherwise it's very hard to notice that an unexpected version is pulled in unless it's a bug like fsnotify's that it caused a crash.

But the downgrade in gazelle itself can certainly help mitigate this issue to some level 👍