refactor(builtin): cleanup & refactoring nodejs toolchain support aft…

…er review

README.md

@@ -463,7 +463,7 @@ Note: the arguments passed to `bazel run` after `--` are forwarded to the execut
 
 When you add `node_repositories()` to your `WORKSPACE` file it will setup a node toolchain for all currently supported platforms, Linux, macOS and Windows. Amongst other things this adds support for cross-compilations as well as Remote Build Execution support. For more detailed information also see [Bazel Toolchains](https://docs.bazel.build/versions/master/toolchains.html).
 
-If you have an advanced use-case you can also register your own toolchains and call `node_configure` directly to manually setup a toolchain.
+If you have an advanced use-case you can also register your own toolchains and call `node_toolchain_configure` directly to manually setup a toolchain.
 
 #### Cross-compilation
 

internal/common/os_name.bzl

@@ -23,19 +23,16 @@ OS_ARCH_NAMES = [
 
 OS_NAMES = ["_".join(os_arch_name) for os_arch_name in OS_ARCH_NAMES]
 
-def is_windows(os_name):
-    return os_name == OS_NAMES[1]
-
-def os_name(repository_ctx):
+def os_name(rctx):
     """Get the os name for a repository rule
 
     Args:
-      repository_ctx: The repository rule context
+      rctx: The repository rule context
 
     Returns:
       A string describing the os for a repository rule
     """
-    os_name = repository_ctx.os.name.lower()
+    os_name = rctx.os.name.lower()
     if os_name.startswith("mac os"):
         return OS_NAMES[0]
     elif os_name.find("windows") != -1:
@@ -44,3 +41,12 @@ def os_name(repository_ctx):
         return OS_NAMES[2]
     else:
         fail("Unsupported operating system: " + os_name)
+
+def is_darwin_os(rctx):
+    return os_name(rctx) == OS_NAMES[0]
+
+def is_windows_os(rctx):
+    return os_name(rctx) == OS_NAMES[1]
+
+def is_linux_os(rctx):
+    return os_name(rctx) == OS_NAMES[2]

internal/copy_repository/copy_repository.bzl

@@ -15,15 +15,14 @@
 """Custom copy_repository rule used by npm_install and yarn_install.
 """
 
-load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "os_name")
+load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "is_windows_os")
 
 def _copy_file(rctx, src):
     rctx.template(src.basename, src)
 
 def _copy_repository_impl(rctx):
     src_path = "/".join(str(rctx.path(rctx.attr.marker_file)).split("/")[:-1])
-    is_windows = os_name(rctx).find("windows") != -1
-    if is_windows:
+    if is_windows_os(rctx):
         _copy_file(rctx, rctx.path(Label("@build_bazel_rules_nodejs//internal/copy_repository:_copy.bat")))
         result = rctx.execute(["cmd.exe", "/C", "_copy.bat", src_path.replace("/", "\\"), "."])
     else:

internal/node/node.bzl

@@ -14,7 +14,7 @@
 
 """Executing programs
 
-These rules run the node binary with the given sources.
+These rules run the node executable with the given sources.
 
 They support module mapping: any targets in the transitive dependencies with
 a `module_name` attribute can be `require`d by that name.

internal/node/node_labels.bzl

@@ -17,37 +17,39 @@
 Labels are different on windows and linux/OSX.
 """
 
-def get_node_label(os_name):
-    if os_name.find("windows") != -1:
-        label = Label("@nodejs_%s//:bin/node.cmd" % os_name)
+load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "is_windows_os", "os_name")
+
+def get_node_label(rctx):
+    if is_windows_os(rctx):
+        label = Label("@nodejs_%s//:bin/node.cmd" % os_name(rctx))
     else:
-        label = Label("@nodejs_%s//:bin/node" % os_name)
+        label = Label("@nodejs_%s//:bin/node" % os_name(rctx))
     return label
 
