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 381 forks source link

Hard failing if module versions aren't aligned between bazel dep and go.mod for a given module #1957

Closed Buzz-Lightyear closed 1 month ago

Buzz-Lightyear commented 1 month ago

What type of PR is this?

Bug fix/Change in behavior

What package or component does this PR mostly affect?

go_deps

What does this PR do? Why is it needed? If a given Go module is requested both via go.mod and directly as a bazel_dep, this PR will change Gazelle's behavior to hard fail as opposed to use the Bazel dep so long as it's a higher version than the one in go.mod

Ideally, Gazelle should fail and alert the developer to the mismatch so that they may align the versions in order to ensure that Native Go builds use the same SoT as Bazel builds.

Tested locally:

>bazel build //...
INFO: Invocation ID: 09d43bbd-a6b9-4e9f-a0b0-094be01359bf
ERROR: Traceback (most recent call last):
        File "/private/var/tmp/_bazel_smuthu/572bd1d287d7ec2c900673904e0dbb38/external/gazelle~/internal/bzlmod/go_deps.bzl", line 571, column 17, in _go_deps_impl
                fail("\n\nMismatch between versions requested for module {module}\nBazel dependency version requested in MODULE.bazel: {bazel_dep_version}\nGo module version requested in go.mod: {go_module_version}\nPlease resolve this mismatch to prevent discrepancies between native Go and Bazel builds\n\n".format(
Error in fail: 

Mismatch between versions requested for module github.com/cloudflare/circl
Bazel dependency version requested in MODULE.bazel: 1.3.8
Go module version requested in go.mod: 1.3.7
Please resolve this mismatch to prevent discrepancies between native Go and Bazel builds

ERROR: error evaluating module extension go_deps in @@gazelle~//:extensions.bzl
INFO: Elapsed time: 0.195s
INFO: 0 processes.
ERROR: Build did NOT complete successfully
INFO: Build Event Protocol files produced successfully.
FAILED: 
    Fetching module extension go_deps in @@gazelle~//:extensions.bzl; starting

Which issues(s) does this PR fix? No issue currently but happy to create one if necessary

Bazel Slack Discussion

fmeum commented 1 month ago

@Buzz-Lightyear Does this check also trigger for rules_go and bazel-gazelle themselves? If so, that could be unnecessarily breaking for users.

I know it's not what I said first, but could we use outdated_direct_dep_printer instead of fail? That results in a warning by default but can be opted into an error, which is considerably less breaking.

Buzz-Lightyear commented 1 month ago

@Buzz-Lightyear Does this check also trigger for rules_go and bazel-gazelle themselves? If so, that could be unnecessarily breaking for users.

I'm unsure if I understand the question but if the ask is whether this check would trigger for users of Gazelle + rules_go that aren't using the go_deps module extension, I don't think it will since the check is part of the module extension implementation. But I could be missing something, do let me know!

I know it's not what I said first, but could we use outdated_direct_dep_printer instead of fail? That results in a warning by default but can be opted into an error, which is considerably less breaking. I don't disagree but I'm still leaning sticking with a hard failure. I say this because it makes the contract clearer and should there be misalignment, the fix is in user's control. They can opt to quick fix by changing the bazel_dep or choose to natively replace/update the module in question.

My other main concern is that users may tend to ignore warnings altogether and not differentiate between what each warning is trying to tell them. But then again, that's on the user. Ultimately I'm happy to defer to your preference here.

fmeum commented 1 month ago

My concern is about the Bazel modules rules_go and gazelle, which are also Go modules and thus probably fall under this check. It requires users to always update bazel_dep and go.mod entry for these rulesets in lockstep.

That's ultimately still good, but makes this breakage more common than I first thought. Since we would fail now in a case in which we previously hadn't even shown a warning, I would prefer to do this in two steps and start with a configurable indication that something is wrong (hence the outdated_direct_deps_printer). We could then tighten the check in a future release.

Buzz-Lightyear commented 1 month ago

My concern is about the Bazel modules rules_go and gazelle, which are also Go modules and thus probably fall under this check. It requires users to always update bazel_dep and go.mod entry for these rulesets in lockstep.

That's ultimately still good, but makes this breakage more common than I first thought. Since we would fail now in a case in which we previously hadn't even shown a warning, I would prefer to do this in two steps and start with a configurable indication that something is wrong (hence the outdated_direct_deps_printer). We could then tighten the check in a future release.

That makes perfect sense, didn't realize folks pull in rules_go as a Go module as well. Let me create a new PR to change this to a warning.

Buzz-Lightyear commented 1 month ago

@fmeum https://github.com/bazel-contrib/bazel-gazelle/pull/1963