From f30d2901289b28097525dcd25f1c0020eb1c4afa Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Fri, 31 Jan 2020 14:10:27 -0800 Subject: [PATCH] fix(builtin): fix logic error in linker conflict resolution Fixes https://github.com/bazelbuild/rules_nodejs/issues/1595. The `elif` path in ``` if mappings[k][0] == "runfiles": return True elif v[0] == "runfiles": return False ``` is now exercised by the linker integration tests. --- internal/linker/link_node_modules.bzl | 2 +- internal/linker/test/integration/BUILD.bazel | 40 ++++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/internal/linker/link_node_modules.bzl b/internal/linker/link_node_modules.bzl index 96e550fa89..fb985dfdd3 100644 --- a/internal/linker/link_node_modules.bzl +++ b/internal/linker/link_node_modules.bzl @@ -43,7 +43,7 @@ def _link_mapping(label, mappings, k, v): # v = ["bin", "angular/packages/compiler"] if mappings[k][0] == "runfiles": return True - elif v[0] != "runfiles": + elif v[0] == "runfiles": return False fail(("conflicting mapping at %s: %s maps to both %s and %s" % (label, k, mappings[k], v)), "deps") else: diff --git a/internal/linker/test/integration/BUILD.bazel b/internal/linker/test/integration/BUILD.bazel index b58c3c1bd8..cb14043bdc 100644 --- a/internal/linker/test/integration/BUILD.bazel +++ b/internal/linker/test/integration/BUILD.bazel @@ -77,9 +77,13 @@ linked( testonly = True, out = "actual_with_conflicts", program = ":run_program", + # do not sort deps = [ # NB: reference the copy of index.js in the output folder "//%s/absolute_import:copy_to_bin" % package_name(), + # Intentinally include this before static_linked_pkg as order matters for the linker. + # The order here exercises a different code path in the linker conflict resolution logic + # than `example_with_conflicts_alt`. ":run_program", # NB: static_linked maps to both # ["runfiles", "build_bazel_rules_nodejs/internal/linker/test/integration/static_linked_pkg"] and @@ -97,6 +101,35 @@ linked( ], ) +linked( + name = "example_with_conflicts_alt", + testonly = True, + out = "actual_with_conflicts_alt", + program = ":run_program", + # do not sort + deps = [ + # NB: reference the copy of index.js in the output folder + "//%s/absolute_import:copy_to_bin" % package_name(), + # NB: static_linked maps to both + # ["runfiles", "build_bazel_rules_nodejs/internal/linker/test/integration/static_linked_pkg"] and + # ["bin", "build_bazel_rules_nodejs/internal/linker/test/integration/static_linked_pkg"] + # as the "runfiles" mapping comes from `:run_program` + "//internal/linker/test/integration/static_linked_pkg", + # NB: @linker_scoped/static_linked maps to both + # ["runfiles", "build_bazel_rules_nodejs/internal/linker/test/integration/static_linked_scoped_pkg"] and + # ["src", "build_bazel_rules_nodejs/internal/linker/test/integration/static_linked_scoped_pkg"] + # as the "runfiles" mapping comes from `:run_program` + "//internal/linker/test/integration/static_linked_scoped_pkg", + # Intentinally include this before static_linked_pkg as order matters for the linker. + # The order here exercises a different code path in the linker conflict resolution logic + # than `example_with_conflicts`. + ":run_program", + "//internal/linker/test/integration/dynamic_linked_pkg", + "//internal/linker/test/integration/dynamic_linked_scoped_pkg", + "@npm//semver", + ], +) + golden_file_test( # default rule in this package name = "integration", @@ -110,3 +143,10 @@ golden_file_test( actual = "actual_with_conflicts", golden = "golden.txt", ) + +golden_file_test( + # default rule in this package + name = "integration_conflicts_alt", + actual = "actual_with_conflicts_alt", + golden = "golden.txt", +)