feat: add node_urls parameter to bzlmod toolchain in the node extension (#3763)

redsun82 · Paolo Tranquilli · web-flow · commit 4c4844946b12 · 2024-09-18T10:01:12.000-07:00
---------

Co-authored-by: Paolo Tranquilli &lt;redsun82@gihub.com&gt;
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -19,12 +19,14 @@ jobs:
   test:
     uses: bazel-contrib/.github/.github/workflows/bazel.yaml@v6
     with:
-      folders: '[".", "e2e/headers", "e2e/smoke", "e2e/nodejs_host"]'
+      folders: '[".", "e2e/headers", "e2e/smoke", "e2e/nodejs_host", "e2e/conflicting_toolchains"]'
       # stardoc generated docs fail on diff_test with Bazel 6.4.0 so don't test against it in root repository
       exclude: |
         [
           {"bazelversion": "6.4.0", "os": "macos-latest"},
           {"bazelversion": "6.4.0", "os": "windows-latest"},
           {"bazelversion": "6.4.0", folder: "."},
           {"bazelversion": "6.4.0", bzlmodEnabled: true}
+          # this test is for bzlmod only
+          {folder: "e2e/conflicting_toolchains", bzlmodEnabled: false},
         ]
diff --git a/e2e/conflicting_toolchains/.bazelversion b/e2e/conflicting_toolchains/.bazelversion
@@ -0,0 +1 @@
+../../.bazelversion
diff --git a/e2e/conflicting_toolchains/.gitignore b/e2e/conflicting_toolchains/.gitignore
@@ -0,0 +1 @@
+error.txt
diff --git a/e2e/conflicting_toolchains/BUILD.bazel b/e2e/conflicting_toolchains/BUILD.bazel
@@ -0,0 +1,13 @@
+# this is set up to make bazel test //... pass
+load("@bazel_skylib//rules:write_file.bzl", "write_file")
+
+write_file(
+    name = "empty",
+    out = "empty.sh",
+    content = [],
+)
+
+sh_test(
+    name = "dummy",
+    srcs = [":empty"],
+)
diff --git a/e2e/conflicting_toolchains/MODULE.bazel b/e2e/conflicting_toolchains/MODULE.bazel
@@ -0,0 +1,2 @@
+# this is set up to make bazel test //... pass
+bazel_dep(name = "bazel_skylib", version = "1.7.1", dev_dependency = True)
diff --git a/e2e/conflicting_toolchains/test.sh b/e2e/conflicting_toolchains/test.sh
@@ -0,0 +1,22 @@
+#!/bin/bash
+# this is the integration test checking various combinations of conflicting nodejs toolchain definitions
+
+set -eu
+
+for test_attr in test_*; do
+  pushd $test_attr > /dev/null
+  attr=${test_attr#test_}
+  echo -n "testing conflict on $attr... "
+  if bazel mod tidy &> error.txt; then
+    echo "ERROR: bazel mod tidy should have failed with following MODULE.bazel:"
+    cat MODULE.bazel
+    exit 1
+  elif ! grep "conflicting toolchains" error.txt > /dev/null; then
+    echo "ERROR: expected bazel mod tidy to mention conflicting toolchains, found:"
+    cat error.txt
+    exit 1
+  else
+    echo "PASS"
+  fi
+  popd > /dev/null
+done
diff --git a/e2e/conflicting_toolchains/test_include_headers/.bazelversion b/e2e/conflicting_toolchains/test_include_headers/.bazelversion
@@ -0,0 +1 @@
+../.bazelversion
diff --git a/e2e/conflicting_toolchains/test_include_headers/MODULE.bazel b/e2e/conflicting_toolchains/test_include_headers/MODULE.bazel
@@ -0,0 +1,14 @@
+bazel_dep(name = "rules_nodejs", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "rules_nodejs",
+    path = "../../..",
+)
+
+node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
+node.toolchain(
+    name = "mynode",
+)
+node.toolchain(
+    name = "mynode",
+    node_urls = ["file://whatever"],
+)
diff --git a/e2e/conflicting_toolchains/test_node_urls/.bazelversion b/e2e/conflicting_toolchains/test_node_urls/.bazelversion
@@ -0,0 +1 @@
+../.bazelversion
diff --git a/e2e/conflicting_toolchains/test_node_urls/MODULE.bazel b/e2e/conflicting_toolchains/test_node_urls/MODULE.bazel
@@ -0,0 +1,14 @@
+bazel_dep(name = "rules_nodejs", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "rules_nodejs",
+    path = "../../..",
+)
+
+node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
+node.toolchain(
+    name = "mynode",
+)
+node.toolchain(
+    name = "mynode",
+    include_headers = True,
+)
diff --git a/e2e/conflicting_toolchains/test_node_version/.bazelversion b/e2e/conflicting_toolchains/test_node_version/.bazelversion
@@ -0,0 +1 @@
+../.bazelversion
diff --git a/e2e/conflicting_toolchains/test_node_version/MODULE.bazel b/e2e/conflicting_toolchains/test_node_version/MODULE.bazel
@@ -0,0 +1,14 @@
+bazel_dep(name = "rules_nodejs", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "rules_nodejs",
+    path = "../../..",
+)
+
+node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
+node.toolchain(
+    name = "mynode",
+)
+node.toolchain(
+    name = "mynode",
+    node_version = "15.14.0",
+)
diff --git a/e2e/conflicting_toolchains/test_node_version_from_nvmrc/.bazelversion b/e2e/conflicting_toolchains/test_node_version_from_nvmrc/.bazelversion
@@ -0,0 +1 @@
+../.bazelversion
diff --git a/e2e/conflicting_toolchains/test_node_version_from_nvmrc/.nvmrc b/e2e/conflicting_toolchains/test_node_version_from_nvmrc/.nvmrc
diff --git a/e2e/conflicting_toolchains/test_node_version_from_nvmrc/BUILD.bazel b/e2e/conflicting_toolchains/test_node_version_from_nvmrc/BUILD.bazel
diff --git a/e2e/conflicting_toolchains/test_node_version_from_nvmrc/MODULE.bazel b/e2e/conflicting_toolchains/test_node_version_from_nvmrc/MODULE.bazel
@@ -0,0 +1,14 @@
+bazel_dep(name = "rules_nodejs", version = "0.0.0", dev_dependency = True)
+local_path_override(
+    module_name = "rules_nodejs",
+    path = "../../..",
+)
+
+node = use_extension("@rules_nodejs//nodejs:extensions.bzl", "node", dev_dependency = True)
+node.toolchain(
+    name = "mynode",
+)
+node.toolchain(
+    name = "mynode",
+    node_version_from_nvmrc = "//:.nvmrc",
+)
diff --git a/e2e/smoke/BUILD.bazel b/e2e/smoke/BUILD.bazel
@@ -202,6 +202,12 @@ write_file(
 
 # Files used in test cases later that contain the correct nodejs version
 # that is imported into the workspace.
+write_file(
+    name = "write_node_version_15",
+    out = "expected_node_15",
+    content = ["v15.14.0"],
+)
+
 write_file(
     name = "write_node_version_17",
     out = "expected_node_17",
@@ -268,3 +274,22 @@ diff_test(
     file1 = "write_node_version_16",
     file2 = "thing_toolchain_16",
 )
+
+my_nodejs(
+    name = "run_15",
+    out = "thing_toolchain_15",
+    entry_point = "version.js",
+    # using the select statement will download toolchains for all three platforms
+    # you can also just provide an individual toolchain if you don't want to download them all
+    toolchain = select({
+        "@bazel_tools//src/conditions:linux_x86_64": "@node15_linux_amd64//:toolchain",
+        "@bazel_tools//src/conditions:darwin": "@node15_darwin_amd64//:toolchain",
+        "@bazel_tools//src/conditions:windows": "@node15_windows_amd64//:toolchain",
+    }),
+)
+
+diff_test(
+    name = "test_node_version_15",
+    file1 = "write_node_version_15",
+    file2 = "thing_toolchain_15",
+)
diff --git a/e2e/smoke/MODULE.bazel b/e2e/smoke/MODULE.bazel
@@ -14,10 +14,21 @@ node.toolchain(
     name = "node17",
     node_version = "17.9.1",
 )
+node.toolchain(
+    name = "node15",
+    node_urls = [
+        "https://nodejs.org/dist/v{version}/{filename}",
+        "https://mirrors.dotsrc.org/nodejs/release/v{version}/{filename}",
+    ],
+    node_version = "15.14.0",
+)
 
 # FIXME(6.0): a repo rule with name=foo should create a repo named @foo, not @foo_toolchains
 use_repo(
     node,
+    "node15_darwin_amd64",
+    "node15_linux_amd64",
+    "node15_windows_amd64",
     "node17_darwin_amd64",
     "node17_linux_amd64",
     "node17_windows_amd64",
diff --git a/nodejs/extensions.bzl b/nodejs/extensions.bzl
@@ -1,6 +1,24 @@
 "extensions for bzlmod"
 
-load(":repositories.bzl", "DEFAULT_NODE_REPOSITORY", "DEFAULT_NODE_VERSION", "nodejs_register_toolchains")
+load(
+    ":repositories.bzl",
+    "DEFAULT_NODE_REPOSITORY",
+    "DEFAULT_NODE_URL",
+    "DEFAULT_NODE_VERSION",
+    "nodejs_register_toolchains",
+)
+
+def _toolchain_repr(toolchain):
+    """ Return a `toolchain` tag object representation useful for diagnostics """
+    key_values = [(attr, getattr(toolchain, attr)) for attr in _ATTRS]
+    return ", ".join(["%s = %r" % (attr, value) for attr, value in key_values if value])
+
+def _toolchains_equal(lhs, rhs):
+    """ Compare two `toolchain` tag objects """
+    for attr in _ATTRS:
+        if getattr(lhs, attr) != getattr(rhs, attr):
+            return False
+    return True
 
 def _toolchain_extension(module_ctx):
     registrations = {}
@@ -14,54 +32,63 @@ def _toolchain_extension(module_ctx):
                     # Prioritize the root-most registration of the default node toolchain version and
                     # ignore any further registrations (modules are processed breadth-first)
                     continue
-                if toolchain.node_version == registrations[toolchain.name].node_version and toolchain.node_version_from_nvmrc == registrations[toolchain.name].node_version_from_nvmrc:
+                if not _toolchains_equal(toolchain, registrations[toolchain.name]):
+                    fail("Multiple conflicting toolchains declared:\n* {}\n* {}".format(
+                        _toolchain_repr(toolchain),
+                        _toolchain_repr(registrations[toolchain.name]),
+                    ))
+                else:
                     # No problem to register a matching toolchain twice
                     continue
-                fail("Multiple conflicting toolchains declared for name {} ({} and {})".format(
-                    toolchain.name,
-                    toolchain.node_version,
-                    registrations[toolchain.name],
-                ))
             else:
-                registrations[toolchain.name] = struct(
-                    node_version = toolchain.node_version,
-                    node_version_from_nvmrc = toolchain.node_version_from_nvmrc,
-                    include_headers = toolchain.include_headers,
-                )
+                registrations[toolchain.name] = toolchain
 
     for k, v in registrations.items():
         nodejs_register_toolchains(
             name = k,
             node_version = v.node_version,
             node_version_from_nvmrc = v.node_version_from_nvmrc,
+            node_urls = v.node_urls,
             include_headers = v.include_headers,
             register = False,
         )
 
-node = module_extension(
-    implementation = _toolchain_extension,
-    tag_classes = {
-        "toolchain": tag_class(attrs = {
-            "name": attr.string(
-                doc = "Base name for generated repositories",
-                default = DEFAULT_NODE_REPOSITORY,
-            ),
-            "node_version": attr.string(
-                doc = "Version of the Node.js interpreter",
-                default = DEFAULT_NODE_VERSION,
-            ),
-            "node_version_from_nvmrc": attr.label(
-                allow_single_file = True,
-                doc = """The .nvmrc file containing the version of Node.js to use.
+_ATTRS = {
+    "name": attr.string(
+        doc = "Base name for generated repositories",
+        default = DEFAULT_NODE_REPOSITORY,
+    ),
+    "node_version": attr.string(
+        doc = "Version of the Node.js interpreter",
+        default = DEFAULT_NODE_VERSION,
+    ),
+    "node_version_from_nvmrc": attr.label(
+        allow_single_file = True,
+        doc = """The .nvmrc file containing the version of Node.js to use.
 
 If set then the version found in the .nvmrc file is used instead of the one specified by node_version.""",
-            ),
-            "include_headers": attr.bool(
-                doc = """Set headers field in NodeInfo provided by this toolchain.
+    ),
+    "include_headers": attr.bool(
+        doc = """Set headers field in NodeInfo provided by this toolchain.
 
 This setting creates a dependency on a c++ toolchain.
 """,
-            ),
-        }),
+    ),
+    "node_urls": attr.string_list(
+        doc = """List of URLs to use to download Node.js.
+
+ Each entry is a template for downloading a node distribution.
+
+ The `{version}` parameter is substituted with the `node_version` attribute,
+ and `{filename}` with the matching entry from the `node_repositories` attribute.
+ """,
+        default = [DEFAULT_NODE_URL],
+    ),
+}
+
+node = module_extension(
+    implementation = _toolchain_extension,
+    tag_classes = {
+        "toolchain": tag_class(attrs = _ATTRS),
     },
 )

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	`+# this is set up to make bazel test //... pass`
	`2`	`+bazel_dep(name = "bazel_skylib", version = "1.7.1", dev_dependency = True)`