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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 6 commits into from

Conversation

Buzz-Lightyear
Copy link
Contributor

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

fmeum
fmeum reviewed Oct 8, 2024
View reviewed changes
internal/bzlmod/go_deps.bzl
@@ -592,6 +592,7 @@ def _go_deps_impl(module_ctx):
),
)

repos_processed = {}
for path, module in module_resolutions.items():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@stefanpenner stefanpenner Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",),)))

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Certainly @fmeum

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1954 @fmeum

@Buzz-Lightyear Buzz-Lightyear force-pushed the srini/gomoddup branch 2 times, most recently from c8b68ab to cdfa3d5 Compare October 9, 2024 14:32
stefanpenner
stefanpenner reviewed Oct 9, 2024
View reviewed changes
internal/bzlmod/utils.bzl
segments = reversed(path_segments[0].split(".")) + path_segments[1:]
candidate_name = "_".join(segments).replace("-", "_")

def _encode_case(c):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Buzz-Lightyear Buzz-Lightyear mentioned this pull request Oct 10, 2024
Merged
fmeum added a commit that referenced this pull request Oct 11, 2024
@Buzz-Lightyear @fmeum
go_deps should fail with a clear message when 2 modules that only dif…
8bbeae7 
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stefanpenner stefanpenner stefanpenner left review comments

@linzhp linzhp linzhp left review comments

@fmeum fmeum fmeum left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Buzz-Lightyear @stefanpenner @linzhp @fmeum