diff --git a/aws_lambda_builders/workflows/nodejs_npm/actions.py b/aws_lambda_builders/workflows/nodejs_npm/actions.py index 2cd00ff0f..3e0c0c56c 100644 --- a/aws_lambda_builders/workflows/nodejs_npm/actions.py +++ b/aws_lambda_builders/workflows/nodejs_npm/actions.py @@ -10,6 +10,7 @@ from aws_lambda_builders import utils from aws_lambda_builders.actions import ActionFailedError, BaseAction, Purpose from aws_lambda_builders.utils import extract_tarfile +from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import production_closure from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError, SubprocessNpm LOG = logging.getLogger(__name__) @@ -421,7 +422,10 @@ def __init__(self, install_dir, project_root, artifacts_dir, subprocess_npm, osu self._osutils = osutils def execute(self): - closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir) + # read the install's own record rather than paying an npm process per function + closure = production_closure(self._project_root, self._install_dir) + if closure is None: + closure = self._subprocess_npm.resolve_dependency_closure(self._install_dir) destination = os.path.join(self._artifacts_dir, "node_modules") if closure is None: @@ -562,8 +566,8 @@ def _outermost_packages(self, closure): is - hoisting it would shadow the top-level version for every other caller - and it is already reachable through the dependency that contains it, so only the outermost paths are linked. """ - # Compare through normcase, link the original path: npm's spelling and the build's can differ in - # case on Windows and still name one directory. See aws/aws-lambda-builders#935. + # Compare through normcase, link the original path: npm's spelling and the build's can differ + # in case on Windows and still name one directory. See aws/aws-lambda-builders#935. paths = [os.path.realpath(path) for path in closure] excluded = {os.path.normcase(os.path.realpath(d)) for d in (self._project_root, self._install_dir)} candidates = [path for path in paths if os.path.normcase(path) not in excluded] diff --git a/aws_lambda_builders/workflows/nodejs_npm/lockfile_closure.py b/aws_lambda_builders/workflows/nodejs_npm/lockfile_closure.py new file mode 100644 index 000000000..df2f3beed --- /dev/null +++ b/aws_lambda_builders/workflows/nodejs_npm/lockfile_closure.py @@ -0,0 +1,143 @@ +""" +Derive a workspace member's production dependency closure from a lockfile, with no npm subprocess. + +Replaces `npm ls --all --parseable --omit=dev` for the #933 artifact-narrowing step, and answers the +same shape: absolute real paths, one per resolved package. None means the caller should ask npm. + +Reads npm's HIDDEN lockfile, `node_modules/.package-lock.json`, not the project's own: only the hidden +one records the tree npm reified. See aws/aws-lambda-builders#938. +""" + +import json +import logging +import os +from typing import Dict, List, Optional, Set + +LOG = logging.getLogger(__name__) + +# lockfileVersion 1 predates the per-path `packages` map this resolver reads +MINIMUM_LOCKFILE_VERSION = 2 + +# npm's record of the tree it reified, written on every install from npm 7 on +HIDDEN_LOCKFILE = os.path.join("node_modules", ".package-lock.json") + + +def hidden_lockfile_path(project_root: str) -> str: + """Where npm records the tree it installed under `project_root`.""" + return os.path.join(project_root, HIDDEN_LOCKFILE) + + +def _describes_the_installed_tree(project_root: str) -> bool: + """ + Is npm's hidden lockfile still a true record of `node_modules`? + + npm's own staleness test, which it applies before trusting the file itself. `>=`, not `>`: a fresh + install writes both in one clock tick. See aws/aws-lambda-builders#938. + """ + try: + hidden = os.path.getmtime(hidden_lockfile_path(project_root)) + tree = os.path.getmtime(os.path.join(project_root, "node_modules")) + except OSError: + return False + return hidden >= tree + + +def _resolve_key(packages: Dict, from_key: str, name: str) -> Optional[str]: + """ + Find the packages-map key that `name` resolves to when required from `from_key`. + + Walks up looking for `node_modules/` the way node does. `from_key` is "" at the root, + else a member path like "endpoints/orders" or "node_modules/express". + """ + prefix = from_key + while True: + candidate = f"{prefix}/node_modules/{name}" if prefix else f"node_modules/{name}" + if candidate in packages: + return candidate + if not prefix: + return None + # strip one path segment and try again, which is what walking up node_modules means + prefix = prefix.rsplit("/", 1)[0] if "/" in prefix else "" + if prefix.endswith("/node_modules"): + prefix = prefix[: -len("/node_modules")] + + +def _entry_dir(project_root: str, key: str, entry: Dict) -> str: + """The directory a packages-map entry lives in.""" + # a workspace link points at the real source directory rather than the link's own path + if entry.get("link") and entry.get("resolved"): + return os.path.join(project_root, *entry["resolved"].split("/")) + return os.path.join(project_root, *key.split("/")) if key else project_root + + +def production_closure(project_root: str, install_dir: str) -> Optional[List[str]]: + """ + The production dependency closure of `install_dir`, as absolute directories. + + None when the hidden lockfile cannot answer: absent, stale, too old a version, unreadable, or + silent about this install directory. + """ + if not _describes_the_installed_tree(project_root): + LOG.debug("NODEJS no current hidden lockfile under %s; asking npm instead", project_root) + return None + + try: + with open(hidden_lockfile_path(project_root)) as handle: + lock = json.load(handle) + except (OSError, ValueError): + return None + + if lock.get("lockfileVersion", 1) < MINIMUM_LOCKFILE_VERSION or "packages" not in lock: + return None + + packages = lock["packages"] + project_root = os.path.realpath(project_root) + rel = os.path.relpath(os.path.realpath(install_dir), project_root).replace(os.sep, "/") + start = "" if rel == "." else rel + if start not in packages: + return None + + # a link entry for a workspace member carries no dependencies; its source entry does + def deps_of(key: str) -> Dict[str, str]: + entry = packages.get(key) or {} + if entry.get("link") and entry.get("resolved"): + entry = packages.get(entry["resolved"], entry) + # Production only. Peers included because npm 7+ installs them and `npm ls --all` walks them; + # a declined optional peer is dropped by the on-disk check below. + out = dict(entry.get("dependencies") or {}) + out.update(entry.get("optionalDependencies") or {}) + out.update(entry.get("peerDependencies") or {}) + return out + + found: Set[str] = set() + stack = [start] + while stack: + key = stack.pop() + for name in deps_of(key): + resolved = _resolve_key(packages, key, name) + if resolved is None or resolved in found: + continue + entry = packages[resolved] + if entry.get("dev"): + continue + found.add(resolved) + # resolve a link's dependencies from its source directory, as node does through the real + # path: walking up from the link would find a hoisted copy the source nests + if entry.get("link") and entry.get("resolved"): + stack.append(entry["resolved"]) + else: + stack.append(resolved) + + # Keep only what is on disk, as npm ls does: an optional dependency excluded by os/cpu/libc is + # listed but not installed. + dirs = set() + for key in found: + directory = _entry_dir(project_root, key, packages[key]) + if os.path.isdir(directory): + dirs.add(directory) + else: + LOG.debug("NODEJS lockfile lists %s but it is not installed; leaving it out", key) + # npm --parseable prints these too and the caller filters them, so emit them for parity + dirs.add(project_root) + dirs.add(os.path.realpath(install_dir)) + return sorted(dirs) diff --git a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py index c3929f2e2..a3690499f 100644 --- a/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py +++ b/tests/integration/workflows/nodejs_npm/test_nodejs_npm.py @@ -13,6 +13,7 @@ from aws_lambda_builders.builder import LambdaBuilder from aws_lambda_builders.exceptions import WorkflowFailedError from aws_lambda_builders.supported_runtimes import NODEJS_RUNTIMES +from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import production_closure from aws_lambda_builders.workflows.nodejs_npm.npm import SubprocessNpm from aws_lambda_builders.workflows.nodejs_npm.utils import EXPERIMENTAL_FLAG_NODEJS_MONOREPO, OSUtils from tests.testing_utils import read_link_without_junction_prefix @@ -538,6 +539,58 @@ def test_build_in_source_in_workspaces_monorepo_links_the_hoisted_dependencies(s ) self.assertEqual(require_handler.returncode, 0, require_handler.stderr) + def test_the_lockfile_closure_agrees_with_npm_ls_on_a_real_install(self): + # the property that lets the lockfile replace `npm ls`, and one a unit test cannot establish: + # it would compare this code against a fixture written from this code + monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo") + npm = SubprocessNpm(OSUtils()) + npm.run(["install", "--silent", "--no-audit", "--no-fund"], cwd=monorepo_dir) + + for endpoint in ("fn", "other"): + install_dir = os.path.join(monorepo_dir, "endpoints", endpoint) + from_npm = npm.resolve_dependency_closure(install_dir) + self.assertIsNotNone(from_npm, "npm ls must answer, or this test proves nothing") + from_lockfile = production_closure(monorepo_dir, install_dir) + self.assertIsNotNone(from_lockfile, "npm's hidden lockfile must be readable by the resolver") + + normalise = {os.path.normcase(os.path.realpath(p)) for p in from_npm} + self.assertEqual( + {os.path.normcase(os.path.realpath(p)) for p in from_lockfile}, + normalise, + f"the two closure sources disagree for endpoints/{endpoint}", + ) + + def test_a_manifest_the_project_lockfile_has_not_recorded_still_reaches_the_artifacts(self): + # `npm install --no-save` installs a newly declared dependency without writing + # package-lock.json, so a closure read from that file would drop it + monorepo_dir = os.path.join(self.temp_testdata_dir, "workspaces-monorepo") + source_dir = os.path.join(monorepo_dir, "endpoints", "fn") + npm = SubprocessNpm(OSUtils()) + npm.run(["install", "--silent", "--no-audit", "--no-fund"], cwd=monorepo_dir) + + # declare a dependency the lockfile knows nothing about, exactly as editing package.json does + manifest_path = os.path.join(source_dir, "package.json") + with open(manifest_path) as manifest: + manifest_json = json.load(manifest) + manifest_json["dependencies"]["ms"] = "^2.1.3" + with open(manifest_path, "w") as manifest: + json.dump(manifest_json, manifest) + with open(os.path.join(monorepo_dir, "package-lock.json")) as lockfile: + self.assertNotIn("ms", json.load(lockfile)["packages"]["endpoints/fn"].get("dependencies", {})) + + self.builder.build( + source_dir, + self.artifacts_dir, + self.scratch_dir, + manifest_path, + # one runtime: this pins the closure source, which no runtime varies + runtime=NODEJS_RUNTIMES[-1], + build_in_source=True, + experimental_flags=[EXPERIMENTAL_FLAG_NODEJS_MONOREPO], + ) + + self.assertIn("ms", set(os.listdir(os.path.join(self.artifacts_dir, "node_modules")))) + @parameterized.expand(SUPPORTED_RUNTIMES) def test_build_in_source_with_removed_dependencies(self, runtime): # run a build with default requirements and confirm dependencies are downloaded diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json index 3fa3f05a6..46512ef26 100644 --- a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/package-lock.json @@ -42,6 +42,13 @@ "resolved": "packages/shared", "link": true }, + "node_modules/fake-http-request": { + "version": "1.4.0", + "resolved": "https://registry.npmjs.org/fake-http-request/-/fake-http-request-1.4.0.tgz", + "integrity": "sha512-wepY7hl//Guu08EyxL9MCghPhOQQDzWnmw7/U9gVmX0Hr4/0e6eaUIlHJeMbzu+6wDeaY8mzy8Sb7vu6hSIQBQ==", + "license": "MIT", + "peer": true + }, "node_modules/minimal-request-promise": { "version": "1.3.0", "resolved": "https://registry.npmjs.org/minimal-request-promise/-/minimal-request-promise-1.3.0.tgz", @@ -57,7 +64,10 @@ "packages/shared": { "name": "@nodejs-workspaces-monorepo/shared", "version": "1.0.0", - "license": "APACHE2.0" + "license": "APACHE2.0", + "peerDependencies": { + "fake-http-request": "^1.4.0" + } } } } diff --git a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json index 6d6cdff53..383a761bb 100644 --- a/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json +++ b/tests/integration/workflows/nodejs_npm/testdata/workspaces-monorepo/packages/shared/package.json @@ -3,5 +3,8 @@ "version": "1.0.0", "license": "APACHE2.0", "main": "index.js", - "files": ["index.js"] + "files": ["index.js"], + "peerDependencies": { + "fake-http-request": "^1.4.0" + } } diff --git a/tests/unit/workflows/nodejs_npm/test_actions.py b/tests/unit/workflows/nodejs_npm/test_actions.py index 3ded102e8..9331a4c7a 100644 --- a/tests/unit/workflows/nodejs_npm/test_actions.py +++ b/tests/unit/workflows/nodejs_npm/test_actions.py @@ -18,6 +18,7 @@ NodejsNpmCIAction, NodejsNpmTestAction, ) +from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import hidden_lockfile_path from aws_lambda_builders.workflows.nodejs_npm.npm import NpmExecutionError from aws_lambda_builders.workflows.nodejs_npm.utils import OSUtils @@ -312,6 +313,16 @@ def _action(self): osutils=self.osutils, ) + def _hidden_lockfile(self, version, packages): + # the only lockfile the action reads, and it must not be older than the tree + path = hidden_lockfile_path(self.root) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as handle: + json.dump({"lockfileVersion": version, "packages": packages}, handle) + tree = os.path.getmtime(os.path.join(self.root, "node_modules")) + os.utime(path, (tree + 1, tree + 1)) + return path + def _linked(self): node_modules = os.path.join(self.artifacts_dir, "node_modules") found = set() @@ -322,6 +333,47 @@ def _linked(self): found.add(entry) return found + def test_a_usable_hidden_lockfile_answers_the_closure_without_running_npm(self): + # the whole point of reading the lockfile: no npm process per function + self._package("endpoints/fn", "@mono/fn") + self._package("node_modules/lodash", "lodash") + self._hidden_lockfile( + 3, + { + "": {"workspaces": ["endpoints/fn"]}, + "endpoints/fn": {"dependencies": {"lodash": "^4.0.0"}}, + "node_modules/lodash": {"version": "4.17.21"}, + }, + ) + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash"}) + self.subprocess_npm.resolve_dependency_closure.assert_not_called() + + def test_a_hidden_lockfile_that_cannot_answer_falls_back_to_npm(self): + # version 1 carries no per-path map, so the fallback is the only thing that can produce a closure + self._package("endpoints/fn", "@mono/fn") + mine = self._package("node_modules/lodash", "lodash") + self._hidden_lockfile(1, {}) + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, mine] + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash"}) + self.subprocess_npm.resolve_dependency_closure.assert_called_once_with(self.install_dir) + + def test_no_hidden_lockfile_falls_back_to_npm(self): + # the pre-npm-7 and other-package-manager case: every existing closure test runs this path + self._package("endpoints/fn", "@mono/fn") + mine = self._package("node_modules/lodash", "lodash") + self.subprocess_npm.resolve_dependency_closure.return_value = [self.root, self.install_dir, mine] + + self._action().execute() + + self.assertEqual(self._linked(), {"lodash"}) + self.subprocess_npm.resolve_dependency_closure.assert_called_once_with(self.install_dir) + def test_links_only_this_function_s_dependencies(self): self._package("endpoints/fn", "@mono/fn") mine = self._package("node_modules/lodash", "lodash") diff --git a/tests/unit/workflows/nodejs_npm/test_lockfile_closure.py b/tests/unit/workflows/nodejs_npm/test_lockfile_closure.py new file mode 100644 index 000000000..d8c41ce24 --- /dev/null +++ b/tests/unit/workflows/nodejs_npm/test_lockfile_closure.py @@ -0,0 +1,165 @@ +import json +import os +import shutil +import tempfile +from unittest import TestCase + +from aws_lambda_builders.workflows.nodejs_npm.lockfile_closure import hidden_lockfile_path, production_closure + + +class TestProductionClosure(TestCase): + """ + Feeds production_closure a synthetic workspace tree: two members with disjoint dependencies, + a shared workspace package, and a version conflict (the shared package nests its own copy of + a package one member also depends on). + + The lockfile written here is npm's hidden one, `node_modules/.package-lock.json`, the only file + the resolver reads. + """ + + def setUp(self): + self.project_root = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, self.project_root, ignore_errors=True) + + self.lock = { + "name": "mono", + "lockfileVersion": 3, + "packages": { + "": {"name": "mono", "workspaces": ["functions/*", "packages/*"]}, + "functions/fn-a": {"dependencies": {"@mono/shared": "^1.0.0", "lodash": "^4.17.20"}}, + "functions/fn-b": {"dependencies": {"@mono/shared": "^1.0.0", "axios": "^1.6.0"}}, + "packages/shared": {"version": "1.0.0", "dependencies": {"lodash": "4.17.15"}}, + "node_modules/@mono/fn-a": {"link": True, "resolved": "functions/fn-a"}, + "node_modules/@mono/fn-b": {"link": True, "resolved": "functions/fn-b"}, + "node_modules/@mono/shared": {"link": True, "resolved": "packages/shared"}, + "node_modules/lodash": {"version": "4.17.20"}, + "node_modules/axios": {"version": "1.6.0", "dependencies": {"follow-redirects": "^1.15.0"}}, + "node_modules/follow-redirects": {"version": "1.15.6"}, + "packages/shared/node_modules/lodash": {"version": "4.17.15"}, + "node_modules/eslint": {"version": "9.0.0", "dev": True}, + }, + } + for key in self.lock["packages"]: + if key: + os.makedirs(os.path.join(self.project_root, *key.split("/")), exist_ok=True) + self.lockfile_path = self._write_lockfile(self.lock) + + def _write_lockfile(self, lock): + path = hidden_lockfile_path(self.project_root) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as handle: + json.dump(lock, handle) + # the resolver refuses one older than the tree, and these tests make the folders first + tree = os.path.getmtime(os.path.join(self.project_root, "node_modules")) + os.utime(path, (tree + 1, tree + 1)) + return path + + def _closure(self, member): + install_dir = os.path.join(self.project_root, *member.split("/")) + return production_closure(self.project_root, install_dir) + + def test_member_closure_holds_own_dependencies_and_shared_package(self): + closure = self._closure("functions/fn-b") + + expected = { + os.path.realpath(self.project_root), + os.path.join(os.path.realpath(self.project_root), "functions", "fn-b"), + os.path.join(os.path.realpath(self.project_root), "packages", "shared"), + os.path.join(os.path.realpath(self.project_root), "node_modules", "axios"), + os.path.join(os.path.realpath(self.project_root), "node_modules", "follow-redirects"), + os.path.join(os.path.realpath(self.project_root), "packages", "shared", "node_modules", "lodash"), + } + self.assertEqual(set(closure), expected) + + def test_conflicting_version_resolves_to_the_nested_copy_not_the_hoisted_one(self): + # fn-b never depends on lodash itself, so the hoisted 4.17.20 must not appear - resolving the + # shared package from its link path instead of its source would find it + closure = self._closure("functions/fn-b") + + hoisted = os.path.join(os.path.realpath(self.project_root), "node_modules", "lodash") + nested = os.path.join(os.path.realpath(self.project_root), "packages", "shared", "node_modules", "lodash") + self.assertNotIn(hoisted, closure) + self.assertIn(nested, closure) + + def test_member_depending_on_the_hoisted_version_gets_both_copies(self): + # fn-a takes the hoisted copy, and its shared dependency still carries the nested one + closure = self._closure("functions/fn-a") + + self.assertIn(os.path.join(os.path.realpath(self.project_root), "node_modules", "lodash"), closure) + self.assertIn( + os.path.join(os.path.realpath(self.project_root), "packages", "shared", "node_modules", "lodash"), closure + ) + + def test_peer_dependencies_are_followed(self): + # npm 7+ installs peers and `npm ls --all` walks them, so a peer-only edge is still resolved + os.makedirs(os.path.join(self.project_root, "node_modules", "peer-host"), exist_ok=True) + self.lock["packages"]["packages/shared"]["peerDependencies"] = {"peer-host": "^1.0.0"} + self.lock["packages"]["node_modules/peer-host"] = {"version": "1.0.0"} + self.lockfile_path = self._write_lockfile(self.lock) + + closure = self._closure("functions/fn-b") + + self.assertIn(os.path.join(os.path.realpath(self.project_root), "node_modules", "peer-host"), closure) + + def test_an_optional_peer_the_install_declined_is_left_out(self): + # the lockfile declares the peer but npm installed nothing, so there is no directory to link + self.lock["packages"]["packages/shared"]["peerDependencies"] = {"absent-peer": "^1.0.0"} + self.lock["packages"]["packages/shared"]["peerDependenciesMeta"] = {"absent-peer": {"optional": True}} + self.lockfile_path = self._write_lockfile(self.lock) + + closure = self._closure("functions/fn-b") + + self.assertNotIn(os.path.join(os.path.realpath(self.project_root), "node_modules", "absent-peer"), closure) + + def test_dev_dependencies_are_left_out(self): + self.lock["packages"]["functions/fn-a"]["devDependencies"] = {"eslint": "^9.0.0"} + self.lockfile_path = self._write_lockfile(self.lock) + + closure = self._closure("functions/fn-a") + + self.assertNotIn(os.path.join(os.path.realpath(self.project_root), "node_modules", "eslint"), closure) + + def test_entry_not_on_disk_is_left_out(self): + shutil.rmtree(os.path.join(self.project_root, "node_modules", "follow-redirects")) + + closure = self._closure("functions/fn-b") + + self.assertNotIn(os.path.join(os.path.realpath(self.project_root), "node_modules", "follow-redirects"), closure) + + def test_version_1_lockfile_answers_none(self): + self.lock["lockfileVersion"] = 1 + del self.lock["packages"] + self.lockfile_path = self._write_lockfile(self.lock) + + self.assertIsNone(self._closure("functions/fn-a")) + + def test_install_dir_the_lockfile_does_not_describe_answers_none(self): + os.makedirs(os.path.join(self.project_root, "functions", "fn-unknown")) + + self.assertIsNone(self._closure("functions/fn-unknown")) + + def test_unreadable_lockfile_answers_none(self): + with open(self.lockfile_path, "w") as handle: + handle.write("not json") + + self.assertIsNone(self._closure("functions/fn-a")) + + def test_a_hidden_lockfile_older_than_the_tree_answers_none(self): + # npm's own rule: another tool mutating node_modules invalidates the hidden lockfile + tree = os.path.getmtime(os.path.join(self.project_root, "node_modules")) + os.utime(self.lockfile_path, (tree - 60, tree - 60)) + + self.assertIsNone(self._closure("functions/fn-a")) + + def test_a_hidden_lockfile_as_recent_as_the_tree_is_trusted(self): + # equal mtimes are what a fresh install produces, so `>` would fall back on every first build + tree = os.path.getmtime(os.path.join(self.project_root, "node_modules")) + os.utime(self.lockfile_path, (tree, tree)) + + self.assertIsNotNone(self._closure("functions/fn-a")) + + def test_no_hidden_lockfile_at_all_answers_none(self): + # npm 6 never wrote one, and neither does a tree another package manager built + os.remove(self.lockfile_path) + + self.assertIsNone(self._closure("functions/fn-a"))