-def get_npm_label(os_name):
-    if os_name.find("windows") != -1:
-        label = Label("@nodejs_%s//:bin/npm.cmd" % os_name)
+def get_npm_label(rctx):
+    if is_windows_os(rctx):
+        label = Label("@nodejs_%s//:bin/npm.cmd" % os_name(rctx))
     else:
-        label = Label("@nodejs_%s//:bin/npm" % os_name)
+        label = Label("@nodejs_%s//:bin/npm" % os_name(rctx))
     return label
 
-def get_npm_node_repositories_label(os_name):
-    if os_name.find("windows") != -1:
-        label = Label("@nodejs_%s//:bin/npm_node_repositories.cmd" % os_name)
+def get_npm_node_repositories_label(rctx):
+    if is_windows_os(rctx):
+        label = Label("@nodejs_%s//:bin/npm_node_repositories.cmd" % os_name(rctx))
     else:
-        label = Label("@nodejs_%s//:bin/npm_node_repositories" % os_name)
+        label = Label("@nodejs_%s//:bin/npm_node_repositories" % os_name(rctx))
     return label
 
-def get_yarn_label(os_name):
-    if os_name.find("windows") != -1:
-        label = Label("@nodejs_%s//:bin/yarn.cmd" % os_name)
+def get_yarn_label(rctx):
+    if is_windows_os(rctx):
+        label = Label("@nodejs_%s//:bin/yarn.cmd" % os_name(rctx))
     else:
-        label = Label("@nodejs_%s//:bin/yarn" % os_name)
+        label = Label("@nodejs_%s//:bin/yarn" % os_name(rctx))
     return label
 
-def get_yarn_node_repositories_label(os_name):
-    if os_name.find("windows") != -1:
-        label = Label("@nodejs%s//:bin/yarn_node_repositories.cmd" % os_name)
+def get_yarn_node_repositories_label(rctx):
+    if is_windows_os(rctx):
+        label = Label("@nodejs%s//:bin/yarn_node_repositories.cmd" % os_name(rctx))
     else:
-        label = Label("@nodejs_%s//:bin/yarn_node_repositories" % os_name)
+        label = Label("@nodejs_%s//:bin/yarn_node_repositories" % os_name(rctx))
     return label

internal/node/node_repositories.bzl

@@ -20,10 +20,10 @@ See https://docs.bazel.build/versions/master/skylark/repository_rules.html
 
 load("//internal/common:check_bazel_version.bzl", "check_bazel_version")
 load("//internal/common:check_version.bzl", "check_version")
-load("//internal/common:os_name.bzl", "OS_ARCH_NAMES", "is_windows", "os_name")
+load("//internal/common:os_name.bzl", "OS_ARCH_NAMES", "is_windows_os", "os_name")
 load("//internal/npm_install:npm_install.bzl", "yarn_install")
 load("//third_party/github.com/bazelbuild/bazel-skylib:lib/paths.bzl", "paths")
-load("//toolchains/node:node_configure.bzl", node_toolchain_configure = "node_configure")
+load("//toolchains/node:node_toolchain_configure.bzl", "node_toolchain_configure")
 load(":node_labels.bzl", "get_yarn_node_repositories_label")
 
 # Callers that don't specify a particular version will get these.
