feat: use facts to auto-retrieve node versions if possible

[gzm0]

PR Checklist

Please check if your PR fulfills the following requirements:

• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• 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?

Node versions / digests are hardcoded and shipped with rules_nodejs.

What is the new behavior?

If the bazel facts feature is available (bazel >= 8.5.0) rules_nodejs retrieves required Node digests as needed and stores them in facts.

Does this PR introduce a breaking change?

• Yes
• No

Other information

Sorry for the out of the blue PR, I felt it was faster to just build this and discuss whether this is worth including on the PR directly.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9a822bde70

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[gzm0]

The new tests fail on bazel 7.x (expected). Any help on how to exclude is appreciated :) I'll start digging.

[gzm0]

@jbedard, I have updated this to only use facts for unknown node versions. For this to be easy, I've opened #3917.

To review this PR in isolation, only look at the last three commits:
https://github.com/bazel-contrib/rules_nodejs/pull/3916/changes/463749c99bb52184b632dd1ed4dd38f7c094a9d3..0db48b3bac8d50b5b264ab78fe169aeb3a0e2f4f

Note that I'm still working on fixing / excluding the new test for bazel 7.7.

[gzm0]

I'm trying to get has_facts added to bazel features so we can adjust the tests (bazel-contrib/bazel_features#147).

My plan is to exclude usage of the new node26 repositories from the e2e test, but not the definition. Keeping the definition ensures that versions that would fall back to facts don't try to call module_ctx.facts.

[gzm0]

• Added a fix for the tests that pulls in Add external_deps.has_facts bazel_features#147 via git_override.
• Marking as work in progress to prevent accidental merging.

It seems that the git_override does not work on CI. I've tested locally that e2e/nodejs_host$ bazel test ... passes.

[gzm0]

This is ready for final review.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b52161ab80

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

[jbedard]

Can't we still run the tests, it just won't be reproducible?

[gzm0]

No. With the current PR, we'll hit this line eventually:

rules_nodejs/nodejs/repositories.bzl

Lines 90 to 92 in e6706c5

	fail("No nodejs is available for {} at version {}".format(host_os, node_version) +
	"\n Consider upgrading by setting node_version in a call to node_repositories in WORKSPACE." +
	"\n Note that Node 16.x is the minimum published for Apple Silicon (M1 Macs), and 20.x is the minimum for Windows ARM64.")

(because we pass empty node_repositories).

What we can do, is to call fetch_node_repositories in the extension even if facts are not supported.
Then, if we fetched something, we can return reproducible = False. This will snapshot the repository rule invocation parameters in the lockfile (at least with sufficiently new bazel versions when I checked this last). I will take a stab at this (I'll open a separate PR).

What I would advise against is to make the repository rule itself not reproducible (especially w/o explicit opt in). IMHO this would be a step back.

[gzm0]

Alternative: #3919

[jbedard]

What we can do, is to call fetch_node_repositories in the extension even if facts are not supported.
Then, if we fetched something, we can return reproducible = False. This will snapshot the repository rule invocation parameters in the lockfile (at least with sufficiently new bazel versions when I checked this last). I will take a stab at this (I'll open a separate PR).

This sounds ideal to me to me I think, IIUC.

What I would advise against is to make the repository rule itself not reproducible (especially w/o explicit opt in). IMHO this would be a step back.

That would only be a step back when (1) there checksums are not in rules_nodejs so your new feature is used, but (2) there is no lockfile (it's not checked in) or the bazel version is too old to persist facts. So it would only be a step back when your new feature is used with old bazel versions and no lockfile? That doesn't sound horrible to me, and encourages the use of lockfiles...

We could potentially output a warning or even fail() if we had to fetch node to determine checksums but there is no facts API? That would only be for old bazel versions basically though...

[gzm0]

This sounds ideal to me to me I think, IIUC.

That's what #3919 does. So let's focus on that one.

Regarding the point about the repository rule not being reproducible:

The execution of repository rules does not get snapshotted to the lockfile (this is in contrast to module extensions). So even when running on an up-to-date bazel version with a checked-in lockfile, a non-reproducible repository rule is kind of bad.

Just to be clear: Another way of saying what I'm trying to say is we should not call fetch_node_repositories in a repository rule. We should always call it in the module extension and then call nodejs_register_toolchains with the resolved digests (both this PR and #3919 do that).

I have tested with older bazel versions down to 7.1.0 and they all snapshot the repository rule invocations and their parameters to the lockfile (like this). So we'll only end up with something non-reproducible if the lockfile is not checked in (which always was the case).