Skip to content
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

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

Merged
merged 17 commits into from Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
131 changes: 122 additions & 9 deletions src/python/pants/backend/javascript/subsystems/nodejs.py
Expand Up @@ -6,35 +6,46 @@
import os.path
from dataclasses import dataclass, field
from itertools import groupby
from typing import ClassVar, Iterable, Mapping
from typing import ClassVar, Iterable, Mapping, Collection

from nodesemver import min_satisfying

from pants.core.util_rules import asdf, search_paths, system_binaries
from pants.core.util_rules.asdf import AsdfPathString, AsdfToolPathsResult
from pants.core.util_rules.external_tool import (
DownloadedExternalTool,
ExternalToolRequest,
ExternalToolVersion,
TemplatedExternalToolOptionsMixin,
)
from pants.core.util_rules.external_tool import rules as external_tool_rules
from pants.core.util_rules.system_binaries import BinaryNotFoundError
from pants.core.util_rules.search_paths import ValidatedSearchPaths, ValidateSearchPathsRequest
from pants.core.util_rules.system_binaries import (
BinaryNotFoundError,
BinaryPath,
BinaryPathRequest,
BinaryPaths,
BinaryPathTest,
)
from pants.engine.env_vars import PathEnvironmentVariable
from pants.engine.fs import EMPTY_DIGEST, Digest, DownloadFile
from pants.engine.internals.native_engine import FileDigest
from pants.engine.platform import Platform
from pants.engine.process import Process
from pants.engine.rules import Get, Rule, collect_rules, rule
from pants.engine.unions import UnionRule
from pants.option.option_types import DictOption
from pants.option.option_types import DictOption, StrListOption
from pants.option.subsystem import Subsystem
from pants.util.docutil import bin_name
from pants.util.frozendict import FrozenDict
from pants.util.logging import LogLevel
from pants.util.ordered_set import FrozenOrderedSet
from pants.util.strutil import softwrap


class NodeJS(Subsystem, TemplatedExternalToolOptionsMixin):
options_scope = "nodejs"
help = "The NodeJS Javascript runtime (including npm and npx)."
help = "The Node.js Javascript runtime (including npm and npx)."

