Skip to content

fix: validate node_version[_from_nvmrc] consistently #3888

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
jbedard wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: validate node_version[_from_nvmrc] consistently #3888

jbedard wants to merge 2 commits into from
+46 −26

Conversation

@jbedard
Copy link
Collaborator

@jbedard jbedard commented Dec 22, 2025 •
edited
Loading

Close #3887

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?

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

Other information

@jbedard jbedard force-pushed the 3887-dirs branch 3 times, most recently from 6814536 to 3f678e2 Compare December 22, 2025 21:48
@aspect-workflows
Copy link

aspect-workflows bot commented Dec 22, 2025 •
edited
Loading

Test

All tests were cache hits

1 test (100.0%) was fully cached saving 50ms.

@jbedard jbedard force-pushed the 3887-dirs branch 9 times, most recently from 182ac64 to 716bc18 Compare December 23, 2025 20:36
@jbedard jbedard force-pushed the 3887-dirs branch 2 times, most recently from 0d9166d to 5f42b37 Compare December 23, 2025 21:55
@jbedard jbedard changed the title fix: node v24+ directory structure fix: validate node_version[_from_nvmrc] consistently Jan 5, 2026
jbedard
jbedard commented Jan 5, 2026
View reviewed changes
nodejs/repositories.bzl
if not node_repositories.items():
node_repositories = NODE_VERSIONS

# Skip the download if we know it will fail
Copy link
Collaborator Author

@jbedard jbedard Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we would skip it and not fail... why? Then _prepare_node would be invoked and generate all the extra bin files despite node never been downloaded and extracted...

@jbedard jbedard requested a review from alexeagle January 5, 2026 19:26
@jbedard jbedard marked this pull request as ready for review January 5, 2026 19:26
alan-agius4
alan-agius4 reviewed Jan 6, 2026
View reviewed changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexeagle alexeagle Awaiting requested review from alexeagle alexeagle is a code owner

@mattem mattem Awaiting requested review from mattem mattem is a code owner

@gregmagolan gregmagolan Awaiting requested review from gregmagolan gregmagolan is a code owner

1 more reviewer

@alan-agius4 alan-agius4 alan-agius4 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: rules_nodejs breaks with Node.js 24.12.0

3 participants

@jbedard @alexeagle @alan-agius4