fix: correct precedence for Node compile cache environment variables (#3014)
In #2991 I tried to simplify this logic and introduced a slight bug as a
result. I thought it was important to set NODE_DISABLE_COMPILE_CACHE
before the node process started, but I realize now that I was mistaken
about that. Node does not enable its compile cache by default, so we are
free to wait until runtime to set NODE_DISABLE_COMPILE_CACHE=1. This
will still prevent any subsequent calls to `module.enableCompileCache()`
from enabling the cache.
This change therefore moves the logic for setting
NODE_DISABLE_COMPILE_CACHE into the bootstrap JS file, and as a side
effect it fixes things so that NODE_DISABLE_COMPILE_CACHE correctly
takes precedence over NODE_COMPILE_CACHE if both variables are set.
The motivation for this is to make it easier to move to
hermetic_launcher, which does not provide a way to set environment
variables.
---
### Changes are visible to end-users: yes
- Searched for relevant documentation and updated as needed: yes
- Breaking change (forces users to change their own code or config): no
- Suggested release notes appear below: no
### Test plan
- Covered by existing test cases
- New test cases added
diff --git a/js/private/js_binary.sh.tpl b/js/private/js_binary.sh.tpl
index 96f15b8..157d366 100644
--- a/js/private/js_binary.sh.tpl
+++ b/js/private/js_binary.sh.tpl
@@ -386,10 +386,6 @@
fi
export JS_BINARY__FS_PATCH_ROOTS
-# Disable Node's module compile cache by default (aspect-build/rules_js#2937).
-# We will re-enable it at runtime if NODE_COMPILE_CACHE is set.
-export NODE_DISABLE_COMPILE_CACHE=1
-
# Put the node wrapper directory and optionally the npm wrapper directory on the path so that
# child processes can find them.
if [ "${npm_bin_dir:-}" ]; then
diff --git a/js/private/node-bootstrap/bootstrap.cjs b/js/private/node-bootstrap/bootstrap.cjs
index 537c8c1..bcb5c6c 100644
--- a/js/private/node-bootstrap/bootstrap.cjs
+++ b/js/private/node-bootstrap/bootstrap.cjs
@@ -1,15 +1,16 @@
-// Code coverage. We load this before anything else here, so that the coverage session
-// sees as much of this process compiled under it as possible. We require coverage.cjs
-// conditionally to cut down on code size for non-test targets.
-if (process.env.JS_BINARY__COVERAGE_REPORT || process.env.COVERAGE_DIR) {
- require('./coverage.cjs')
+// Disable Node's module compile cache unless the user explicitly opted in
+// (aspect-build/rules_js#2937). Note that setting NODE_DISABLE_COMPILE_CACHE
+// at runtime has no effect unless module.enableCompileCache() is subsequently
+// called, in which case it will prevent the cache from being enabled.
+if (!process.env.NODE_COMPILE_CACHE && !process.env.NODE_DISABLE_COMPILE_CACHE) {
+ process.env.NODE_DISABLE_COMPILE_CACHE = 1
}
-// The launcher exports NODE_DISABLE_COMPILE_CACHE unconditionally, and then we re-enable
-// the cache here if necessary.
-if (process.env.NODE_COMPILE_CACHE) {
- delete process.env.NODE_DISABLE_COMPILE_CACHE
- require('node:module').enableCompileCache?.(process.env.NODE_COMPILE_CACHE)
+// Code coverage. We load this early on so that the coverage session sees as much of this
+// process compiled under it as possible. We require coverage.cjs conditionally to cut
+// down on code size for non-test targets.
+if (process.env.JS_BINARY__COVERAGE_REPORT || process.env.COVERAGE_DIR) {
+ require('./coverage.cjs')
}
const patchfs = require('./fs.cjs').patcher
diff --git a/js/private/test/image/checksum_test.expected b/js/private/test/image/checksum_test.expected
index 7f2d3e8..2948945 100644
--- a/js/private/test/image/checksum_test.expected
+++ b/js/private/test/image/checksum_test.expected
@@ -1,4 +1,4 @@
-0f7398d8d2fb575ee72775ddc412a8689f0335b76d4c2692b3fb0729e3376fb3 js/private/test/image/cksum_node.tar
+5bc8b423e2b42706a9a06b9188dd6c176ac00e63601bd9a6f71494285088dce9 js/private/test/image/cksum_node.tar
70b10220a2c05d87da4271c17c38ded8febc725e20e06f1c3809d2bb02ba4ae7 js/private/test/image/cksum_package_store_3p.tar
2cb6f678d6eb0b2e9d5e2637f41ae3f192233752f1ea2a55cede2531deec2a64 js/private/test/image/cksum_package_store_1p.tar
79afa99006aff19460354e72cf2634db693670d09ccc7531fbf27f1f20fe0a5f js/private/test/image/cksum_node_modules.tar
diff --git a/js/private/test/image/custom_layers_nomatch_test_node.listing b/js/private/test/image/custom_layers_nomatch_test_node.listing
index 3d00bf4..1774586 100644
--- a/js/private/test/image/custom_layers_nomatch_test_node.listing
+++ b/js/private/test/image/custom_layers_nomatch_test_node.listing
@@ -8,7 +8,7 @@
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 0 0 3356 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 0 0 3480 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
-r-xr-xr-x 0 0 0 37120 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
diff --git a/js/private/test/image/custom_owner_test_node.listing b/js/private/test/image/custom_owner_test_node.listing
index d54cdfe..ca25d7b 100644
--- a/js/private/test/image/custom_owner_test_node.listing
+++ b/js/private/test/image/custom_owner_test_node.listing
@@ -7,7 +7,7 @@
drwxr-xr-x 0 100 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/
drwxr-xr-x 0 100 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/
drwxr-xr-x 0 100 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 100 0 3356 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 100 0 3480 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
-r-xr-xr-x 0 100 0 37120 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
drwxr-xr-x 0 100 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
drwxr-xr-x 0 100 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
diff --git a/js/private/test/image/default_test_node.listing b/js/private/test/image/default_test_node.listing
index aca2d1f..8b6258b 100644
--- a/js/private/test/image/default_test_node.listing
+++ b/js/private/test/image/default_test_node.listing
@@ -7,7 +7,7 @@
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 0 0 3356 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 0 0 3480 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
-r-xr-xr-x 0 0 0 37120 Jan 1 1970 ./js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
diff --git a/js/private/test/image/non_ascii/custom_layer_groups_test_node.listing b/js/private/test/image/non_ascii/custom_layer_groups_test_node.listing
index 97e64a5..2790778 100644
--- a/js/private/test/image/non_ascii/custom_layer_groups_test_node.listing
+++ b/js/private/test/image/non_ascii/custom_layer_groups_test_node.listing
@@ -9,7 +9,7 @@
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 0 0 3356 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 0 0 3480 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/non_ascii/bin2.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/nodejs/
diff --git a/js/private/test/image/platform_deps/rspack_linux_arm64_test_node.listing b/js/private/test/image/platform_deps/rspack_linux_arm64_test_node.listing
index 3dabb8f..096da4d 100644
--- a/js/private/test/image/platform_deps/rspack_linux_arm64_test_node.listing
+++ b/js/private/test/image/platform_deps/rspack_linux_arm64_test_node.listing
@@ -9,7 +9,7 @@
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 0 0 3356 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 0 0 3480 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
-r-xr-xr-x 0 0 0 37120 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/rules_nodejs++node+nodejs_linux_arm64/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/platform_deps/bin.runfiles/rules_nodejs++node+nodejs_linux_arm64/bin/
diff --git a/js/private/test/image/regex_edge_cases_test_node.listing b/js/private/test/image/regex_edge_cases_test_node.listing
index 3d00bf4..1774586 100644
--- a/js/private/test/image/regex_edge_cases_test_node.listing
+++ b/js/private/test/image/regex_edge_cases_test_node.listing
@@ -8,7 +8,7 @@
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/
--r-xr-xr-x 0 0 0 3356 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
+-r-xr-xr-x 0 0 0 3480 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/bootstrap.cjs
-r-xr-xr-x 0 0 0 37120 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/_main/js/private/node-bootstrap/fs.cjs
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/
drwxr-xr-x 0 0 0 0 Jan 1 1970 ./app/js/private/test/image/bin.runfiles/rules_nodejs++node+nodejs_linux_amd64/bin/
diff --git a/js/private/test/js_binary_sh/BUILD.bazel b/js/private/test/js_binary_sh/BUILD.bazel
index 3efd3aa..ca31ee4 100644
--- a/js/private/test/js_binary_sh/BUILD.bazel
+++ b/js/private/test/js_binary_sh/BUILD.bazel
@@ -238,21 +238,22 @@
expected = "export JSON_ENCODE=\"[\\\"twx\\\"]\"",
)
-####################################################################################################
-# Node module compile cache: off by default, re-enabled by node-bootstrap/bootstrap.cjs when
-# NODE_COMPILE_CACHE is set.
+##############################################################################################
+# Node module compile cache: off by default, but can be enabled by setting NODE_COMPILE_CACHE.
# https://github.com/aspect-build/rules_js/issues/2937
write_file(
name = "write_compile_cache",
out = "compile_cache.js",
# Ask Node to enable the cache (into a sandbox-relative dir so nothing leaks out) and
- # report the resulting status. It comes back DISABLED unless NODE_COMPILE_CACHE was set.
+ # report the exact status constant it returns.
content = ["""
const { enableCompileCache, constants } = require("node:module");
const { status } = enableCompileCache("node-compile-cache");
- const disabled = status === constants.compileCacheStatus.DISABLED;
- console.log("COMPILE_CACHE_DISABLED=" + (disabled ? "yes" : "no"));
+ const name = Object.keys(constants.compileCacheStatus).find(
+ (key) => constants.compileCacheStatus[key] === status
+ );
+ console.log("COMPILE_CACHE_STATUS=" + name);
"""],
)
@@ -273,11 +274,12 @@
assert_contains(
name = "compile_cache_disabled_by_default_test",
actual = ":compile-cache-default-stdout",
- expected = "COMPILE_CACHE_DISABLED=yes",
+ expected = "COMPILE_CACHE_STATUS=DISABLED",
)
-# Opt-in: an explicit NODE_COMPILE_CACHE (relative, so it stays inside the sandbox)
-# means the cache should be left enabled. Prepares for #2938.
+# Opt-in: an explicit NODE_COMPILE_CACHE (relative, so it stays inside the sandbox) means
+# the cache should be enabled. Prepares for #2938. This variable will cause node to enable
+# the cache at startup, so we will get a status of ALREADY_ENABLED.
js_binary(
name = "compile_cache_optin",
entry_point = "compile_cache.js",
@@ -294,7 +296,7 @@
assert_contains(
name = "compile_cache_optin_test",
actual = ":compile-cache-optin-stdout",
- expected = "COMPILE_CACHE_DISABLED=no",
+ expected = "COMPILE_CACHE_STATUS=ALREADY_ENABLED",
)
# bazel run: the cache is disabled here too for now (simulated via the env attribute);
@@ -315,13 +317,13 @@
assert_contains(
name = "compile_cache_bazel_run_test",
actual = ":compile-cache-bazel-run-stdout",
- expected = "COMPILE_CACHE_DISABLED=yes",
+ expected = "COMPILE_CACHE_STATUS=DISABLED",
)
-# Node gives NODE_DISABLE_COMPILE_CACHE precedence over NODE_COMPILE_CACHE, but we do the
-# opposite.
+# If both variables are set, then NODE_DISABLE_COMPILE_CACHE wins, following node's
+# behavior.
js_binary(
- name = "compile_cache_dir_wins_over_disable",
+ name = "compile_cache_disable_wins_over_dir",
entry_point = "compile_cache.js",
env = {
"NODE_COMPILE_CACHE": "node-compile-cache",
@@ -330,14 +332,14 @@
)
js_run_binary(
- name = "_compile_cache_dir_wins_over_disable",
+ name = "_compile_cache_disable_wins_over_dir",
silent_on_success = False,
- stdout = "compile-cache-dir-wins-over-disable-stdout",
- tool = ":compile_cache_dir_wins_over_disable",
+ stdout = "compile-cache-disable-wins-over-dir-stdout",
+ tool = ":compile_cache_disable_wins_over_dir",
)
assert_contains(
- name = "compile_cache_dir_wins_over_disable_test",
- actual = ":compile-cache-dir-wins-over-disable-stdout",
- expected = "COMPILE_CACHE_DISABLED=no",
+ name = "compile_cache_disable_wins_over_dir_test",
+ actual = ":compile-cache-disable-wins-over-dir-stdout",
+ expected = "COMPILE_CACHE_STATUS=DISABLED",
)
diff --git a/js/private/test/snapshots/launcher.sh b/js/private/test/snapshots/launcher.sh
index 504b857..d006907 100644
--- a/js/private/test/snapshots/launcher.sh
+++ b/js/private/test/snapshots/launcher.sh
@@ -506,10 +506,6 @@
fi
export JS_BINARY__FS_PATCH_ROOTS
-# Disable Node's module compile cache by default (aspect-build/rules_js#2937).
-# We will re-enable it at runtime if NODE_COMPILE_CACHE is set.
-export NODE_DISABLE_COMPILE_CACHE=1
-
# Put the node wrapper directory and optionally the npm wrapper directory on the path so that
# child processes can find them.
if [ "${npm_bin_dir:-}" ]; then
