Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
fishy opened this issue Nov 13, 2024 · 14 comments
Open

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

fishy opened this issue Nov 13, 2024 · 14 comments

Comments

@fishy
Copy link

fishy commented Nov 13, 2024

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 (fsnotify/fsnotify#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
Copy link
Member

fmeum commented Nov 13, 2024

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
Copy link
Author

fishy commented Nov 13, 2024

@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
Copy link
Member

fmeum commented Nov 13, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

git bisect shows that bf8c993 is the first bad commit, which is probably not that useful 😥

@fishy
Copy link
Author

fishy commented Nov 14, 2024

Actually fsnotify is also used by gazelle and that's the commit bumped it to 1.8.0: bf8c993#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R11

I wonder if that's the issue.

@fishy
Copy link
Author

fishy commented Nov 14, 2024

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
Copy link
Member

fmeum commented Nov 14, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

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
Copy link
Member

fmeum commented Nov 14, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

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
Copy link
Member

fmeum commented Nov 14, 2024

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
Copy link
Author

fishy commented Nov 14, 2024

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 👍

fmeum added a commit that referenced this issue Nov 15, 2024
**What type of PR is this?**

Bug fix

**What package or component does this PR mostly affect?**

`go_deps`

**What does this PR do? Why is it needed?**

With Bzlmod now being the primary way Gazelle is used, we should no
longer update the Go dependencies with each release as this just reduces
the amount of flexibility afforded to dependent projects. Thanks to MVS,
they can easily force newer versions.

Also test with Bazel 8.0.0rc2 in BCR presubmit.

**Which issues(s) does this PR fix?**

Work towards #1977

**Other notes for review**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants