Skip to content

Commit aeff5e7

Browse files
gregmagolanalexeagle
authored andcommitted
refactor(builtin): cleanup & refactoring nodejs toolchain support after review
1 parent 9afb8db commit aeff5e7

10 files changed

Lines changed: 91 additions & 170 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ Note: the arguments passed to `bazel run` after `--` are forwarded to the execut
476476

477477
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).
478478

479-
If you have an advanced use-case you can also register your own toolchains and call `node_configure` directly to manually setup a toolchain.
479+
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.
480480

481481
#### Cross-compilation
482482

‎internal/common/os_name.bzl‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,19 +23,16 @@ OS_ARCH_NAMES = [
2323

2424
OS_NAMES = ["_".join(os_arch_name) for os_arch_name in OS_ARCH_NAMES]
2525

26-
def is_windows(os_name):
27-
return os_name == OS_NAMES[1]
28-
29-
def os_name(repository_ctx):
26+
def os_name(rctx):
3027
"""Get the os name for a repository rule
3128
3229
Args:
33-
repository_ctx: The repository rule context
30+
rctx: The repository rule context
3431
3532
Returns:
3633
A string describing the os for a repository rule
3734
"""
38-
os_name = repository_ctx.os.name.lower()
35+
os_name = rctx.os.name.lower()
3936
if os_name.startswith("mac os"):
4037
return OS_NAMES[0]
4138
elif os_name.find("windows") != -1:
@@ -44,3 +41,12 @@ def os_name(repository_ctx):
4441
return OS_NAMES[2]
4542
else:
4643
fail("Unsupported operating system: " + os_name)
44+
45+
def is_darwin_os(rctx):
46+
return os_name(rctx) == OS_NAMES[0]
47+
48+
def is_windows_os(rctx):
49+
return os_name(rctx) == OS_NAMES[1]
50+
51+
def is_linux_os(rctx):
52+
return os_name(rctx) == OS_NAMES[2]

‎internal/copy_repository/copy_repository.bzl‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,15 +15,14 @@
1515
"""Custom copy_repository rule used by npm_install and yarn_install.
1616
"""
1717

18-
load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "os_name")
18+
load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "is_windows_os")
1919

2020
def _copy_file(rctx, src):
2121
rctx.template(src.basename, src)
2222

2323
def _copy_repository_impl(rctx):
2424
src_path = "/".join(str(rctx.path(rctx.attr.marker_file)).split("/")[:-1])
25-
is_windows = os_name(rctx).find("windows") != -1
26-
if is_windows:
25+
if is_windows_os(rctx):
2726
_copy_file(rctx, rctx.path(Label("@build_bazel_rules_nodejs//internal/copy_repository:_copy.bat")))
2827
result = rctx.execute(["cmd.exe", "/C", "_copy.bat", src_path.replace("/", "\\"), "."])
2928
else:

‎internal/node/generate_build_file.js‎

Lines changed: 0 additions & 86 deletions
This file was deleted.

‎internal/node/node.bzl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414

1515
"""Executing programs
1616
17-
These rules run the node binary with the given sources.
17+
These rules run the node executable with the given sources.
1818
1919
They support module mapping: any targets in the transitive dependencies with
2020
a `module_name` attribute can be `require`d by that name.

‎internal/node/node_labels.bzl‎

Lines changed: 22 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -17,37 +17,39 @@
1717
Labels are different on windows and linux/OSX.
1818
"""
1919

20-
def get_node_label(os_name):
21-
if os_name.find("windows") != -1:
22-
label = Label("@nodejs_%s//:bin/node.cmd" % os_name)
20+
load("@build_bazel_rules_nodejs//internal/common:os_name.bzl", "is_windows_os", "os_name")
21+
22+
def get_node_label(rctx):
23+
if is_windows_os(rctx):
24+
label = Label("@nodejs_%s//:bin/node.cmd" % os_name(rctx))
2325
else:
24-
label = Label("@nodejs_%s//:bin/node" % os_name)
26+
label = Label("@nodejs_%s//:bin/node" % os_name(rctx))
2527
return label
2628

27-
def get_npm_label(os_name):
28-
if os_name.find("windows") != -1:
29-
label = Label("@nodejs_%s//:bin/npm.cmd" % os_name)
29+
def get_npm_label(rctx):
30+
if is_windows_os(rctx):
31+
label = Label("@nodejs_%s//:bin/npm.cmd" % os_name(rctx))
3032
else:
31-
label = Label("@nodejs_%s//:bin/npm" % os_name)
33+
label = Label("@nodejs_%s//:bin/npm" % os_name(rctx))
3234
return label
3335

34-
def get_npm_node_repositories_label(os_name):
35-
if os_name.find("windows") != -1:
36-
label = Label("@nodejs_%s//:bin/npm_node_repositories.cmd" % os_name)
36+
def get_npm_node_repositories_label(rctx):
37+
if is_windows_os(rctx):
38+
label = Label("@nodejs_%s//:bin/npm_node_repositories.cmd" % os_name(rctx))
3739
else:
38-
label = Label("@nodejs_%s//:bin/npm_node_repositories" % os_name)
40+
label = Label("@nodejs_%s//:bin/npm_node_repositories" % os_name(rctx))
3941
return label
4042

41-
def get_yarn_label(os_name):
42-
if os_name.find("windows") != -1:
43-
label = Label("@nodejs_%s//:bin/yarn.cmd" % os_name)
43+
def get_yarn_label(rctx):
44+
if is_windows_os(rctx):
45+
label = Label("@nodejs_%s//:bin/yarn.cmd" % os_name(rctx))
4446
else:
45-
label = Label("@nodejs_%s//:bin/yarn" % os_name)
47+
label = Label("@nodejs_%s//:bin/yarn" % os_name(rctx))
4648
return label
4749

48-
def get_yarn_node_repositories_label(os_name):
49-
if os_name.find("windows") != -1:
50-
label = Label("@nodejs%s//:bin/yarn_node_repositories.cmd" % os_name)
50+
def get_yarn_node_repositories_label(rctx):
51+
if is_windows_os(rctx):
52+
label = Label("@nodejs%s//:bin/yarn_node_repositories.cmd" % os_name(rctx))
5153
else:
52-
label = Label("@nodejs_%s//:bin/yarn_node_repositories" % os_name)
54+
label = Label("@nodejs_%s//:bin/yarn_node_repositories" % os_name(rctx))
5355
return label

‎internal/node/node_repositories.bzl‎

Lines changed: 30 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,10 @@ See https://docs.bazel.build/versions/master/skylark/repository_rules.html
2020

2121
load("//internal/common:check_bazel_version.bzl", "check_bazel_version")
2222
load("//internal/common:check_version.bzl", "check_version")
23-
load("//internal/common:os_name.bzl", "OS_ARCH_NAMES", "is_windows", "os_name")
23+
load("//internal/common:os_name.bzl", "OS_ARCH_NAMES", "is_windows_os", "os_name")
2424
load("//internal/npm_install:npm_install.bzl", "yarn_install")
2525
load("//third_party/github.com/bazelbuild/bazel-skylib:lib/paths.bzl", "paths")
26-
load("//toolchains/node:node_configure.bzl", node_toolchain_configure = "node_configure")
26+
load("//toolchains/node:node_toolchain_configure.bzl", "node_toolchain_configure")
2727
load(":node_labels.bzl", "get_yarn_node_repositories_label")
2828

