From 7df94f22810e721b0c17f1800fd918f1cbe6af69 Mon Sep 17 00:00:00 2001 From: Buzz Lightyear Date: Tue, 8 Oct 2024 16:56:45 -0400 Subject: [PATCH 1/5] Don't hard fail if there's a second request to bazelize the same Go module --- internal/bzlmod/go_deps.bzl | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index dcd0db37c..11e92bc13 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -592,6 +592,7 @@ def _go_deps_impl(module_ctx): ), ) + repos_processed = {} for path, module in module_resolutions.items(): if hasattr(module, "module_name"): # Do not create a go_repository for a Go module provided by a bazel_dep. @@ -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 From 9ca14e9ec019b288ab8feff35c1fb21ea0517743 Mon Sep 17 00:00:00 2001 From: Buzz-Lightyear Date: Wed, 9 Oct 2024 09:16:23 -0400 Subject: [PATCH 2/5] Updating repo naming scheme to distinguish importpaths that only differ in case --- internal/bzlmod/go_deps.bzl | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index 11e92bc13..d6cc7455a 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -167,7 +167,12 @@ 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 _encode_case(c): + """Repo names end up as directory names, therefore we can't rely on case to distinguish importpaths that only differ in case""" + return "1" + c.lower() if c.isupper() else c + + return "".join([_encode_case(c) if c.isalnum() else "_" for c in candidate_name.elems()]) def _is_dev_dependency(module_ctx, tag): if hasattr(tag, "_is_dev_dependency"): @@ -592,7 +597,6 @@ def _go_deps_impl(module_ctx): ), ) - repos_processed = {} for path, module in module_resolutions.items(): if hasattr(module, "module_name"): # Do not create a go_repository for a Go module provided by a bazel_dep. @@ -603,10 +607,7 @@ 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 From acb6e24209d9d57f3afd1fcaf98eb8b2efa7fac4 Mon Sep 17 00:00:00 2001 From: Buzz-Lightyear Date: Wed, 9 Oct 2024 09:22:06 -0400 Subject: [PATCH 3/5] Using underscore for better readability --- internal/bzlmod/go_deps.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index d6cc7455a..43d9bdf53 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -170,7 +170,7 @@ def _repo_name(importpath): def _encode_case(c): """Repo names end up as directory names, therefore we can't rely on case to distinguish importpaths that only differ in case""" - return "1" + c.lower() if c.isupper() else c + return "_" + c.lower() if c.isupper() else c return "".join([_encode_case(c) if c.isalnum() else "_" for c in candidate_name.elems()]) From cd774742c6e34ccb5a21c037fdef9e72e7a9f23f Mon Sep 17 00:00:00 2001 From: Buzz-Lightyear Date: Wed, 9 Oct 2024 10:42:39 -0400 Subject: [PATCH 4/5] Adding tests --- internal/bzlmod/go_deps.bzl | 22 ++++++---------------- internal/bzlmod/utils.bzl | 11 +++++++++++ tests/bcr/go_mod/MODULE.bazel | 2 +- tests/bcr/go_mod/pkg/BUILD.bazel | 2 +- tests/bcr/go_work/MODULE.bazel | 2 +- tests/bcr/go_work/pkg/BUILD.bazel | 2 +- tests/bzlmod/utils_test.bzl | 12 +++++++++++- 7 files changed, 32 insertions(+), 21 deletions(-) diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index 43d9bdf53..d30c84e28 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -28,6 +28,7 @@ load( "format_rule_call", "get_directive_value", "with_replaced_or_new_fields", + "repo_name", ) visibility("//") @@ -163,17 +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("-", "_") - - def _encode_case(c): - """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 _is_dev_dependency(module_ctx, tag): if hasattr(tag, "_is_dev_dependency"): # Synthetic tags generated from go_deps.from_file have this "hidden" attribute. @@ -504,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) @@ -528,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, @@ -600,8 +590,8 @@ def _go_deps_impl(module_ctx): for path, module in module_resolutions.items(): 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 diff --git a/internal/bzlmod/utils.bzl b/internal/bzlmod/utils.bzl index 53d1cad5f..9f64f5acd 100644 --- a/internal/bzlmod/utils.bzl +++ b/internal/bzlmod/utils.bzl @@ -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): + """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, *, diff --git a/tests/bcr/go_mod/MODULE.bazel b/tests/bcr/go_mod/MODULE.bazel index 7bceaa326..43fb071cd 100644 --- a/tests/bcr/go_mod/MODULE.bazel +++ b/tests/bcr/go_mod/MODULE.bazel @@ -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", diff --git a/tests/bcr/go_mod/pkg/BUILD.bazel b/tests/bcr/go_mod/pkg/BUILD.bazel index 66dc22d24..56377290e 100644 --- a/tests/bcr/go_mod/pkg/BUILD.bazel +++ b/tests/bcr/go_mod/pkg/BUILD.bazel @@ -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", diff --git a/tests/bcr/go_work/MODULE.bazel b/tests/bcr/go_work/MODULE.bazel index 0e4220acb..5b49587d7 100644 --- a/tests/bcr/go_work/MODULE.bazel +++ b/tests/bcr/go_work/MODULE.bazel @@ -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", diff --git a/tests/bcr/go_work/pkg/BUILD.bazel b/tests/bcr/go_work/pkg/BUILD.bazel index 21333bf5b..b5da2c9bc 100644 --- a/tests/bcr/go_work/pkg/BUILD.bazel +++ b/tests/bcr/go_work/pkg/BUILD.bazel @@ -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", diff --git a/tests/bzlmod/utils_test.bzl b/tests/bzlmod/utils_test.bzl index 87cd9f98b..fb7c760c2 100644 --- a/tests/bzlmod/utils_test.bzl +++ b/tests/bzlmod/utils_test.bzl @@ -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, @@ -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, ) From c94aca169d403e86ef70815825fd9c672cc0509e Mon Sep 17 00:00:00 2001 From: Buzz-Lightyear Date: Thu, 10 Oct 2024 09:21:03 -0400 Subject: [PATCH 5/5] Merge fix --- internal/bzlmod/go_deps.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/bzlmod/go_deps.bzl b/internal/bzlmod/go_deps.bzl index c4da2ae45..8721676ba 100644 --- a/internal/bzlmod/go_deps.bzl +++ b/internal/bzlmod/go_deps.bzl @@ -590,8 +590,8 @@ def _go_deps_impl(module_ctx): for path, module in module_resolutions.items(): 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), None) - root_module_direct_dev_deps.pop(_repo_name(path), None) + root_module_direct_deps.pop(repo_name(path), None) + root_module_direct_dev_deps.pop(repo_name(path), 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