default_version = "v16.15.0"
default_known_versions = [
Expand Down Expand Up @@ -95,6 +106,39 @@ async def download_known_version(
download_file = DownloadFile(url, FileDigest(known_version.sha256, known_version.filesize))
return await Get(DownloadedExternalTool, ExternalToolRequest(download_file, exe))

class EnvironmentAware(Subsystem.EnvironmentAware):
env_vars_used_by_options = ("PATH",)

search_path = StrListOption(
default=["<PATH>"],
help=softwrap(
f"""
A list of paths to search for Node.js distributions.

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

Choose a reason for hiding this comment

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

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

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


You can specify absolute paths to binaries
and/or to directories containing binaries. The order of entries does
not matter.

The following special strings are supported:

For all runtime environment types:

* `<PATH>`, the contents of the PATH env var

When the environment is a `local_environment` target:

* `{AsdfPathString.STANDARD}`, {AsdfPathString.STANDARD.description("Node.js")}
* `{AsdfPathString.LOCAL}`, {AsdfPathString.LOCAL.description("binaries")}
"""
),
advanced=True,
metavar="<binary-paths>",
)


class UserChosenNodeJSResolveAliases(FrozenDict[str, str]):
pass
Expand Down Expand Up @@ -201,18 +245,83 @@ class NodeJsBootstrap:
nodejs_search_paths: tuple[str, ...]


async def _nodejs_search_paths(env_tgt: EnvironmentTarget, paths: Collection[str]) -> tuple[str, ...]:
asdf_result = await AsdfToolPathsResult.get_un_cachable_search_paths(
paths,
env_tgt=env_tgt,
tool_name="nodejs",
tool_description="Node.js distribution",
paths_option_name="[nodejs].search_path",
)
asdf_standard_tool_paths = asdf_result.standard_tool_paths
asdf_local_tool_paths = asdf_result.local_tool_paths
special_strings: dict[str, Iterable[str]] = {
AsdfPathString.STANDARD: asdf_standard_tool_paths,
AsdfPathString.LOCAL: asdf_local_tool_paths,
}

expanded: list[str] = []
for s in paths:
if s == "<PATH>":
expanded.extend(await Get(PathEnvironmentVariable, {})) # noqa: PNT30: Linear search
elif s in special_strings:
expanded.extend(special_strings[s])
else:
expanded.append(s)
return tuple(expanded)


@rule
async def nodejs_bootstrap() -> NodeJsBootstrap:
return NodeJsBootstrap(nodejs_search_paths=())
async def nodejs_bootstrap(nodejs_env_aware: NodeJS.EnvironmentAware) -> NodeJsBootstrap:
search_paths = await Get(
ValidatedSearchPaths,
ValidateSearchPathsRequest(
env_tgt=nodejs_env_aware.env_tgt,
search_paths=tuple(nodejs_env_aware.search_path),
option_origin=f"[{NodeJS.options_scope}].search_path",
environment_key="nodejs_search_path",
is_default=nodejs_env_aware._is_default("search_path"),
local_only=FrozenOrderedSet((AsdfPathString.STANDARD, AsdfPathString.LOCAL)),
),
)

expanded_paths = await _nodejs_search_paths(nodejs_env_aware.env_tgt, search_paths)

return NodeJsBootstrap(nodejs_search_paths=expanded_paths)


class _BinaryPathsPerVersion(FrozenDict[str, tuple[BinaryPath, ...]]):
pass


@rule(level=LogLevel.DEBUG, desc="Testing for Node.js binaries.")
async def get_valid_nodejs_paths_by_version(bootstrap: NodeJsBootstrap) -> _BinaryPathsPerVersion:
paths = await Get(
BinaryPaths,
BinaryPathRequest(
search_path=bootstrap.nodejs_search_paths,
binary_name="node",
test=BinaryPathTest(
["--version"], fingerprint_stdout=False
), # Hack to retain version info
Comment on lines +355 to +357
Copy link
Contributor Author

@tobni tobni Mar 16, 2023

Choose a reason for hiding this comment

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

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

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@tobni tobni Mar 20, 2023

Choose a reason for hiding this comment

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

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".

),
)

group_by_version = groupby((path for path in paths.paths), key=lambda path: path.fingerprint)
return _BinaryPathsPerVersion({version: tuple(paths) for version, paths in group_by_version})


@rule(level=LogLevel.INFO, desc="Finding Node.js distribution binaries.")
tobni marked this conversation as resolved.
Show resolved Hide resolved
async def determine_nodejs_binaries(
nodejs: NodeJS, bootstrap: NodeJsBootstrap, platform: Platform
nodejs: NodeJS, platform: Platform, paths_per_version: _BinaryPathsPerVersion
) -> NodejsBinaries:
satisfying_version = min_satisfying(paths_per_version.keys(), nodejs.version)
if satisfying_version:
return NodejsBinaries(os.path.dirname(paths_per_version[satisfying_version][0].path))

decoded_versions = groupby(
(ExternalToolVersion.decode(unparsed) for unparsed in nodejs.known_versions),
lambda version: version.version,
lambda v: v.version,
)

decoded_per_version = {
Expand All @@ -231,7 +340,8 @@ async def determine_nodejs_binaries(
f"""
Cannot find any `node` binaries satisfying the range '{nodejs.version}'.

To fix, add a `[nodejs].url_platform_mapping` version that satisfies the range.
To fix, either list a `[{NodeJS.options_scope}].url_platform_mapping` version that satisfies the range,
or ensure `[{NodeJS.options_scope}].search_path` contains a path to binaries that satisfy the range.
"""
)
)
Expand Down Expand Up @@ -268,4 +378,7 @@ def rules() -> Iterable[Rule | UnionRule]:
return (
*collect_rules(),
*external_tool_rules(),
*asdf.rules(),
*system_binaries.rules(),
*search_paths.rules(),
)
110 changes: 106 additions & 4 deletions src/python/pants/backend/javascript/subsystems/nodejs_test.py
Expand Up @@ -3,14 +3,19 @@

from __future__ import annotations

import stat
from pathlib import Path
from textwrap import dedent
from typing import NoReturn
from unittest.mock import Mock

import pytest

from pants.backend.javascript.subsystems import nodejs
from pants.backend.javascript.subsystems.nodejs import (
NodeJS,
NodeJsBootstrap,
NodejsBinaries,
_BinaryPathsPerVersion,
determine_nodejs_binaries,
)
from pants.backend.javascript.target_types import JSSourcesGeneratorTarget
Expand All @@ -21,10 +26,12 @@
ExternalToolRequest,
ExternalToolVersion,
)
from pants.core.util_rules.system_binaries import BinaryNotFoundError, BinaryPath
from pants.engine.internals.native_engine import EMPTY_DIGEST
from pants.engine.platform import Platform
from pants.engine.process import ProcessResult
from pants.testutil.rule_runner import MockGet, QueryRule, RuleRunner, run_rule_with_mocks
from pants.util.contextutil import temporary_dir


@pytest.fixture
Expand All @@ -36,6 +43,7 @@ def rule_runner() -> RuleRunner:
*config_files.rules(),
*target_types_rules.rules(),
QueryRule(ProcessResult, [nodejs.NodeJSToolProcess]),
QueryRule(NodejsBinaries, ()),
],
target_types=[JSSourcesGeneratorTarget],
)
Expand Down Expand Up @@ -96,7 +104,7 @@ def given_known_version(version: str) -> str:
pytest.param(">2.1 || <2.1", _SEMVER_1_1_0, id="or_range"),
],
)
def test_node_version_from_semver(semver_range: str, expected: str):
def test_node_version_from_semver_download(semver_range: str, expected: str) -> None:
nodejs_subsystem = Mock(spec_set=NodeJS)
nodejs_subsystem.version = semver_range
nodejs_subsystem.known_versions = [
Expand All @@ -108,16 +116,110 @@ def test_node_version_from_semver(semver_range: str, expected: str):
]
run_rule_with_mocks(
determine_nodejs_binaries,
rule_args=(nodejs_subsystem, NodeJsBootstrap(()), Platform.linux_x86_64),
rule_args=(nodejs_subsystem, Platform.linux_x86_64, _BinaryPathsPerVersion()),
mock_gets=[
MockGet(
DownloadedExternalTool,
(ExternalToolRequest,),
mock=lambda *_: DownloadedExternalTool(EMPTY_DIGEST, "myexe"),
)
),
],
)