2929
# Callers that don't specify a particular version will get these.
@@ -195,11 +195,9 @@ def _prepare_node(repository_ctx):
195195
Args:
196196
repository_ctx: The repository rule context
197197
"""
198-
is_windows_os = os_name(repository_ctx).find("windows") != -1
199198

200199
# TODO: Maybe we want to encode the OS as a specific attribute rather than do it based on naming?
201-
is_windows_repository = repository_ctx.attr.name.find("windows") != -1
202-
is_windows = is_windows_os or is_windows_repository
200+
is_windows = is_windows_os(repository_ctx) or "_windows_" in repository_ctx.attr.name
203201
if repository_ctx.attr.vendored_node:
204202
node_exec = "/".join([f for f in [
205203
"../../..",
@@ -439,37 +437,30 @@ if %errorlevel% neq 0 exit /b %errorlevel%
439437
for package_json in repository_ctx.attr.package_json
440438
]), executable = True)
441439

442-
# Generate build file for this repository - exposes the node runtime and utilities generated above.
443-
repository_ctx.template(
444-
"generate_build_file.js",
445-
repository_ctx.path(Label("//internal/node:generate_build_file.js")),
446-
{
447-
"TEMPLATED_is_windows": "true" if is_windows else "false",
448-
"TEMPLATED_node_actual": node_entry,
449-
"TEMPLATED_node_bin_actual": node_exec_label,
450-
"TEMPLATED_npm_actual": npm_node_repositories_entry,
451-
"TEMPLATED_vendored_node": "true" if repository_ctx.attr.vendored_node else "false",
452-
"TEMPLATED_yarn_actual": yarn_node_repositories_entry,
453-
},
454-
)
455-
host_os = os_name(repository_ctx)
456-
if ("_%s" % host_os) in repository_ctx.attr.name or repository_ctx.attr.name == "nodejs":
457-
# We have to use the relative path here otherwise bazel reports a cycle
458-
result = repository_ctx.execute([node_entry, "generate_build_file.js"])
459-
else:
460-
# NOTE: Ideally we would not need this logic here and could just depend on the @nodejs//:node_bin alias but it is not possible.
461-
# See: https://github.com/bazelbuild/bazel/issues/8674
462-
# 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
463-
# the linux executable but on windows we need to ensure that we use the executable for windows.
464-
node_path = "node.exe" if is_windows_os else "bin/node"
465-
466-
# NOTE: If no vendored node is provided we just assume that there exists a nodejs external repository
467-
node_label = Label(node_exec_label) if repository_ctx.attr.vendored_node else Label("@nodejs_%s//:bin/nodejs/%s" % (host_os, node_path))
468-
host_node = repository_ctx.path(node_label)
469-
result = repository_ctx.execute([host_node, "generate_build_file.js"])
470-
471-
if result.return_code:
472-
fail("generate_build_file.js failed: \nSTDOUT:\n%s\nSTDERR:\n%s" % (result.stdout, result.stderr))
440+
# Base BUILD file for this repository
441+
repository_ctx.file("BUILD.bazel", content = """# Generated by node_repositories.bzl
442+
package(default_visibility = ["//visibility:public"])
443+
exports_files([
444+
"run_npm.sh.template",
445+
"bin/node_repo_args.sh",{exported_node_bin}
446+
"bin/node{entry_ext}",
447+
"bin/npm{entry_ext}",
448+
"bin/npm_node_repositories{entry_ext}",
449+
"bin/yarn{entry_ext}",
450+
"bin/yarn_node_repositories{entry_ext}",
451+
])
452+
alias(name = "node_bin", actual = "{node_bin_actual}")
453+
alias(name = "node", actual = "{node_actual}")
454+
alias(name = "npm", actual = "{npm_actual}")
455+
alias(name = "yarn", actual = "{yarn_actual}")
456+
""".format(
457+
entry_ext = ".cmd" if is_windows else "",
458+
exported_node_bin = "" if repository_ctx.attr.vendored_node else ("\n \"%s\"," % node_exec_label),
459+
node_bin_actual = node_exec_label,
460+
node_actual = node_entry,
461+
npm_actual = npm_node_repositories_entry,
462+
yarn_actual = yarn_node_repositories_entry,
463+
))
473464

474465
def _nodejs_repo_impl(repository_ctx):
475466
_download_node(repository_ctx)
@@ -508,8 +499,9 @@ _yarn_repo = repository_rule(
508499
def _nodejs_host_os_alias_impl(repository_ctx):
509500
host_os = os_name(repository_ctx)
510501
node_repository = "@nodejs_%s" % host_os
511-
file_ending = ".cmd" if is_windows(host_os) else ""
512-
actual_node_bin = "bin/nodejs/node.exe" if is_windows(host_os) else "bin/nodejs/bin/node"
502+
is_windows_host = is_windows_os(repository_ctx)
503+
file_ending = ".cmd" if is_windows_host else ""
504+
actual_node_bin = "bin/nodejs/node.exe" if is_windows_host else "bin/nodejs/bin/node"
513505
repository_ctx.template(
514506
"BUILD.bazel",
515507
Label("@build_bazel_rules_nodejs//internal/node:BUILD.nodejs_host_os_alias.tpl"),

‎internal/npm_install/npm_install.bzl‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ See discussion in the README.
2222
"""
2323

