From d95ef2cfcdd9051e06f1ed05ba9dc4ab57f67e5e Mon Sep 17 00:00:00 2001 From: Kevin Cui Date: Wed, 12 Feb 2025 12:17:00 +0800 Subject: [PATCH] fix(install): reinstall same dependencies When installing duplicated dependencies, it triggers concurrency issues in Node.js (see: https://github.com/nodejs/node/pull/53534). This can be avoided by pre-deduplication. Signed-off-by: Kevin Cui --- src/cmd/install.test.ts | 29 +++++++++ src/utils/fs.test.ts | 64 +++++++++++++++++++ src/utils/npm.ts | 15 ++++- tests/fixtures/fs_copy_dir/1.txt | 0 tests/fixtures/fs_copy_dir/bar/2.txt | 0 tests/fixtures/fs_copy_dir/bar/baz/3.txt | 0 tests/fixtures/install_all/README.md | 11 +++- .../install_all/entry/package.oo.yaml | 2 + .../remote_storage/a-0.0.1/package.oo.yaml | 1 + .../remote_storage/c-0.0.2/package.oo.yaml | 3 + .../remote_storage/e-0.0.2/package.oo.yaml | 2 + .../remote_storage/e-0.0.3/package.oo.yaml | 2 + .../remote_storage/f-0.0.1/package.oo.yaml | 4 ++ 13 files changed, 129 insertions(+), 4 deletions(-) create mode 100644 src/utils/fs.test.ts create mode 100644 tests/fixtures/fs_copy_dir/1.txt create mode 100644 tests/fixtures/fs_copy_dir/bar/2.txt create mode 100644 tests/fixtures/fs_copy_dir/bar/baz/3.txt create mode 100644 tests/fixtures/install_all/remote_storage/e-0.0.2/package.oo.yaml create mode 100644 tests/fixtures/install_all/remote_storage/e-0.0.3/package.oo.yaml create mode 100644 tests/fixtures/install_all/remote_storage/f-0.0.1/package.oo.yaml diff --git a/src/cmd/install.test.ts b/src/cmd/install.test.ts index 1a25073..6f5dbbf 100644 --- a/src/cmd/install.test.ts +++ b/src/cmd/install.test.ts @@ -127,6 +127,9 @@ describe.sequential("install all", () => { publish(path.join(remoteStorage, "c-0.0.2"), ctx.registry.endpoint, "fake-token"), publish(path.join(remoteStorage, "d-0.0.1"), ctx.registry.endpoint, "fake-token"), publish(path.join(remoteStorage, "e-0.0.1"), ctx.registry.endpoint, "fake-token"), + publish(path.join(remoteStorage, "e-0.0.2"), ctx.registry.endpoint, "fake-token"), + publish(path.join(remoteStorage, "f-0.0.1"), ctx.registry.endpoint, "fake-token"), + publish(path.join(remoteStorage, "e-0.0.3"), ctx.registry.endpoint, "fake-token"), ]); } @@ -155,6 +158,8 @@ describe.sequential("install all", () => { "a-0.0.1", "b-0.0.1", "c-0.0.1", + "f-0.0.1", + "e-0.0.3", ])); expect(result.deps).toStrictEqual({ @@ -200,6 +205,27 @@ describe.sequential("install all", () => { target: expect.any(String), meta: expect.any(Object), }, + "e-0.0.2": { + name: "e", + version: "0.0.2", + isAlreadyExist: false, + target: expect.any(String), + meta: expect.any(Object), + }, + "e-0.0.3": { + name: "e", + version: "0.0.3", + isAlreadyExist: false, + target: expect.any(String), + meta: expect.any(Object), + }, + "f-0.0.1": { + name: "f", + version: "0.0.1", + isAlreadyExist: false, + target: expect.any(String), + meta: expect.any(Object), + }, }); const fileList = await globby(`**/${ooPackageName}`, { @@ -215,6 +241,9 @@ describe.sequential("install all", () => { "c-0.0.2/package.oo.yaml", "d-0.0.1/package.oo.yaml", "e-0.0.1/package.oo.yaml", + "e-0.0.2/package.oo.yaml", + "e-0.0.3/package.oo.yaml", + "f-0.0.1/package.oo.yaml", ])); }); }); diff --git a/src/utils/fs.test.ts b/src/utils/fs.test.ts new file mode 100644 index 0000000..96a128a --- /dev/null +++ b/src/utils/fs.test.ts @@ -0,0 +1,64 @@ +import { readdir } from "node:fs/promises"; +import path from "node:path"; +import { globby } from "globby"; +import { beforeEach, describe, expect, it } from "vitest"; +import { fixture } from "../../tests/helper/fs"; +import { copyDir, remove, tempDir } from "./fs"; + +beforeEach(async (ctx) => { + const temp = await tempDir(); + + ctx.workdir = temp; + + return async () => { + await remove(temp); + }; +}); + +describe.concurrent("copyDir", () => { + it("should copy dir", async (ctx) => { + const p = fixture("fs_copy_dir"); + const target = path.join(ctx.workdir, "target"); + await copyDir(p, target); + expect(await readdir(target)).toEqual(await readdir(p)); + }); + + it("should copy dir with filter", async (ctx) => { + const p = fixture("fs_copy_dir"); + const target = path.join(ctx.workdir, "target"); + + const files = await globby("**/3.txt", { + cwd: p, + dot: true, + onlyFiles: false, + gitignore: true, + absolute: true, + }); + + await copyDir(p, target, (source, _) => { + if (source === p) { + return true; + } + + return files[0].includes(source); + }); + + const files2 = await globby("**", { + cwd: target, + dot: true, + onlyFiles: false, + gitignore: true, + absolute: false, + }); + + expect(files2).toContain(path.join("bar", "baz", "3.txt")); + }); + + // see: https://github.com/nodejs/node/pull/53534 + it.skip("should copy dir with dir already exists", async (ctx) => { + const p = fixture("fs_copy_dir"); + const target = path.join(ctx.workdir, "target"); + const ps = Array.from({ length: 100 }).fill(0).map(() => copyDir(p, target)); + await Promise.all(ps); + }); +}); diff --git a/src/utils/npm.ts b/src/utils/npm.ts index 69c2542..0d7b52e 100644 --- a/src/utils/npm.ts +++ b/src/utils/npm.ts @@ -127,6 +127,12 @@ export async function transformNodeModules(dir: string) { absolute: true, }); + const map = new Map(); + const ps = result // Filter out paths that do not conform to this rule: node_modules/PACKAGE_NAME/package/package.oo.yaml .filter((p) => { @@ -137,12 +143,15 @@ export async function transformNodeModules(dir: string) { .map(async (p) => { const source = path.dirname(p); const { name, version } = await generatePackageJson(source, false); - return { + + map.set(`${name}-${version}`, { source, name, version, - }; + }); }); - return await Promise.all(ps); + await Promise.all(ps); + + return Array.from(map.values()); } diff --git a/tests/fixtures/fs_copy_dir/1.txt b/tests/fixtures/fs_copy_dir/1.txt new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/fs_copy_dir/bar/2.txt b/tests/fixtures/fs_copy_dir/bar/2.txt new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/fs_copy_dir/bar/baz/3.txt b/tests/fixtures/fs_copy_dir/bar/baz/3.txt new file mode 100644 index 0000000..e69de29 diff --git a/tests/fixtures/install_all/README.md b/tests/fixtures/install_all/README.md index 8864059..e1cf30a 100644 --- a/tests/fixtures/install_all/README.md +++ b/tests/fixtures/install_all/README.md @@ -6,9 +6,15 @@ entry a: 0.0.1 c: 0.0.2 d: 0.0.1 + e: 0.0.2 b: 0.0.1 e: 0.0.1 c: 0.0.1 + e: 0.0.2 + b: 0.0.1 + f: 0.0.1 + e: 0.0.1 + e: 0.0.3 ``` #### Local Storage (Already exists locally) @@ -26,11 +32,14 @@ b: 0.0.1 c: 0.0.2 d: 0.0.1 e: 0.0.1 +e: 0.0.2 +e: 0.0.3 +f: 0.0.1 ``` ### Installation Logic -1. Only install the `a:0.0.1` `b:0.0.1` `c:0.0.2` `d:0.0.1` `e:0.0.1` dependencies. +1. Only install the `a:0.0.1` `b:0.0.1` `c:0.0.2` `d:0.0.1` `e:0.0.1` `e:0.0.2` `f:0.0.1` `e:0.0.3` dependencies. 2. Do not install the `c:0.0.1` dependency as it is already installed locally. 3. Do install the `e:0.0.1` dependency as it is sub-dependency of `b:0.0.1`. 1. _oopm_ don't know about this sub-dependency as it is not mentioned in the `entry` package. diff --git a/tests/fixtures/install_all/entry/package.oo.yaml b/tests/fixtures/install_all/entry/package.oo.yaml index 8a34212..0d90cb7 100644 --- a/tests/fixtures/install_all/entry/package.oo.yaml +++ b/tests/fixtures/install_all/entry/package.oo.yaml @@ -4,3 +4,5 @@ dependencies: a: 0.0.1 b: 0.0.1 c: 0.0.1 + f: 0.0.1 + e: 0.0.3 diff --git a/tests/fixtures/install_all/remote_storage/a-0.0.1/package.oo.yaml b/tests/fixtures/install_all/remote_storage/a-0.0.1/package.oo.yaml index e65402d..4ddfeef 100644 --- a/tests/fixtures/install_all/remote_storage/a-0.0.1/package.oo.yaml +++ b/tests/fixtures/install_all/remote_storage/a-0.0.1/package.oo.yaml @@ -3,3 +3,4 @@ version: 0.0.1 dependencies: c: 0.0.2 d: 0.0.1 + e: 0.0.2 diff --git a/tests/fixtures/install_all/remote_storage/c-0.0.2/package.oo.yaml b/tests/fixtures/install_all/remote_storage/c-0.0.2/package.oo.yaml index 55a1e18..6ea1a8c 100644 --- a/tests/fixtures/install_all/remote_storage/c-0.0.2/package.oo.yaml +++ b/tests/fixtures/install_all/remote_storage/c-0.0.2/package.oo.yaml @@ -1,2 +1,5 @@ name: c version: 0.0.2 +dependencies: + e: 0.0.2 + b: 0.0.1 diff --git a/tests/fixtures/install_all/remote_storage/e-0.0.2/package.oo.yaml b/tests/fixtures/install_all/remote_storage/e-0.0.2/package.oo.yaml new file mode 100644 index 0000000..b5cec8c --- /dev/null +++ b/tests/fixtures/install_all/remote_storage/e-0.0.2/package.oo.yaml @@ -0,0 +1,2 @@ +name: e +version: 0.0.2 diff --git a/tests/fixtures/install_all/remote_storage/e-0.0.3/package.oo.yaml b/tests/fixtures/install_all/remote_storage/e-0.0.3/package.oo.yaml new file mode 100644 index 0000000..168650a --- /dev/null +++ b/tests/fixtures/install_all/remote_storage/e-0.0.3/package.oo.yaml @@ -0,0 +1,2 @@ +name: e +version: 0.0.3 diff --git a/tests/fixtures/install_all/remote_storage/f-0.0.1/package.oo.yaml b/tests/fixtures/install_all/remote_storage/f-0.0.1/package.oo.yaml new file mode 100644 index 0000000..4a28409 --- /dev/null +++ b/tests/fixtures/install_all/remote_storage/f-0.0.1/package.oo.yaml @@ -0,0 +1,4 @@ +name: f +version: 0.0.1 +dependencies: + e: 0.0.1