Don't hard fail if there's a second request to bazelize the same Go module #1949

Buzz-Lightyear · 2024-10-08T21:05:27Z

What type of PR is this?

Uncomment one line below and remove others.
Bug fix

What package or component does this PR mostly affect?

go_deps

What does this PR do? Why is it needed?
It's possible that a go.mod file may contain a (module, version) pair listed both as a direct and indirect requirement. In such cases, the module extension attempts to create a second go_repository rule which fails with an error like this:

ERROR: Traceback (most recent call last):
        File "/private/var/tmp/<some_path>/gazelle~/internal/bzlmod/go_deps.bzl", line 646, column 22, in _go_deps_impl
                go_repository(**go_repository_args)
Error in repository_rule: A repo named <some_repo> is already generated by this module extension at /private/var/tmp/<some_path>/gazelle~/internal/bzlmod/go_deps.bzl:646:22

This PR fixes such an error by simply keeping track of the modules that were already processed.

Which issues(s) does this PR fix?
I'm happy to create an issue but the instructions said I could skip if it's a small fix

…odule

fmeum · 2024-10-08T21:32:31Z

internal/bzlmod/go_deps.bzl

@@ -592,6 +592,7 @@ def _go_deps_impl(module_ctx):
                ),
            )

+    repos_processed = {}
    for path, module in module_resolutions.items():


This is a dict keyed by path, so I'm surprised to see duplicated modules. Could you share what path and module look like for a colliding pair?

I'm also curious, would it be possible to add the failing scenario to the existing test suite?
I would like to be sure that we aren't masking a upstream problem

It's possible that a go.mod file may contain a (module, version) pair listed

I see this is the scenario, do you think it would be possible to add it to something like https://github.com/bazelbuild/bazel-gazelle/blob/master/tests/bcr/go_mod/go.mod

Maybe @fmeum has a better idea to ensure we prevent regression

Sure, this is the full error message:

ERROR: Traceback (most recent call last): File "/private/var/tmp/_bazel_smuthu/572bd1d287d7ec2c900673904e0dbb38/external/gazelle~/internal/bzlmod/go_deps.bzl", line 646, column 22, in _go_deps_impl go_repository(**go_repository_args) Error in repository_rule: A repo named com_github_shurcool_githubv4 is already generated by this module extension at /private/var/tmp/_bazel_smuthu/572bd1d287d7ec2c900673904e0dbb38/external/gazelle~/internal/bzlmod/go_deps.bzl:646:22

In one of my go.mod(s), I see:

github.com/shurcool/githubv4 v0.0.0-20230424031643-6cea62ecd5a9

and

github.com/shurcooL/githubv4 v0.0.0-20230424031643-6cea62ecd5a9 // indirect

Output of go mod why:

macOS 14.6.1 smuthu in ~/Sandbox/nimbus on branch smuthu/onboard/k8s-cluster-bootstrap-temporal >go mod why -m github.com/shurcooL/githubv4 # github.com/shurcooL/githubv4 <Internal Package> github.com/shurcool/githubv4 github.com/shurcool/githubv4.test github.com/shurcooL/githubv4

path: github.com/shurcooL/githubv4
module: struct(local_path = None, raw_version = "0.0.0-20230424031643-6cea62ecd5a9", repo_name = "com_github_shurcool_githubv4", to_path = None, version = ((("", 0), ("", 0), ("", 0)), (("20230424031643-6cea62ecd5a9",),)))

Could you just update all your imports to use github.com/shurcooL/githubv4?

@linzhp Thanks for chiming in and yes, that does work for our specific use case. However in the future it's possible that this won't be an option and it's ideal to solve this for the generic use case.

If the solution is simply that Gazelle won't distinguish modules that only differ in case, then it's best to document that explicitly here. Currently, the extension just fails because it won't create duplicate go_repository rules.

Could you update this PR to instead fail with a clean error message mentioning the two colliding names and explaining that module names that differ only in casing and punctuation are not supported (yet)?

Certainly @fmeum

#1954 @fmeum

…er in case

stefanpenner · 2024-10-09T15:26:27Z

internal/bzlmod/utils.bzl