2424
load("//internal/common:check_bazel_version.bzl", "check_bazel_version")
25-
load("//internal/common:os_name.bzl", "os_name")
25+
load("//internal/common:os_name.bzl", "is_windows_os")
2626
load("//internal/node:node_labels.bzl", "get_node_label", "get_npm_label", "get_yarn_label")
2727

2828
COMMON_ATTRIBUTES = dict(dict(), **{
@@ -203,10 +203,9 @@ def _npm_install_impl(repository_ctx):
203203

204204
_check_min_bazel_version("npm_install", repository_ctx)
205205

206-
os = os_name(repository_ctx)
207-
is_windows = os.find("windows") != -1
208-
node = repository_ctx.path(get_node_label(os))
209-
npm = get_npm_label(os)
206+
is_windows_host = is_windows_os(repository_ctx)
207+
node = repository_ctx.path(get_node_label(repository_ctx))
208+
npm = get_npm_label(repository_ctx)
210209
npm_args = ["install"]
211210

212211
if repository_ctx.attr.prod_only:
@@ -221,7 +220,7 @@ def _npm_install_impl(repository_ctx):
221220
root = repository_ctx.path("")
222221

223222
# The entry points for npm install for osx/linux and windows
224-
if not is_windows:
223+
if not is_windows_host:
225224
repository_ctx.file(
226225
"npm",
227226
content = """#!/usr/bin/env bash
@@ -267,7 +266,7 @@ cd "{root}" && "{npm}" {npm_args}
267266

268267
repository_ctx.report_progress("Running npm install on %s" % repository_ctx.attr.package_json)
269268
result = repository_ctx.execute(
270-
[repository_ctx.path("npm.cmd" if is_windows else "npm")],
269+
[repository_ctx.path("npm.cmd" if is_windows_host else "npm")],
271270
timeout = repository_ctx.attr.timeout,
272271
quiet = repository_ctx.attr.quiet,
273272
)
@@ -316,9 +315,8 @@ def _yarn_install_impl(repository_ctx):
316315

317316
_check_min_bazel_version("yarn_install", repository_ctx)
318317

319-
os = os_name(repository_ctx)
320-
node = repository_ctx.path(get_node_label(os))
321-
yarn = get_yarn_label(os)
318+
node = repository_ctx.path(get_node_label(repository_ctx))
319+
yarn = get_yarn_label(repository_ctx)
322320

323321
# If symlink_node_modules is true then run the package manager
324322
# in the package.json folder; otherwise, run it in the root of

‎toolchains/node/node_toolchain.bzl‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ This module implements the node toolchain rule.
1616
"""
1717

1818
NodeInfo = provider(
19-
doc = "Information about how to invoke the node binary.",
19+
doc = "Information about how to invoke the node executable.",
2020
fields = {
2121
"target_tool": "A hermetically downloaded nodejs executable target for the target platform.",
2222
"target_tool_path": "Path to an existing nodejs executable for the target platform..",
@@ -27,7 +27,7 @@ def _node_toolchain_impl(ctx):
2727
if ctx.attr.target_tool and ctx.attr.target_tool_path:
2828
fail("Can only set one of target_tool or target_tool_path but both where set.")
2929
if not ctx.attr.target_tool and not ctx.attr.target_tool_path:
30-
print("No nodejs binary was found or built, executing run for rules_nodejs targets might not work.")
30+
fail("Must set one of target_tool or target_tool_path.")
3131

3232
toolchain_info = platform_common.ToolchainInfo(
3333
nodeinfo = NodeInfo(
@@ -51,3 +51,8 @@ node_toolchain = rule(
5151
),
5252
},
5353
)
54+
"""
55+
Defines a node toolchain.
56+
57+
For usage see https://docs.bazel.build/versions/master/toolchains.html#defining-toolchains.
58+
"""

0 commit comments

Comments
 (0)