fix: fix logic error in linker conflict resolution

Fixes #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.
bazel-contrib · Jan 31, 2020 · fe72f51 · fe72f51
1 parent 5c36bd7
commit fe72f51
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 1 deletion.
diff --git 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
@@ -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",
+)