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

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

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7df94f2
Don't hard fail if there's a second request to bazelize the same Go m…
Buzz-Lightyear Oct 8, 2024
9ca14e9
Updating repo naming scheme to distinguish importpaths that only diff…
Buzz-Lightyear Oct 9, 2024
acb6e24
Using underscore for better readability
Buzz-Lightyear Oct 9, 2024
cd77474
Adding tests
Buzz-Lightyear Oct 9, 2024
b9a7f7b
Sync with trunk
Buzz-Lightyear Oct 10, 2024
c94aca1
Merge fix
Buzz-Lightyear Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/bzlmod/go_deps.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

if hasattr(module, "module_name"):
# Do not create a go_repository for a Go module provided by a bazel_dep.
Expand All @@ -602,6 +603,10 @@ def _go_deps_impl(module_ctx):
# Do not create a go_repository for a dep shared with the non-isolated instance of
# go_deps.
continue
if module.repo_name in repos_processed:
continue

repos_processed[module.repo_name] = True
go_repository_args = {
"name": module.repo_name,
# Compared to the name attribute, the content of this attribute does not go through repo
Expand Down