From 596018a96ed2d7f872609ee20ea65ce0b943dfac Mon Sep 17 00:00:00 2001 From: Greg Magolan Date: Thu, 4 Jul 2019 19:15:14 -0400 Subject: [PATCH] refactor(builtin): cleanup & refactoring nodejs toolchain support after review --- README.md | 2 +- internal/common/os_name.bzl | 18 ++-- internal/copy_repository/copy_repository.bzl | 5 +- internal/node/generate_build_file.js | 86 ------------------- internal/node/node.bzl | 2 +- internal/node/node_labels.bzl | 42 ++++----- internal/node/node_repositories.bzl | 68 +++++++-------- internal/npm_install/npm_install.bzl | 18 ++-- toolchains/node/node_toolchain.bzl | 9 +- ...igure.bzl => node_toolchain_configure.bzl} | 11 ++- 10 files changed, 91 insertions(+), 170 deletions(-) delete mode 100644 internal/node/generate_build_file.js rename toolchains/node/{node_configure.bzl => node_toolchain_configure.bzl} (79%) diff --git a/README.md b/README.md index 557e9cc48f..be65f97e6a 100644 --- a/README.md +++ b/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 diff --git a/internal/common/os_name.bzl b/internal/common/os_name.bzl index c0e7894517..8b629b9716 100644 --- a/internal/common/os_name.bzl +++ b/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] diff --git a/internal/copy_repository/copy_repository.bzl b/internal/copy_repository/copy_repository.bzl index 0e76cf3ff1..e6d264dfd8 100644 --- a/internal/copy_repository/copy_repository.bzl +++ b/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: diff --git a/internal/node/generate_build_file.js b/internal/node/generate_build_file.js deleted file mode 100644 index 8c51b62390..0000000000 --- a/internal/node/generate_build_file.js +++ /dev/null @@ -1,86 +0,0 @@ -/** - * @license - * Copyright 2017 The Bazel Authors. All rights reserved. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * - * You may obtain a copy of the License at - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -/** - * @fileoverview This script generates the BUILD.bazel file for NodeJS, npm & yarn. - */ -'use strict'; - -const fs = require('fs'); -const path = require('path'); - -const IS_WINDOWS = TEMPLATED_is_windows; -const IS_VENDORED_NODE = TEMPLATED_vendored_node; -const NODE_ACTUAL = 'TEMPLATED_node_actual'; -const NODE_BIN_ACTUAL = 'TEMPLATED_node_bin_actual'; -const NPM_ACTUAL = 'TEMPLATED_npm_actual'; -const YARN_ACTUAL = 'TEMPLATED_yarn_actual'; - -if (require.main === module) { - main(); -} - -/** - * Create a new directory and any necessary subdirectories - * if they do not exist. - */ -function mkdirp(p) { - if (!fs.existsSync(p)) { - mkdirp(path.dirname(p)); - fs.mkdirSync(p); - } -} - -/** - * Writes a file, first ensuring that the directory to - * write to exists. - */ -function writeFileSync(p, content) { - mkdirp(path.dirname(p)); - fs.writeFileSync(p, content); -} - -/** - * Main entrypoint. - * Write BUILD file. - */ -function main() { - generateBuildFile() -} - -module.exports = { main }; - -function generateBuildFile() { - const binaryExt = IS_WINDOWS ? '.cmd' : ''; - const exportedNodeBin = IS_VENDORED_NODE ? '' : `\n "${NODE_BIN_ACTUAL}",`; - const buildFile = `# Generated by node_repositories.bzl -package(default_visibility = ["//visibility:public"]) -exports_files([ - "run_npm.sh.template", - "bin/node_repo_args.sh",${exportedNodeBin} - "bin/node${binaryExt}", - "bin/npm${binaryExt}", - "bin/npm_node_repositories${binaryExt}", - "bin/yarn${binaryExt}", - "bin/yarn_node_repositories${binaryExt}", - ]) -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}") -` - writeFileSync('BUILD.bazel', buildFile); -} diff --git a/internal/node/node.bzl b/internal/node/node.bzl index 72bf3b0261..82c624e352 100644 --- a/internal/node/node.bzl +++ b/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. diff --git a/internal/node/node_labels.bzl b/internal/node/node_labels.bzl index 92b99d7cf0..8b4130af83 100644 --- a/internal/node/node_labels.bzl +++ b/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 diff --git a/internal/node/node_repositories.bzl b/internal/node/node_repositories.bzl index da5eedbbf2..9c22fd4c46 100644 --- a/internal/node/node_repositories.bzl +++ b/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"), diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 9092140417..95f6d2ca7c 100644 --- a/internal/npm_install/npm_install.bzl +++ b/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 diff --git a/toolchains/node/node_toolchain.bzl b/toolchains/node/node_toolchain.bzl index 098267dcd9..52baa9d1a4 100644 --- a/toolchains/node/node_toolchain.bzl +++ b/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. +""" diff --git a/toolchains/node/node_configure.bzl b/toolchains/node/node_toolchain_configure.bzl similarity index 79% rename from toolchains/node/node_configure.bzl rename to toolchains/node/node_toolchain_configure.bzl index e702cfee1b..45383c2d68 100644 --- a/toolchains/node/node_configure.bzl +++ b/toolchains/node/node_toolchain_configure.bzl @@ -12,7 +12,7 @@ # See the License for the specific language governing permissions and # limitations under the License. """ -Defines a repository rule for configuring the node binary. +Defines a repository rule for configuring the node executable. """ def _impl(repository_ctx): @@ -22,7 +22,12 @@ def _impl(repository_ctx): if repository_ctx.attr.target_tool: substitutions = {"%{TOOL_ATTRS}": " target_tool = \"%s\"\n" % repository_ctx.attr.target_tool} else: - default_tool_path = repository_ctx.attr.target_tool_path or repository_ctx.which("node") or "" + if repository_ctx.attr.target_tool_path: + default_tool_path = repository_ctx.attr.target_tool_path + else: + default_tool_path = repository_ctx.which("node") + if not default_tool_path: + fail("No node found on local path. node must available on the PATH or target_tool_path must be provided") substitutions = {"%{TOOL_ATTRS}": " target_tool_path = \"%s\"\n" % default_tool_path} repository_ctx.template( @@ -32,7 +37,7 @@ def _impl(repository_ctx): False, ) -node_configure = repository_rule( +node_toolchain_configure = repository_rule( implementation = _impl, attrs = { "target_tool": attr.label(