nodejs_subsystem.download_known_version.assert_called_once_with(
ExternalToolVersion.decode(expected), Platform.linux_x86_64
)


@pytest.mark.parametrize(
("semver_range", "expected_path"),
[
pytest.param("1.x", "1/1/0", id="x_range"),
pytest.param("2.0 - 3.0", "2/1/0", id="hyphen"),
pytest.param(">2.2.0", "2/2/2", id="gt"),
pytest.param("2.2.x", "2/2/0", id="x_range_patch"),
pytest.param("~2.2.0", "2/2/0", id="thilde"),
pytest.param("^2.2.0", "2/2/0", id="caret"),
pytest.param("3.0.0", "3/0/0", id="exact"),
pytest.param("=3.0.0", "3/0/0", id="exact_equals"),
pytest.param("<3.0.0 >2.1", "2/2/0", id="and_range"),
pytest.param(">2.1 || <2.1", "1/1/0", id="or_range"),
],
)
def test_node_version_from_semver_bootstrap(semver_range: str, expected_path: str) -> None:
nodejs_subsystem = Mock(spec_set=NodeJS)
nodejs_subsystem.version = semver_range
discoverable_versions = _BinaryPathsPerVersion(
{
"1.1.0": (BinaryPath("1/1/0/node"),),
"2.1.0": (BinaryPath("2/1/0/node"),),
"2.2.0": (BinaryPath("2/2/0/node"),),
"2.2.2": (BinaryPath("2/2/2/node"),),
"3.0.0": (BinaryPath("3/0/0/node"),),
}
)

def mock_download(*_) -> NoReturn:
raise AssertionError("Should not run.")

result = run_rule_with_mocks(
determine_nodejs_binaries,
rule_args=(nodejs_subsystem, Platform.linux_x86_64, discoverable_versions),
mock_gets=[
MockGet(DownloadedExternalTool, (ExternalToolRequest,), mock=mock_download),
],
)

assert result.binary_dir == expected_path


def test_finding_no_node_version_is_an_error() -> None:
nodejs_subsystem = Mock(spec_set=NodeJS)
nodejs_subsystem.version = "*"
nodejs_subsystem.known_versions = []
discoverable_versions = _BinaryPathsPerVersion()

def mock_download(*_) -> DownloadedExternalTool:
return DownloadedExternalTool(EMPTY_DIGEST, "myexe")

with pytest.raises(BinaryNotFoundError):
run_rule_with_mocks(
determine_nodejs_binaries,
rule_args=(nodejs_subsystem, Platform.linux_x86_64, discoverable_versions),
mock_gets=[
MockGet(DownloadedExternalTool, (ExternalToolRequest,), mock=mock_download),
],
)


def mock_nodejs(version: str) -> str:
"""Return a bash script that emulates `node --version`."""
return dedent(
f"""\
#!/bin/bash

if [[ "$1" == '--version' ]]; then
echo '{version}'
fi
"""
)


def test_find_valid_binary(rule_runner: RuleRunner) -> None:
mock_binary = mock_nodejs("v3.0.0")
with temporary_dir() as tmpdir:
binary_dir = Path(tmpdir) / "bin"
binary_dir.mkdir()
binary_path = binary_dir / "node"
binary_path.write_text(mock_binary)
binary_path.chmod(binary_path.stat().st_mode | stat.S_IEXEC)

rule_runner.set_options(
[
f"--nodejs-search-path=['{binary_dir}']",
"--nodejs-version=>2",
],
env_inherit={"PATH"},
)
result = rule_runner.request(NodejsBinaries, ())
assert result.binary_dir == str(binary_dir)