Skip to content

Commit 340a9ce

Browse files
jbedardalan-agius4
andauthored
fix: validate node_version[_from_nvmrc] consistently (#3888)
Close #3887 ## PR Checklist Please check if your PR fulfills the following requirements: - [x] Tests for the changes have been added (for bug fixes / features) - [x] Docs have been added / updated (for bug fixes / features) ## PR Type What kind of change does this PR introduce? - [x] Bugfix - [ ] Feature (please, look at the "Scope of the project" section in the README.md file) - [ ] Code style update (formatting, local variables) - [ ] Refactoring (no functional changes, no api changes) - [ ] Build related changes - [ ] CI related changes - [ ] Documentation content changes - [ ] Other... Please describe: ## What is the current behavior? When using `node_version_from_nvmrc` validation is not performed and the download macro exits early without error and without downloading+extracting node. Issue Number: #3887 ## What is the new behavior? The `node_version` and `node_version_from_nvmrc` validation is aligned and fail the same. ## Does this PR introduce a breaking change? - [ ] Yes - [x] No ## Other information --------- Co-authored-by: Alan Agius <alan.agius4@gmail.com>
1 parent d584fa3 commit 340a9ce

File tree

5 files changed

+46
-26
lines changed

5 files changed

+46
-26
lines changed

e2e/smoke/BUILD.bazel

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,12 @@ write_file(
234234
content = ["v16.5.0"],
235235
)
236236

237+
write_file(
238+
name = "write_node_version_24",
239+
out = "expected_node_24",
240+
content = ["v24.12.0"],
241+
)
242+
237243
# To see what nodejs version is used by default
238244
my_nodejs(
239245
name = "run_no_toolchain",
@@ -312,3 +318,24 @@ diff_test(
312318
file1 = "write_node_version_15",
313319
file2 = "thing_toolchain_15",
314320
)
321+
322+
my_nodejs(
323+
name = "run_24",
324+
out = "thing_toolchain_24",
325+
entry_point = "version.js",
326+
# using the select statement will download toolchains for all three platforms
327+
# you can also just provide an individual toolchain if you don't want to download them all
328+
toolchain = select({
329+
"@bazel_tools//src/conditions:linux_x86_64": "@node24_linux_amd64//:toolchain",
330+
"@bazel_tools//src/conditions:linux_aarch64": "@node24_linux_arm64//:toolchain",
331+
"@bazel_tools//src/conditions:darwin_x86_64": "@node24_darwin_amd64//:toolchain",
332+
"@bazel_tools//src/conditions:darwin_arm64": "@node24_darwin_arm64//:toolchain",
333+
"@bazel_tools//src/conditions:windows": "@node24_windows_amd64//:toolchain",
334+
}),
335+
)
336+
337+
diff_test(
338+
name = "test_node_version_24",
339+
file1 = "write_node_version_24",
340+
file2 = "thing_toolchain_24",
341+
)

e2e/smoke/MODULE.bazel

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@ node.toolchain(
2222
],
2323
node_version = "15.14.0",
2424
)
25+
node.toolchain(
26+
name = "node24",
27+
node_version = "24.12.0",
28+
)
2529

2630
# FIXME(6.0): a repo rule with name=foo should create a repo named @foo, not @foo_toolchains
2731
use_repo(
@@ -34,6 +38,11 @@ use_repo(
3438
"node17_linux_amd64",
3539
"node17_linux_arm64",
3640
"node17_windows_amd64",
41+
"node24_darwin_amd64",
42+
"node24_darwin_arm64",
43+
"node24_linux_amd64",
44+
"node24_linux_arm64",
45+
"node24_windows_amd64",
3746
"nodejs_darwin_amd64",
3847
"nodejs_darwin_arm64",
3948
"nodejs_linux_amd64",

e2e/smoke/WORKSPACE.bazel

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,11 @@ nodejs_register_toolchains(
2525
node_version = "15.14.0",
2626
)
2727

28+
nodejs_register_toolchains(
29+
name = "node24",
30+
node_version = "24.12.0",
31+
)
32+
2833
http_archive(
2934
name = "npm_acorn-8.5.0",
3035
build_file_content = """load("@bazel_lib//lib:copy_directory.bzl", "copy_directory")

nodejs/private/os_name.bzl

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515
"""Helper function for repository rules
1616
"""
1717

18-
load(":node_versions.bzl", "NODE_VERSIONS")
19-
2018
OS_ARCH_NAMES = [
2119
("windows", "amd64"),
2220
("windows", "arm64"),
@@ -65,18 +63,3 @@ def os_name(rctx):
6563

6664
def is_windows_os(rctx):
6765
return rctx.os.name.find("windows") != -1
68-
69-
def node_exists_for_os(node_version, os_name, node_repositories):
70-
if not node_repositories:
71-
node_repositories = NODE_VERSIONS
72-
73-
return "-".join([node_version, os_name]) in node_repositories.keys()
74-
75-
def assert_node_exists_for_host(rctx):
76-
node_version = rctx.attr.node_version
77-
node_repositories = rctx.attr.node_repositories
78-
79-
if not node_exists_for_os(node_version, os_name(rctx), node_repositories):
80-
fail("No nodejs is available for {} at version {}".format(os_name(rctx), node_version) +
81-
"\n Consider upgrading by setting node_version in a call to node_repositories in WORKSPACE." +
82-
"\n Note that Node 16.x is the minimum published for Apple Silicon (M1 Macs), and 20.x is the minimum for Windows ARM64.")

nodejs/repositories.bzl

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
load("//nodejs/private:node_versions.bzl", "NODE_VERSIONS")
44
load("//nodejs/private:nodejs_repo_host_os_alias.bzl", "nodejs_repo_host_os_alias")
55
load("//nodejs/private:nodejs_toolchains_repo.bzl", "PLATFORMS", "nodejs_toolchains_repo")
6-
load("//nodejs/private:os_name.bzl", "assert_node_exists_for_host", "node_exists_for_os")
76

87
# Default base name for node toolchain repositories
98
# created by the module extension
@@ -85,19 +84,17 @@ def _download_node(repository_ctx):
8584
if not node_repositories.items():
8685
node_repositories = NODE_VERSIONS
8786

88-
# Skip the download if we know it will fail
89-
if not node_exists_for_os(node_version, host_os, node_repositories):
90-
return
91-
9287
node_urls = repository_ctx.attr.node_urls[:]
9388
if not node_urls:
9489
# Go back the default if the user explicitly specifies []
9590
node_urls = [DEFAULT_NODE_URL]
9691

97-
# Download node & npm
9892
version_host_os = "%s-%s" % (node_version, host_os)
99-
if not version_host_os in node_repositories:
100-
fail("Unknown Node.js version-host %s" % version_host_os)
93+
if version_host_os not in node_repositories.keys():
94+
fail("No nodejs is available for {} at version {}".format(host_os, node_version) +
95+
"\n Consider upgrading by setting node_version in a call to node_repositories in WORKSPACE." +
96+
"\n Note that Node 16.x is the minimum published for Apple Silicon (M1 Macs), and 20.x is the minimum for Windows ARM64.")
97+
10198
filename, strip_prefix, sha256 = node_repositories[version_host_os]
10299

103100
urls = [url.format(version = node_version, filename = filename) for url in node_urls]
@@ -311,7 +308,6 @@ def _verify_version_is_valid(version):
311308
fail("Invalid node version: %s" % version)
312309

313310
def _nodejs_repositories_impl(repository_ctx):
314-
assert_node_exists_for_host(repository_ctx)
315311
_download_node(repository_ctx)
316312
_prepare_node(repository_ctx)
317313

0 commit comments

Comments
 (0)