javascript: NodeJS bootstrapping via binary paths, PATH, asdf or nvm #18520

tobni · 2023-03-16T23:22:28Z

Enables (nodejs dialect) semver version specification for NodeJs binary in the nodejs subsystem.

Also enables support for asdf and nvm binary installation discovery, along with $PATH discovery.

Addresses #16959.

Either on PATH or via asdf installations.

tobni · 2023-03-16T23:24:39Z

src/python/pants/core/util_rules/external_tool.py

-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.check_version_constraints()
+class ExternalToolOptionsMixin:


I didn't find much precedence for this kind of code reuse, but I'm not sure how else to fit the square "downloaded external tool" peg into the bootstrapping round hole, as the interfaces for TemplatedExternalTool just doesn't fit semver versions.

I'm fine with it. @thejcannon : you recently had to emulate ExternalTool without actually implementing it: is there any connection to what @tobni is doing here?

tobni · 2023-03-16T23:26:29Z

3rdparty/python/requirements.txt

@@ -38,6 +38,7 @@ types-setuptools==62.6.1
 types-toml==0.10.8
 typing-extensions==4.3.0
 mypy-typing-asserts==0.1.1
+node-semver==0.9.0


I asked about this in slack https://pantsbuild.slack.com/archives/C0D7TNJHL/p1678052689358149

tobni · 2023-03-16T23:29:19Z

src/python/pants/backend/javascript/subsystems/nodejs.py

+                This option take precedence over the templated url download,
+                if a version matching the configured version range is found
+                in these paths.


This makes it less hermetic(I think?), but also what I think is most intuitive to users? I'm guessing here.

Hm, interesting. And the advantage to doing this is that it avoids the download? That's probably not necessary on an end user machine, but could make sense in CI. But in CI you could also imagine that somebody would be fine explicitly enabling a local search path (rather than having it on by default) for node as an optimization.

And the advantage to doing this is that it avoids the download?

Indeed. That, and a little bit of consistency across backend, what with go and python backends both defaulting to search_paths to find their runtimes.

That, and a little bit of consistency across backend, what with go and python backends both defaulting to search_paths to find their runtimes.

While true, that has caused us no end of issues, heh. Hermetic by default would be quite nice: it's what we have for the java/scala backends.

Flipping the switch is simple, so I'll do it!

tobni · 2023-03-16T23:30:23Z

src/python/pants/backend/javascript/subsystems/nodejs.py

+            test=BinaryPathTest(
+                ["--version"], fingerprint_stdout=False
+            ),  # Hack to retain version info


Is this too hacky? I'm annoyed by the prospect to have to iterate the paths again to call the node --version.

I think that it seems fine? AFAICT, fingerprint_stdout exists for cases where the output might be large.

More the name of the variable that gave me pause, I think. It's called "fingerprint" but is the --version output. Fingerprint usually makes me think "hash".

tobni · 2023-03-16T23:33:11Z

src/python/pants/backend/javascript/subsystems/nodejs.py

@@ -133,6 +142,8 @@ class EnvironmentAware(Subsystem.EnvironmentAware):

                * `{AsdfPathString.STANDARD}`, {AsdfPathString.STANDARD.description("Node.js")}
                * `{AsdfPathString.LOCAL}`, {AsdfPathString.LOCAL.description("binaries")}
+                * `<NVM>`, all NodeJS versions under $NVM_DIR/versions/node
+                * `<NVM_LOCAL>`, the nvm installation with the version in BUILD_ROOT/.nvmrc


Should it be possible to defer the version option to this file as well? Repeating the version in both pants.toml and .nvmrc (or .node-version, for that matter) seem like bad UX.

Should it be possible to defer the version option to this file as well?

Do you mean "infer"? Right now I don't think that it would be possible, because ExternalTool has the version as a class member.

Second best though would be to validate that both copies of the version are equal.

Yep, infer. I think I'll leave this as is for now if that's alright, validating the contents should probably happen in all backends with asdf support for example, and I think that work is a larger tangle.

stuhood

Thanks a lot!

stuhood · 2023-03-20T16:37:21Z

src/python/pants/backend/javascript/subsystems/nodejs.py

+                This option take precedence over the templated url download,
+                if a version matching the configured version range is found
+                in these paths.


Hm, interesting. And the advantage to doing this is that it avoids the download? That's probably not necessary on an end user machine, but could make sense in CI. But in CI you could also imagine that somebody would be fine explicitly enabling a local search path (rather than having it on by default) for node as an optimization.

stuhood · 2023-03-20T16:43:32Z

src/python/pants/backend/javascript/subsystems/nodejs.py