@@ -195,11 +195,9 @@ def _prepare_node(repository_ctx):
     Args:
       repository_ctx: The repository rule context
     """
-    is_windows_os = os_name(repository_ctx).find("windows") != -1
 
     # TODO: Maybe we want to encode the OS as a specific attribute rather than do it based on naming?
-    is_windows_repository = repository_ctx.attr.name.find("windows") != -1
-    is_windows = is_windows_os or is_windows_repository
+    is_windows = is_windows_os(repository_ctx) or "_windows_" in repository_ctx.attr.name
     if repository_ctx.attr.vendored_node:
         node_exec = "/".join([f for f in [
             "../../..",
@@ -439,37 +437,30 @@ if %errorlevel% neq 0 exit /b %errorlevel%
             for package_json in repository_ctx.attr.package_json
         ]), executable = True)
 
-    # Generate build file for this repository - exposes the node runtime and utilities generated above.
-    repository_ctx.template(
-        "generate_build_file.js",
-        repository_ctx.path(Label("//internal/node:generate_build_file.js")),
-        {
-            "TEMPLATED_is_windows": "true" if is_windows else "false",
-            "TEMPLATED_node_actual": node_entry,
-            "TEMPLATED_node_bin_actual": node_exec_label,
-            "TEMPLATED_npm_actual": npm_node_repositories_entry,
-            "TEMPLATED_vendored_node": "true" if repository_ctx.attr.vendored_node else "false",
-            "TEMPLATED_yarn_actual": yarn_node_repositories_entry,
-        },
-    )
-    host_os = os_name(repository_ctx)
-    if ("_%s" % host_os) in repository_ctx.attr.name or repository_ctx.attr.name == "nodejs":
-        # We have to use the relative path here otherwise bazel reports a cycle
-        result = repository_ctx.execute([node_entry, "generate_build_file.js"])
-    else:
-        # NOTE: Ideally we would not need this logic here and could just depend on the @nodejs//:node_bin alias but it is not possible.
-        # See: https://github.com/bazelbuild/bazel/issues/8674
-        # We have to make sure that we run repository_ctx.execute with the right node executable, so if e.g. we are in the repository containing
-        # the linux executable but on windows we need to ensure that we use the executable for windows.
-        node_path = "node.exe" if is_windows_os else "bin/node"
-
-        # NOTE: If no vendored node is provided we just assume that there exists a nodejs external repository
-        node_label = Label(node_exec_label) if repository_ctx.attr.vendored_node else Label("@nodejs_%s//:bin/nodejs/%s" % (host_os, node_path))
-        host_node = repository_ctx.path(node_label)
-        result = repository_ctx.execute([host_node, "generate_build_file.js"])
-
-    if result.return_code:
-        fail("generate_build_file.js failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))
+    # Base BUILD file for this repository
+    repository_ctx.file("BUILD.bazel", content = """# Generated by node_repositories.bzl
+package(default_visibility = ["//visibility:public"])
+exports_files([
+  "run_npm.sh.template",
+  "bin/node_repo_args.sh",{exported_node_bin}
+  "bin/node{entry_ext}",
+  "bin/npm{entry_ext}",
+  "bin/npm_node_repositories{entry_ext}",
+  "bin/yarn{entry_ext}",
+  "bin/yarn_node_repositories{entry_ext}",
+  ])
+alias(name = "node_bin", actual = "{node_bin_actual}")
+alias(name = "node", actual = "{node_actual}")
+alias(name = "npm", actual = "{npm_actual}")
+alias(name = "yarn", actual = "{yarn_actual}")
+""".format(
+        entry_ext = ".cmd" if is_windows else "",
+        exported_node_bin = "" if repository_ctx.attr.vendored_node else ("\n  \"%s\"," % node_exec_label),
+        node_bin_actual = node_exec_label,
+        node_actual = node_entry,
+        npm_actual = npm_node_repositories_entry,
+        yarn_actual = yarn_node_repositories_entry,
+    ))
 
 def _nodejs_repo_impl(repository_ctx):
     _download_node(repository_ctx)
@@ -508,8 +499,9 @@ _yarn_repo = repository_rule(
 def _nodejs_host_os_alias_impl(repository_ctx):
     host_os = os_name(repository_ctx)
     node_repository = "@nodejs_%s" % host_os
-    file_ending = ".cmd" if is_windows(host_os) else ""
-    actual_node_bin = "bin/nodejs/node.exe" if is_windows(host_os) else "bin/nodejs/bin/node"
+    is_windows_host = is_windows_os(repository_ctx)
+    file_ending = ".cmd" if is_windows_host else ""
+    actual_node_bin = "bin/nodejs/node.exe" if is_windows_host else "bin/nodejs/bin/node"
     repository_ctx.template(
         "BUILD.bazel",
         Label("@build_bazel_rules_nodejs//internal/node:BUILD.nodejs_host_os_alias.tpl"),

internal/npm_install/npm_install.bzl

@@ -22,7 +22,7 @@ See discussion in the README.
 """
 
 load("//internal/common:check_bazel_version.bzl", "check_bazel_version")