+    segments = reversed(path_segments[0].split(".")) + path_segments[1:]
+    candidate_name = "_".join(segments).replace("-", "_")
+
+    def _encode_case(c):


changing the encoding isn't a bad idea, but I do worry that this encoding itself could cause more conflicts. Is there a more conflict resistant encoding? Also, I believe any encoding change would require both the extension and gazelle BUILD.bazel generation to be in proper sync.

I'm not super convinced yet that supportive case sensitivity is the right call – obviously open to convincing, but sharing my current gut-feeling.

@stefanpenner @fmeum It looks like the behavior for modules pulled in via go_repository rules in WORKSPACE is X -> !x:

go_repository( name = "com_github_datadog_gosqlmock", importpath = "github.com/DATA-DOG/go-sqlmock", sum = "h1:OcvFkGmslmlZibjAjaHm3L//6LiuBgolP7OputlJIzU=", version = "v1.5.2", )

->

>ls -la /private/var/tmp/_bazel_smuthu/.../go/pkg/mod/github.com/\!d\!a\!t\!a-\!d\!o\!g/[email protected] | head total 576 dr-xr-xr-x 47 smuthu wheel 1504 Oct 15 10:55 . drwxr-xr-x 3 smuthu wheel 96 Oct 15 10:55 .. dr-xr-xr-x 3 smuthu wheel 96 Oct 15 10:55 .github -r--r--r-- 1 smuthu wheel 73 Oct 15 10:55 .gitignore -r--r--r-- 1 smuthu wheel 478 Oct 15 10:55 .travis.yml -r--r--r-- 1 smuthu wheel 1496 Oct 15 10:55 LICENSE -r--r--r-- 1 smuthu wheel 10371 Oct 15 10:55 README.md -r--r--r-- 1 smuthu wheel 511 Oct 15 10:55 argument.go -r--r--r-- 1 smuthu wheel 1445 Oct 15 10:55 argument_test.go

Should we use the same encoding?

! isn't valid in repo names though.

…fer in case are used (#1954) **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?** Replaces #1949 --------- Co-authored-by: Fabian Meumertzheim <[email protected]>

Don't hard fail if there's a second request to bazelize the same Go m…

7df94f2

…odule

fmeum reviewed Oct 8, 2024

View reviewed changes

Buzz-Lightyear added 2 commits October 9, 2024 09:17

Updating repo naming scheme to distinguish importpaths that only diff…

9ca14e9

…er in case

Using underscore for better readability

acb6e24

Buzz-Lightyear force-pushed the srini/gomoddup branch 2 times, most recently from c8b68ab to cdfa3d5 Compare October 9, 2024 14:32

Adding tests

cd77474

Buzz-Lightyear force-pushed the srini/gomoddup branch from cdfa3d5 to cd77474 Compare October 9, 2024 14:44

stefanpenner reviewed Oct 9, 2024

View reviewed changes

Buzz-Lightyear added 2 commits October 10, 2024 09:03

Sync with trunk

b9a7f7b

Merge fix

c94aca1

Buzz-Lightyear mentioned this pull request Oct 10, 2024

go_deps should fail with a clear message when 2 modules that only differ in case are used #1954

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't hard fail if there's a second request to bazelize the same Go module #1949

Don't hard fail if there's a second request to bazelize the same Go module #1949

Buzz-Lightyear commented Oct 8, 2024

fmeum Oct 8, 2024

stefanpenner Oct 8, 2024

stefanpenner Oct 8, 2024 •

edited

Loading

Buzz-Lightyear Oct 9, 2024

Buzz-Lightyear Oct 9, 2024

linzhp Oct 9, 2024

Buzz-Lightyear Oct 10, 2024

fmeum Oct 10, 2024

Buzz-Lightyear Oct 10, 2024

Buzz-Lightyear Oct 10, 2024

stefanpenner Oct 9, 2024

stefanpenner Oct 9, 2024

Buzz-Lightyear Oct 23, 2024

fmeum Oct 23, 2024

Don't hard fail if there's a second request to bazelize the same Go module #1949

Are you sure you want to change the base?

Don't hard fail if there's a second request to bazelize the same Go module #1949

Conversation

Buzz-Lightyear commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanpenner Oct 8, 2024 •

edited

Loading