-
Notifications
You must be signed in to change notification settings - Fork 532
feat: use facts to auto-retrieve node versions if possible #3916
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.
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
Closed
+163
−17
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
ff1c15b
feat: use facts to auto-retrieve node versions if possible
gzm0 1e4ce48
Buildifier fixes
gzm0 3d4d2f5
Only use facts for unknown node versions
gzm0 7a66f73
Fix tests (using git_override of bazel_features)
gzm0 b52161a
Use bazel features 1.49.0
gzm0 ec96ebc
Return as much extension metadata as possible
gzm0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| """Implementation of node SHASUM fetching for facts.""" | ||
|
|
||
| _REPOSITORY_TYPES = { | ||
| "darwin-arm64.tar.gz": "darwin_arm64", | ||
| "darwin-x64.tar.gz": "darwin_amd64", | ||
| "linux-x64.tar.xz": "linux_amd64", | ||
| "linux-arm64.tar.xz": "linux_arm64", | ||
| "linux-s390x.tar.xz": "linux_s390x", | ||
| "win-x64.zip": "windows_amd64", | ||
| "win-arm64.zip": "windows_arm64", | ||
| "linux-ppc64le.tar.xz": "linux_ppc64le", | ||
| } | ||
|
|
||
| def fetch_node_repositories(module_ctx, version): | ||
| """Fetches node repositories for the given node version. | ||
|
|
||
| Port of scripts/update-nodejs-versions.js | ||
|
|
||
| Args: | ||
| module_ctx: Module context | ||
| version: The node version to fetch repositories for. | ||
|
|
||
| Returns: | ||
| A dictionary in the shape of node_repositories. | ||
| """ | ||
|
|
||
| shasums_filename = "{version}-SHASUMS256.txt".format(version = version) | ||
| url = "https://nodejs.org/dist/v{version}/SHASUMS256.txt".format(version = version) | ||
|
|
||
| result = module_ctx.download(url = url, output = shasums_filename) | ||
| if not result.success: | ||
| fail("Failed to fetch node shasums:", url, result, sep = "\n") | ||
|
|
||
| shasums = module_ctx.read(shasums_filename) | ||
|
|
||
| result = {} | ||
|
|
||
| for line in shasums.splitlines(): | ||
| line = line.strip() | ||
| if not line: | ||
| continue | ||
|
|
||
| parts = line.split(" ") | ||
| if len(parts) != 2: | ||
| fail("{url} contains unexpected line:\n{line}".format( | ||
| url = url, | ||
| line = line, | ||
| )) | ||
|
|
||
| sha, filename = parts | ||
| type = _REPOSITORY_TYPES.get(filename.removeprefix("node-v%s-" % version)) | ||
| if not type: | ||
| continue | ||
|
|
||
| strip_prefix = filename.removesuffix(".tar.gz").removesuffix(".tar.xz").removesuffix(".zip") | ||
| result[version + "-" + type] = (filename, strip_prefix, sha) | ||
|
|
||
| return result |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| """Helper to get node version from user config.""" | ||
|
|
||
| def _verify_version_is_valid(version): | ||
| major, minor, patch = (version.split(".") + [None, None, None])[:3] | ||
| if not major.isdigit() or not minor.isdigit() or not patch.isdigit(): | ||
| fail("Invalid node version: %s" % version) | ||
|
|
||
| def version_from_attr(ctx, attr): | ||
| """Extract the node version from attr. | ||
|
|
||
| Verifies if the extracted version is valid. | ||
|
|
||
| Args: | ||
| ctx: repository or module context | ||
| attr: A struct with fields node_version and node_version_from_nvmrc | ||
|
|
||
| Returns: | ||
| The node version. | ||
| """ | ||
|
|
||
| node_version = attr.node_version | ||
|
|
||
| if attr.node_version_from_nvmrc: | ||
| node_version = str(ctx.read(attr.node_version_from_nvmrc)).strip() | ||
|
|
||
| _verify_version_is_valid(node_version) | ||
|
|
||
| return node_version |
|
gzm0 marked this conversation as resolved.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we still run the tests, it just won't be reproducible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. With the current PR, we'll hit this line eventually:
rules_nodejs/nodejs/repositories.bzl
Lines 90 to 92 in e6706c5
(because we pass empty node_repositories).
What we can do, is to call
fetch_node_repositoriesin 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative: #3919
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds ideal to me to me I think, IIUC.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_repositoriesin a repository rule. We should always call it in the module extension and then callnodejs_register_toolchainswith 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).