-load("//internal/common:os_name.bzl", "os_name")
+load("//internal/common:os_name.bzl", "is_windows_os")
 load("//internal/node:node_labels.bzl", "get_node_label", "get_npm_label", "get_yarn_label")
 
 COMMON_ATTRIBUTES = dict(dict(), **{
@@ -203,10 +203,9 @@ def _npm_install_impl(repository_ctx):
 
     _check_min_bazel_version("npm_install", repository_ctx)
 
-    os = os_name(repository_ctx)
-    is_windows = os.find("windows") != -1
-    node = repository_ctx.path(get_node_label(os))
-    npm = get_npm_label(os)
+    is_windows_host = is_windows_os(repository_ctx)
+    node = repository_ctx.path(get_node_label(repository_ctx))
+    npm = get_npm_label(repository_ctx)
     npm_args = ["install"]
 
     if repository_ctx.attr.prod_only:
@@ -221,7 +220,7 @@ def _npm_install_impl(repository_ctx):
         root = repository_ctx.path("")
 
     # The entry points for npm install for osx/linux and windows
-    if not is_windows:
+    if not is_windows_host:
         repository_ctx.file(
             "npm",
             content = """#!/usr/bin/env bash
@@ -267,7 +266,7 @@ cd "{root}" && "{npm}" {npm_args}
 
     repository_ctx.report_progress("Running npm install on %s" % repository_ctx.attr.package_json)
     result = repository_ctx.execute(
-        [repository_ctx.path("npm.cmd" if is_windows else "npm")],
+        [repository_ctx.path("npm.cmd" if is_windows_host else "npm")],
         timeout = repository_ctx.attr.timeout,
         quiet = repository_ctx.attr.quiet,
     )
@@ -316,9 +315,8 @@ def _yarn_install_impl(repository_ctx):
 
     _check_min_bazel_version("yarn_install", repository_ctx)
 
-    os = os_name(repository_ctx)
-    node = repository_ctx.path(get_node_label(os))
-    yarn = get_yarn_label(os)
+    node = repository_ctx.path(get_node_label(repository_ctx))
+    yarn = get_yarn_label(repository_ctx)
 
     # If symlink_node_modules is true then run the package manager
     # in the package.json folder; otherwise, run it in the root of

toolchains/node/node_toolchain.bzl

@@ -16,7 +16,7 @@ This module implements the node toolchain rule.
 """
 
 NodeInfo = provider(
-    doc = "Information about how to invoke the node binary.",
+    doc = "Information about how to invoke the node executable.",
     fields = {
         "target_tool": "A hermetically downloaded nodejs executable target for the target platform.",
         "target_tool_path": "Path to an existing nodejs executable for the target platform..",
@@ -27,7 +27,7 @@ def _node_toolchain_impl(ctx):
     if ctx.attr.target_tool and ctx.attr.target_tool_path:
         fail("Can only set one of target_tool or target_tool_path but both where set.")
     if not ctx.attr.target_tool and not ctx.attr.target_tool_path:
-        print("No nodejs binary was found or built, executing run for rules_nodejs targets might not work.")
+        fail("Must set one of target_tool or target_tool_path.")
 
     toolchain_info = platform_common.ToolchainInfo(
         nodeinfo = NodeInfo(
@@ -51,3 +51,8 @@ node_toolchain = rule(
         ),
     },
 )
+"""
+Defines a node toolchain.
+
+For usage see https://docs.bazel.build/versions/master/toolchains.html#defining-toolchains.
+"""