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

Show file tree
Hide file tree
Changes from 4 commits
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
18 changes: 7 additions & 11 deletions internal/bzlmod/go_deps.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ load(
"format_rule_call",
"get_directive_value",
"with_replaced_or_new_fields",
"repo_name",
)

visibility("//")
Expand Down Expand Up @@ -163,12 +164,6 @@ def _get_patch_args(path, module_overrides):
override = _get_override_or_default(module_overrides, struct(), {}, path, None, "patch_strip")
return ["-p{}".format(override)] if override else []

def _repo_name(importpath):
path_segments = importpath.split("/")
segments = reversed(path_segments[0].split(".")) + path_segments[1:]
candidate_name = "_".join(segments).replace("-", "_")
return "".join([c.lower() if c.isalnum() else "_" for c in candidate_name.elems()])

def _is_dev_dependency(module_ctx, tag):
if hasattr(tag, "_is_dev_dependency"):
# Synthetic tags generated from go_deps.from_file have this "hidden" attribute.
Expand Down Expand Up @@ -499,9 +494,9 @@ def _go_deps_impl(module_ctx):
if module.is_root and not module_tag.indirect:
root_versions[module_tag.path] = raw_version
if _is_dev_dependency(module_ctx, module_tag):
root_module_direct_dev_deps[_repo_name(module_tag.path)] = None
root_module_direct_dev_deps[repo_name(module_tag.path)] = None
else:
root_module_direct_deps[_repo_name(module_tag.path)] = None
root_module_direct_deps[repo_name(module_tag.path)] = None

version = semver.to_comparable(raw_version)
previous = paths.get(module_tag.path)
Expand All @@ -523,7 +518,7 @@ def _go_deps_impl(module_ctx):
local_path = replacement.local_path

module_resolutions[module_tag.path] = struct(
repo_name = _repo_name(module_tag.path),
repo_name = repo_name(module_tag.path),
version = version,
raw_version = raw_version,
to_path = to_path,
Expand Down Expand Up @@ -595,13 +590,14 @@ def _go_deps_impl(module_ctx):
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.
root_module_direct_deps.pop(_repo_name(path), default = None)
root_module_direct_dev_deps.pop(_repo_name(path), default = None)
root_module_direct_deps.pop(repo_name(path), default = None)
root_module_direct_dev_deps.pop(repo_name(path), default = None)
continue
if getattr(module_ctx, "is_isolated", False) and path in _SHARED_REPOS:
# Do not create a go_repository for a dep shared with the non-isolated instance of
# go_deps.
continue

go_repository_args = {
"name": module.repo_name,
# Compared to the name attribute, the content of this attribute does not go through repo
Expand Down
11 changes: 11 additions & 0 deletions internal/bzlmod/utils.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,17 @@ def with_replaced_or_new_fields(_struct, **replacements):

return struct(**new_struct_assignments)

def repo_name(importpath):
path_segments = importpath.split("/")
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

! isn't valid in repo names though.

"""Repo names end up as directory names, therefore we can't rely on case to distinguish importpaths that only differ in case"""
return "_" + c.lower() if c.isupper() else c

return "".join([_encode_case(c) if c.isalnum() else "_" for c in candidate_name.elems()])

def extension_metadata(
module_ctx,
*,
Expand Down
2 changes: 1 addition & 1 deletion tests/bcr/go_mod/MODULE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ use_repo(
go_deps,
"com_github_bazelbuild_buildtools",
"com_github_bmatcuk_doublestar_v4",
"com_github_datadog_sketches_go",
"com_github__data_dog_sketches_go",
"com_github_envoyproxy_protoc_gen_validate",
"com_github_fmeum_dep_on_gazelle",
"com_github_google_go_jsonnet",
Expand Down
2 changes: 1 addition & 1 deletion tests/bcr/go_mod/pkg/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ go_test(
"@circl//dh/x25519",
"@com_github_bazelbuild_buildtools//labels:go_default_library",
"@com_github_bmatcuk_doublestar_v4//:doublestar",
"@com_github_datadog_sketches_go//ddsketch",
"@com_github__data_dog_sketches_go//ddsketch",
"@com_github_envoyproxy_protoc_gen_validate//validate",
"@com_github_fmeum_dep_on_gazelle//:dep_on_gazelle",
"@com_github_google_go_jsonnet//:go-jsonnet",
Expand Down
2 changes: 1 addition & 1 deletion tests/bcr/go_work/MODULE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ use_repo(
"com_github_99designs_gqlgen",
"com_github_bazelbuild_buildtools",
"com_github_bmatcuk_doublestar_v4",
"com_github_datadog_sketches_go",
"com_github__data_dog_sketches_go",
"com_github_envoyproxy_protoc_gen_validate",
"com_github_fmeum_dep_on_gazelle",
"com_github_google_safetext",
Expand Down
2 changes: 1 addition & 1 deletion tests/bcr/go_work/pkg/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ go_test(
"@circl//dh/x25519",
"@com_github_bazelbuild_buildtools//labels:go_default_library",
"@com_github_bmatcuk_doublestar_v4//:doublestar",
"@com_github_datadog_sketches_go//ddsketch",
"@com_github__data_dog_sketches_go//ddsketch",
"@com_github_envoyproxy_protoc_gen_validate//validate",
"@com_github_fmeum_dep_on_gazelle//:dep_on_gazelle",
"@com_github_google_safetext//yamltemplate",
Expand Down
12 changes: 11 additions & 1 deletion tests/bzlmod/utils_test.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
load("@bazel_skylib//lib:unittest.bzl", "asserts", "unittest")
load("//internal/bzlmod:utils.bzl", "with_replaced_or_new_fields")
load("//internal/bzlmod:utils.bzl", "with_replaced_or_new_fields", "repo_name")

_BEFORE_STRUCT = struct(
direct = True,
Expand All @@ -25,8 +25,18 @@ def _with_replaced_or_new_fields_test_impl(ctx):

with_replaced_or_new_fields_test = unittest.make(_with_replaced_or_new_fields_test_impl)

def _repo_name_test_impl(ctx):
env = unittest.begin(ctx)
asserts.equals(env, "com_github_shurcoo_l_githubv4", repo_name("github.com/shurcooL/githubv4"))
asserts.equals(env, "com_github_shurcool_githubv4", repo_name("github.com/shurcool/githubv4"))
asserts.equals(env, "com_github__d_a_t_a__d_o_g_go_sqlmock", repo_name("github.com/DATA-DOG/go-sqlmock"))
return unittest.end(env)

repo_name_test = unittest.make(_repo_name_test_impl)

def utils_test_suite(name):
unittest.suite(
name,
with_replaced_or_new_fields_test,
repo_name_test,
)