From 64c3a964c25a8a9d15914ae9db411cb07c425168 Mon Sep 17 00:00:00 2001 From: Chuck Grindel Date: Thu, 23 Feb 2023 19:38:53 -0700 Subject: [PATCH] chore: refactor `deps_indexes` separating module resolution from label resolution (#240) - Rename `modules` to `modules_by_name`. - Add `modules_by_label` to support lookup of modules by their primary Bazel label. - Made `labels_for_module` public. - Rename `resolve_module_labels` to `resolve_module`. - Update `pkginfo_target_deps` to resolve the module, then retrieve the labels. Related to #199. --- swiftpkg/internal/deps_indexes.bzl | 132 +++++++----- swiftpkg/internal/pkginfo_target_deps.bzl | 49 +++-- swiftpkg/tests/deps_indexes_tests.bzl | 250 +++++++++------------- 3 files changed, 212 insertions(+), 219 deletions(-) diff --git a/swiftpkg/internal/deps_indexes.bzl b/swiftpkg/internal/deps_indexes.bzl index 6d230826f..53ab89ab2 100644 --- a/swiftpkg/internal/deps_indexes.bzl +++ b/swiftpkg/internal/deps_indexes.bzl @@ -15,40 +15,48 @@ def _new_from_json(json_str): Returns: A `struct` that contains indexes for external dependencies. """ - mi = {} + orig_dict = json.decode(json_str) + return _new( + modules = [ + _new_module_from_dict(mod_dict) + for mod_dict in orig_dict["modules"] + ], + products = [ + _new_product_from_dict(prod_dict) + for prod_dict in orig_dict["products"] + ], + ) + +def _new(modules = [], products = []): + modules_by_name = {} + modules_by_label = {} pi = {} # buildifier: disable=uninitialized def _add_module(m): - entries = mi.get(m.name, []) + entries = modules_by_name.get(m.name, []) entries.append(m) - mi[m.name] = entries + modules_by_name[m.name] = entries + modules_by_label[m.label] = m if m.name != m.c99name: - entries = mi.get(m.c99name, []) + entries = modules_by_name.get(m.c99name, []) entries.append(m) - mi[m.c99name] = entries + modules_by_name[m.c99name] = entries # buildifier: disable=uninitialized def _add_product(p): key = _new_product_index_key(p.identity, p.name) pi[key] = p - orig_dict = json.decode(json_str) - for mod_dict in orig_dict["modules"]: - m = _new_module_from_dict(mod_dict) - _add_module(m) - for prod_dict in orig_dict["products"]: - p = _new_product_from_dict(prod_dict) - _add_product(p) - return _new( - modules = mi, - products = pi, - ) + for module in modules: + _add_module(module) + for product in products: + _add_product(product) -def _new(modules = {}, products = {}): return struct( - modules = modules, - products = products, + modules_by_name = modules_by_name, + modules_by_label = modules_by_label, + products = pi, ) def _new_module_from_dict(mod_dict): @@ -126,10 +134,40 @@ def _labels_for_module(module, depender_src_type): return labels -def _resolve_module_labels( +def _get_module(deps_index, label): + """Return the module associated with the specified label. + + Args: + deps_index: A `dict` as returned by `deps_indexes.new_from_json`. + label: A `struct` as returned by `bazel_labels.new` or a `string` value + that can be parsed into a Bazel label. + + Returns: + If found, a module `struct` as returned by `deps_indexes.new_module`. + Otherwise, `None`. + """ + if type(label) == "string": + label = bazel_labels.parse(label) + return deps_index.modules_by_label.get(label) + +def _modules_for_product(deps_index, product): + """Returns the modules associated with the product. + + Args: + deps_index: A `dict` as returned by `deps_indexes.new_from_json`. + product: A `struct` as returned by `deps_indexes.new_product`. + + Returns: + A `list` of the modules associated with the product. + """ + return lists.flatten(lists.compact([ + _get_module(deps_index, label) + for label in product.target_labels + ])) + +def _resolve_module( deps_index, module_name, - depender_module_name, preferred_repo_name = None, restrict_to_repo_names = []): """Finds a Bazel label that provides the specified module. @@ -137,22 +175,18 @@ def _resolve_module_labels( Args: deps_index: A `dict` as returned by `deps_indexes.new_from_json`. module_name: The name of the module as a `string` - depender_module_name: The name of the depender module as a `string`. preferred_repo_name: Optional. If a target in this repository provides the module, prefer it. restrict_to_repo_names: Optional. A `list` of repository names to restrict the match. Returns: - A `list` of `struct` values as returned by `bazel_labels.new`. + If a module is found, a `struct` as returned by `bazel_labels.new`. + Otherwise, `None`. """ - modules = deps_index.modules.get(module_name, []) + modules = deps_index.modules_by_name.get(module_name, []) if len(modules) == 0: - return [] - - depender_modules = deps_index.modules.get(depender_module_name, []) - if len(depender_modules) == 0: - fail("No depender modules found for {}.".format(depender_module_name)) + return None # If a repo name is provided, prefer that over any other matches if preferred_repo_name != None: @@ -161,14 +195,8 @@ def _resolve_module_labels( modules, lambda m: m.label.repository_name == preferred_repo_name, ) - depender_module = lists.find( - depender_modules, - lambda m: m.label.repository_name == preferred_repo_name, - ) - if depender_module == None: - depender_module = depender_modules[0] if module != None: - return _labels_for_module(module, depender_module.src_type) + return module # If we are meant to only find a match in a set of repo names, then if len(restrict_to_repo_names) > 0: @@ -182,20 +210,11 @@ def _resolve_module_labels( for m in modules if sets.contains(repo_names, m.label.repository_name) ] - depender_modules = [ - m - for m in depender_modules - if sets.contains(repo_names, m.label.repository_name) - ] # Only labels for the first module. if len(modules) == 0: - return [] - if len(depender_modules) == 0: - fail("No depender modules found for {} in the restricted repos.".format( - depender_module_name, - )) - return _labels_for_module(modules[0], depender_modules[0].src_type) + return None + return modules[0] def _new_product_index_key(identity, name): return identity.lower() + "|" + name @@ -254,24 +273,22 @@ def _new_ctx(deps_index, preferred_repo_name = None, restrict_to_repo_names = [] restrict_to_repo_names = restrict_to_repo_names, ) -def _resolve_module_labels_with_ctx( +def _resolve_module_with_ctx( deps_index_ctx, - module_name, - depender_module_name): + module_name): """Finds a Bazel label that provides the specified module. Args: deps_index_ctx: A `struct` as returned by `deps_indexes.new_ctx`. module_name: The name of the module as a `string` - depender_module_name: The name of the depender module as a `string`. Returns: - A `list` of `struct` values as returned by `bazel_labels.new`. + If a module is found, a `struct` as returned by `bazel_labels.new`. + Otherwise, `None`. """ - return _resolve_module_labels( + return _resolve_module( deps_index = deps_index_ctx.deps_index, module_name = module_name, - depender_module_name = depender_module_name, preferred_repo_name = deps_index_ctx.preferred_repo_name, restrict_to_repo_names = deps_index_ctx.restrict_to_repo_names, ) @@ -293,12 +310,15 @@ src_types = struct( deps_indexes = struct( find_product = _find_product, + get_module = _get_module, + labels_for_module = _labels_for_module, + modules_for_product = _modules_for_product, new = _new, new_ctx = _new_ctx, new_from_json = _new_from_json, new_module = _new_module, new_product = _new_product, - resolve_module_labels = _resolve_module_labels, - resolve_module_labels_with_ctx = _resolve_module_labels_with_ctx, + resolve_module = _resolve_module, + resolve_module_with_ctx = _resolve_module_with_ctx, resolve_product_labels = _resolve_product_labels, ) diff --git a/swiftpkg/internal/pkginfo_target_deps.bzl b/swiftpkg/internal/pkginfo_target_deps.bzl index 230ce503c..e130e1f86 100644 --- a/swiftpkg/internal/pkginfo_target_deps.bzl +++ b/swiftpkg/internal/pkginfo_target_deps.bzl @@ -1,6 +1,6 @@ """Module for generating data from target dependencies created by `pkginfos`.""" -load("@cgrindel_bazel_starlib//bzllib:defs.bzl", "bazel_labels") +load("@cgrindel_bazel_starlib//bzllib:defs.bzl", "bazel_labels", "lists") load(":bzl_selects.bzl", "bzl_selects") load(":deps_indexes.bzl", "deps_indexes") load(":pkginfo_dependencies.bzl", "pkginfo_dependencies") @@ -31,33 +31,28 @@ def make_pkginfo_target_deps(bazel_labels): # This should look for a matching product first. Then, look for a # module directly? condition = target_dep.by_name.condition - labels = deps_indexes.resolve_module_labels_with_ctx( + module = deps_indexes.resolve_module_with_ctx( pkg_ctx.deps_index_ctx, target_dep.by_name.name, - depender_module_name, ) - if len(labels) == 0: - # Seeing Package.swift files with byName dependencies that - # cannot be resolved (i.e., they do not exist). - # Example `protoc-gen-swift` in - # `https://github.com/grpc/grpc-swift.git`. - # Printing warnings is discouraged. So, just keep moving - # print("""\ - # Unable to resolve by_name target dependency for {module_name}.\ - # """.format(module_name = target_dep.by_name.name)) - pass + + # Seeing Package.swift files with byName dependencies that + # cannot be resolved (i.e., they do not exist). + # Example `protoc-gen-swift` in + # `https://github.com/grpc/grpc-swift.git`. + modules = [module] if module != None else [] elif target_dep.target: condition = target_dep.target.condition - labels = deps_indexes.resolve_module_labels_with_ctx( + module = deps_indexes.resolve_module_with_ctx( pkg_ctx.deps_index_ctx, target_dep.target.target_name, - depender_module_name, ) - if len(labels) == 0: + if module == None: fail("""\ Unable to resolve target reference target dependency for {module_name}.\ """.format(module_name = target_dep.target.target_name)) + modules = [module] elif target_dep.product: condition = target_dep.product.condition @@ -70,24 +65,42 @@ Unable to resolve target reference target dependency for {module_name}.\ fail("""\ Did not find external dependency with name/identity {}.\ """.format(prod_ref.dep_name)) - labels = deps_indexes.resolve_product_labels( + + product = deps_indexes.find_product( deps_index = pkg_ctx.deps_index_ctx.deps_index, identity = dep.identity, name = prod_ref.product_name, ) - if len(labels) == 0: + if product == None: fail("""\ Unable to resolve product reference target dependency for product {prod_name} provided by {dep_name}. """.format( prod_name = prod_ref.product_name, dep_name = prod_ref.dep_name, )) + modules = deps_indexes.modules_for_product( + deps_index = pkg_ctx.deps_index_ctx.deps_index, + product = product, + ) else: fail("""\ Unrecognized target dependency while generating a Bazel dependency label.\ """) + # Find the depender module + depender_module = deps_indexes.resolve_module_with_ctx( + pkg_ctx.deps_index_ctx, + depender_module_name, + ) + if depender_module == None: + fail("Unable to find depender module named {}.".format(depender_module_name)) + + labels = lists.flatten([ + deps_indexes.labels_for_module(module, depender_module.src_type) + for module in modules + ]) + return bzl_selects.new_from_target_dependency_condition( kind = _target_dep_kind, labels = [ diff --git a/swiftpkg/tests/deps_indexes_tests.bzl b/swiftpkg/tests/deps_indexes_tests.bzl index 1f2a04c7b..4dcc6e5d6 100644 --- a/swiftpkg/tests/deps_indexes_tests.bzl +++ b/swiftpkg/tests/deps_indexes_tests.bzl @@ -7,218 +7,134 @@ load("//swiftpkg/internal:deps_indexes.bzl", "deps_indexes") def _new_from_json_test(ctx): env = unittest.begin(ctx) - actual = _deps_index - expected_modules = { - "ArgumentParser": [ - deps_indexes.new_module( - name = "ArgumentParser", - c99name = "ArgumentParser", - src_type = "swift", - label = bazel_labels.new( - repository_name = "@apple_swift_argument_parser", - package = "Sources/ArgumentParser", - name = "ArgumentParser", - ), - ), - ], - "Bar": [ - deps_indexes.new_module( - name = "Bar", - c99name = "Bar", - src_type = "swift", - label = bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "Bar", - ), - ), - deps_indexes.new_module( - name = "Bar", - c99name = "Bar", - src_type = "swift", - label = bazel_labels.new( - repository_name = "@example_another_repo", - package = "Sources/Bar", - name = "Bar", - ), - ), - ], - "Foo": [ - deps_indexes.new_module( - name = "Foo", - c99name = "Foo", - src_type = "swift", - label = bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "Foo", - ), - ), - deps_indexes.new_module( - name = "Foo", - c99name = "Foo", - src_type = "swift", - label = bazel_labels.new( - repository_name = "@example_another_repo", - package = "Sources/Foo", - name = "Foo", - ), - ), - ], - "ObjcLibrary": [ - deps_indexes.new_module( - name = "ObjcLibrary", - c99name = "ObjcLibrary", - src_type = "objc", - label = bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "ObjcLibrary", - ), - ), - ], - } - expected = deps_indexes.new(modules = expected_modules) - asserts.equals(env, expected, actual) + asserts.equals(env, 4, len(_deps_index.modules_by_name)) + asserts.equals(env, 6, len(_deps_index.modules_by_label)) + asserts.equals(env, 0, len(_deps_index.products)) return unittest.end(env) new_from_json_test = unittest.make(_new_from_json_test) -def _resolve_module_labels_test(ctx): +def _get_module_test(ctx): + env = unittest.begin(ctx) + + tests = [ + struct( + msg = "label is a string", + label = "@apple_swift_argument_parser//Sources/ArgumentParser", + exp_name = "ArgumentParser", + ), + struct( + msg = "label is a struct", + label = bazel_labels.parse( + "@apple_swift_argument_parser//Sources/ArgumentParser", + ), + exp_name = "ArgumentParser", + ), + ] + for t in tests: + actual = deps_indexes.get_module(_deps_index, t.label) + asserts.equals( + env, + bazel_labels.normalize(t.label), + bazel_labels.normalize(actual.label), + t.msg, + ) + asserts.equals(env, t.exp_name, actual.name, t.msg) + + return unittest.end(env) + +get_module_test = unittest.make(_get_module_test) + +def _resolve_module_test(ctx): env = unittest.begin(ctx) tests = [ struct( msg = "Foo module", module = "Foo", - depender_module_name = "Bar", preferred = None, restrict_to = [], - exp = [bazel_labels.new( + exp = bazel_labels.new( repository_name = "@example_cool_repo", package = "", name = "Foo", - )], + ), ), struct( msg = "module not in index", module = "DoesNotExist", - depender_module_name = "DoesNotExist", preferred = None, restrict_to = [], - exp = [], + exp = None, ), struct( msg = "preferred repo name exists", module = "Foo", - depender_module_name = "Bar", preferred = "example_cool_repo", restrict_to = [], - exp = [bazel_labels.new( + exp = bazel_labels.new( repository_name = "@example_cool_repo", package = "", name = "Foo", - )], + ), ), struct( msg = "preferred repo name not found", module = "ArgumentParser", - depender_module_name = "Bar", preferred = "example_another_repo", restrict_to = [], - exp = [bazel_labels.new( + exp = bazel_labels.new( repository_name = "@apple_swift_argument_parser", package = "Sources/ArgumentParser", name = "ArgumentParser", - )], + ), ), struct( msg = "restrict to repos, found one", module = "Foo", - depender_module_name = "Bar", preferred = None, restrict_to = ["some_other_repo", "example_another_repo"], - exp = [bazel_labels.new( + exp = bazel_labels.new( repository_name = "@example_another_repo", package = "Sources/Foo", name = "Foo", - )], + ), ), struct( msg = "restrict to repos, not found", module = "Foo", - depender_module_name = "Bar", preferred = None, restrict_to = ["some_other_repo"], - exp = [], + exp = None, ), struct( msg = "preferred repo and restrict to repos, found preferred", module = "Foo", - depender_module_name = "Bar", preferred = "example_cool_repo", restrict_to = ["example_cool_repo", "example_another_repo"], - exp = [bazel_labels.new( + exp = bazel_labels.new( repository_name = "@example_cool_repo", package = "", name = "Foo", - )], - ), - struct( - msg = "Swift library depends upon Objc library", - module = "ObjcLibrary", - depender_module_name = "Bar", - preferred = None, - restrict_to = [], - exp = [ - bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "ObjcLibrary", - ), - bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "ObjcLibrary_modulemap", - ), - ], - ), - struct( - msg = "Objc library depends upon Swift library", - module = "Foo", - depender_module_name = "ObjcLibrary", - preferred = None, - restrict_to = [], - exp = [ - bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "Foo", - ), - bazel_labels.new( - repository_name = "@example_cool_repo", - package = "", - name = "Foo_modulemap", - ), - ], + ), ), ] for t in tests: - actual = deps_indexes.resolve_module_labels( + actual = deps_indexes.resolve_module( _deps_index, module_name = t.module, - depender_module_name = t.depender_module_name, preferred_repo_name = t.preferred, restrict_to_repo_names = t.restrict_to, ) - asserts.equals(env, t.exp, actual, t.msg) + actual_label = actual.label if actual else None + asserts.equals(env, t.exp, actual_label, t.msg) return unittest.end(env) -resolve_module_labels_test = unittest.make(_resolve_module_labels_test) +resolve_module_test = unittest.make(_resolve_module_test) -def _resolve_module_labels_with_ctx_test(ctx): +def _resolve_module_with_ctx_test(ctx): env = unittest.begin(ctx) deps_index_ctx = deps_indexes.new_ctx( @@ -226,26 +142,70 @@ def _resolve_module_labels_with_ctx_test(ctx): preferred_repo_name = "example_cool_repo", restrict_to_repo_names = ["example_cool_repo", "example_another_repo"], ) - actuals = deps_indexes.resolve_module_labels_with_ctx( + actual = deps_indexes.resolve_module_with_ctx( deps_index_ctx = deps_index_ctx, module_name = "Foo", - depender_module_name = "Bar", ) - asserts.equals(env, 1, len(actuals)) - actual = actuals[0] - asserts.equals(env, "@example_cool_repo", actual.repository_name) - asserts.equals(env, "Foo", actual.name) + exp_label = bazel_labels.parse("@example_cool_repo//:Foo") + asserts.equals(env, exp_label, actual.label) + + return unittest.end(env) + +resolve_module_with_ctx_test = unittest.make(_resolve_module_with_ctx_test) + +def _labels_for_module_test(ctx): + env = unittest.begin(ctx) + + tests = [ + struct( + msg = "Swift depend upon Swift", + dep_module = "@example_cool_repo//:Foo", + depender_module = "@example_cool_repo//:Bar", + exp = [ + bazel_labels.parse("@example_cool_repo//:Foo"), + ], + ), + struct( + msg = "Swift library depends upon Objc library", + dep_module = "@example_cool_repo//:ObjcLibrary", + depender_module = "@example_cool_repo//:Bar", + exp = [ + bazel_labels.parse("@example_cool_repo//:ObjcLibrary"), + bazel_labels.parse("@example_cool_repo//:ObjcLibrary_modulemap"), + ], + ), + struct( + msg = "Objc library depends upon Swift library", + dep_module = "@example_cool_repo//:Foo", + depender_module = "@example_cool_repo//:ObjcLibrary", + exp = [ + bazel_labels.parse("@example_cool_repo//:Foo"), + bazel_labels.parse("@example_cool_repo//:Foo_modulemap"), + ], + ), + ] + for t in tests: + module = deps_indexes.get_module(_deps_index, t.dep_module) + depender = deps_indexes.get_module(_deps_index, t.depender_module) + if module == None: + fail("The module is `None` for {}.".format(t.label)) + if depender == None: + fail("The depender module is `None` for {}.".format(t.depender_label)) + actual = deps_indexes.labels_for_module(module, depender.src_type) + asserts.equals(env, t.exp, actual, t.msg) return unittest.end(env) -resolve_module_labels_with_ctx_test = unittest.make(_resolve_module_labels_with_ctx_test) +labels_for_module_test = unittest.make(_labels_for_module_test) def deps_indexes_test_suite(): return unittest.suite( "deps_indexes_tests", new_from_json_test, - resolve_module_labels_test, - resolve_module_labels_with_ctx_test, + get_module_test, + resolve_module_test, + resolve_module_with_ctx_test, + labels_for_module_test, ) _deps_index_json = """