@@ -133,6 +142,8 @@ class EnvironmentAware(Subsystem.EnvironmentAware):

                * `{AsdfPathString.STANDARD}`, {AsdfPathString.STANDARD.description("Node.js")}
                * `{AsdfPathString.LOCAL}`, {AsdfPathString.LOCAL.description("binaries")}
+                * `<NVM>`, all NodeJS versions under $NVM_DIR/versions/node
+                * `<NVM_LOCAL>`, the nvm installation with the version in BUILD_ROOT/.nvmrc


Should it be possible to defer the version option to this file as well?

Do you mean "infer"? Right now I don't think that it would be possible, because ExternalTool has the version as a class member.

Second best though would be to validate that both copies of the version are equal.

src/python/pants/backend/javascript/subsystems/nodejs.py

stuhood · 2023-03-20T16:50:17Z

src/python/pants/backend/javascript/subsystems/nodejs.py

+            test=BinaryPathTest(
+                ["--version"], fingerprint_stdout=False
+            ),  # Hack to retain version info


I think that it seems fine? AFAICT, fingerprint_stdout exists for cases where the output might be large.

src/python/pants/backend/javascript/subsystems/nodejs.py

stuhood · 2023-03-20T16:55:01Z

src/python/pants/core/util_rules/external_tool.py

-    def __init__(self, *args, **kwargs):
-        super().__init__(*args, **kwargs)
-        self.check_version_constraints()
+class ExternalToolOptionsMixin:


I'm fine with it. @thejcannon : you recently had to emulate ExternalTool without actually implementing it: is there any connection to what @tobni is doing here?

src/python/pants/core/util_rules/search_paths.py

tobni added 9 commits March 16, 2023 21:34

refactor: Extract rule for finding nodejs binaries

d41c3a8

chore: Add node-semver

eab475d

refactor: Extract mixins for options for reuse

1609ad7

feat: Support semver version spec in nodejs subsystem

b32992b

chore: Ignore node-semver having no stubs

aa30b17

feat: Support finding nodejs

9bc2159

Either on PATH or via asdf installations.

feat: support nvm for binary discovery

bdb7df1

feat: Generalize searching version manager installations

b0038bd

test: Cover specific cases in a general testcase

3872ac8

tobni commented Mar 16, 2023

View reviewed changes

tobni added 4 commits March 17, 2023 00:43

fix: Use Sequence type

f17a4b9

doc: Specify format restriction

24a34fc

fix: Do not allow NVM usage in non-local environments

16bec9e

test: Mock async method the 3.7 way

88ebd5d

stuhood added the category:new feature label Mar 20, 2023

stuhood requested a review from sureshjoshi March 20, 2023 16:33

stuhood approved these changes Mar 20, 2023

View reviewed changes

tobni added 4 commits March 20, 2023 20:20

refactor: Consistent capitalization of NodeJS

8427275

chore: Lower log level to debug

5bcbcaf

doc: Add examples of version manager tools

1f0eb9e

feat: Prefer the downloaded nodejs binaries over search paths

38f1ec2

stuhood approved these changes Mar 20, 2023

View reviewed changes

stuhood enabled auto-merge (squash) March 20, 2023 21:34

stuhood merged commit 1477734 into pantsbuild:main Mar 20, 2023

tobni deleted the add/nodejs-bootstrapping branch May 16, 2023 20:15

tobni mentioned this pull request Jul 2, 2024

add binary lookup for finding nodejs install #16959

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

javascript: NodeJS bootstrapping via binary paths, PATH, asdf or nvm #18520

javascript: NodeJS bootstrapping via binary paths, PATH, asdf or nvm #18520

tobni commented Mar 16, 2023 •

edited

Loading

tobni Mar 16, 2023 •

edited

Loading

stuhood Mar 20, 2023

tobni Mar 16, 2023

tobni Mar 16, 2023

stuhood Mar 20, 2023

tobni Mar 20, 2023

stuhood Mar 20, 2023

tobni Mar 20, 2023

tobni Mar 16, 2023 •

edited

Loading

stuhood Mar 20, 2023

tobni Mar 20, 2023 •

edited

Loading

tobni Mar 16, 2023

stuhood Mar 20, 2023

tobni Mar 20, 2023

stuhood left a comment

stuhood Mar 20, 2023

stuhood Mar 20, 2023

stuhood Mar 20, 2023

stuhood Mar 20, 2023

javascript: NodeJS bootstrapping via binary paths, PATH, asdf or nvm #18520

javascript: NodeJS bootstrapping via binary paths, PATH, asdf or nvm #18520

Conversation

tobni commented Mar 16, 2023 • edited Loading

tobni Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobni Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobni Mar 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobni commented Mar 16, 2023 •

edited

Loading

tobni Mar 16, 2023 •

edited

Loading

tobni Mar 16, 2023 •

edited

Loading

tobni Mar 20, 2023 •

edited

Loading