Skip to content

[package-extractor]: fix issue #5719, preventing duplicate-copy conflicts across npm-packlist versions #5720

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.

Sign up for GitHub

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

Merged
octogonz merged 5 commits into from
Mar 25, 2026
+77 −2
Merged

[package-extractor]: fix issue #5719, preventing duplicate-copy conflicts across npm-packlist versions #5720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bce3791
fix(package-extractor): preventing duplicate-copy conflicts across np…
LPegasus Mar 24, 2026
316cd18
docs(README.md): update readme
LPegasus Mar 24, 2026
fa6d3ed
Fix different behavior on Windows that was breaking the unit test
octogonz Mar 24, 2026
0ba8c2c
Prepare to publish a PATCH version of Rush
octogonz Mar 25, 2026
4f3a77e
Merge branch 'main' into rush-issue-5719
octogonz Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ These GitHub repositories provide supplementary resources for Rush Stack:
| [/build-tests/package-extractor-test-02](./build-tests/package-extractor-test-02/) | This project is used by tests in the @rushstack/package-extractor package. |
| [/build-tests/package-extractor-test-03](./build-tests/package-extractor-test-03/) | This project is used by tests in the @rushstack/package-extractor package. |
| [/build-tests/package-extractor-test-04](./build-tests/package-extractor-test-04/) | This project is used by tests in the @rushstack/package-extractor package. |
| [/build-tests/package-extractor-test-05](./build-tests/package-extractor-test-05/) | This project is used by tests in the @rushstack/package-extractor package. |
| [/build-tests/run-scenarios-helpers](./build-tests/run-scenarios-helpers/) | Helpers for the *-scenarios test projects. |
| [/build-tests/rush-amazon-s3-build-cache-plugin-integration-test](./build-tests/rush-amazon-s3-build-cache-plugin-integration-test/) | Tests connecting to an amazon S3 endpoint |
| [/build-tests/rush-lib-declaration-paths-test](./build-tests/rush-lib-declaration-paths-test/) | This project ensures all of the paths in rush-lib/lib/... have imports that resolve correctly. If this project builds, all `lib/**/*.d.ts` files in the `@microsoft/rush-lib` package are valid. |
Expand Down
16 changes: 16 additions & 0 deletions build-tests/package-extractor-test-05/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"name": "package-extractor-test-05",
"description": "This project is used by tests in the @rushstack/package-extractor package.",
"version": "1.0.0",
"private": true,
"main": "./dist/index.js",
"files": [
"dist"
],
"scripts": {
"_phase:build": ""
},
"dependencies": {
"@rushstack/node-core-library": "workspace:*"
}
}
1 change: 1 addition & 0 deletions build-tests/package-extractor-test-05/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export {};
10 changes: 10 additions & 0 deletions common/changes/@microsoft/rush/rush-issue-5719_2026-03-25-00-05.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@microsoft/rush",
"comment": "Fix an issue where \"rush deploy\" could fail with EEXIST due to a \"npm-packlist\" regression (GitHub #5720)",
"type": "none"
}
],
"packageName": "@microsoft/rush"
}
10 changes: 10 additions & 0 deletions common/changes/@rushstack/package-extractor/rush-issue-5719_2026-03-24-11-42.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "@rushstack/package-extractor",
"comment": "preventing duplicate-copy conflicts across npm-packlist versions",
"type": "patch"
}
],
"packageName": "@rushstack/package-extractor"
}
2 changes: 1 addition & 1 deletion common/config/rush/version-policies.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
"policyName": "rush",
"definitionName": "lockStepVersion",
"version": "5.172.0",
"nextBump": "minor",
"nextBump": "patch",
"mainProject": "@microsoft/rush"
}
]
6 changes: 6 additions & 0 deletions common/config/subspaces/default/pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 14 additions & 1 deletion libraries/package-extractor/src/PackageExtractor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,20 @@ export class PackageExtractor {
}
);
const npmPackFiles: string[] = await walkerPromise;
return npmPackFiles;

// npm-packlist@v5 (uses glob@v8) returns "./dist/index.js" for pattern: "./dist/index.js",
// whereas npm-packlist@v2 (uses glob@v7) returns an empty array.
// This may cause duplicate files in the result list, leading to copying conflicts
// in the AssetHandler.ts includeAssetAsync method.
//
// Temporary fix: normalize the file paths to be relative to the package root, and remove duplicates.
//
// TODO: A better long-term fix is to replace "npm-packlist" with "@pnpm/fs.packlist", notes here:
// https://github.com/microsoft/rushstack/pull/5720/changes#r2984873283
const normalizedFiles: string[] = Array.from(
new Set(npmPackFiles.map((file) => path.posix.normalize(file)))
);
return normalizedFiles;
Copy link
Collaborator

@octogonz octogonz Mar 24, 2026

Choose a reason for hiding this comment

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

Key questions for this change:

  1. Is this a perf concern? We are synchronously calling path.normalize() potentially thousands of times, and allocating multiple data structures containing thousands of elements. How many thousands? It is not the full --create-archive list from rush deploy, but more like the total number of files under a single Rush project. An imperative loop here could be more efficient for time/memory, but the costs are probably already tiny compared to the preceding work required to construct npmPackFiles.

  2. Is it the right fix? The problem did not exist with npm-packlist@2, it is apparently npm-packlist@5 that started returning duplicate paths. Why should a packlist contain duplicates? Aren't duplicate paths a bug? If so, then this "fix" is reversing the effects of a bug, which wouldn't be necessary if we simply fix the bug. (More evidence: npm-packlist is maintained by NPM, Inc., who are known for "how can this have no unit test???" type regressions.)

    If it's a bug, then other tools that use npm-packlist should have the same bug. And here we go, PNPM seems to have encountered the same issue:

    fix: packing the same file twice during publish pnpm/pnpm#7250

    That diff interestingly has a similar deduplication line:

    const files = Array.from(new Set((await packlist(src)).map((f) => path.join(f))))

    ...but if you look closely, it is not calling path.normalize(). It seems to be deduplicating for some other reason. PNPM's real fix was to upgrade npm-packlist:

    The fix was to update npm-packlist to the latest version.

    ...which at the time, they accomplished by introducing a wrapper package @pnpm/fs.packlist.

Copy link
Collaborator

@octogonz octogonz Mar 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

We can't downgrade, because that would undo the benefit of PR #5575.

So, one alternative fix is to upgrade to npm-packlist (whose latest version is now already 10.x).

BUT, consider that PNPM's @pnpm/fs.packlist has eliminated its dependency on arborist, so today it is merely:

  • A hardwired dependency on known-good "npm-packlist": "5.1.3"
  • A small amount of normalizing/deduplicating logic in index.ts.

This seems like exactly what rush deploy needs. And Rush primarily uses PNPM, I think we would get more consistent/stable results by replacing our npm-packlist with @pnpm/fs.packlist. That seems like the best solution.

Copy link
Collaborator

@octogonz octogonz Mar 24, 2026

Choose a reason for hiding this comment

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

That said:

So my recommendation is to merge+publish PR #5720, then do a follow up PR to implement the @pnpm/fs.packlist approach.

}

/**
Expand Down
12 changes: 12 additions & 0 deletions libraries/package-extractor/src/test/PackageExtractor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,17 @@ const project1PackageName: string = 'package-extractor-test-01';
const project2PackageName: string = 'package-extractor-test-02';
const project3PackageName: string = 'package-extractor-test-03';
const project4PackageName: string = 'package-extractor-test-04';
const project5PackageName: string = 'package-extractor-test-05';
const project1RelativePath: string = path.join('build-tests', project1PackageName);
const project2RelativePath: string = path.join('build-tests', project2PackageName);
const project3RelativePath: string = path.join('build-tests', project3PackageName);
const project4RelativePath: string = path.join('build-tests', project4PackageName);
const project5RelativePath: string = path.join('build-tests', project5PackageName);
const project1Path: string = path.join(repoRoot, project1RelativePath);
const project2Path: string = path.resolve(repoRoot, project2RelativePath);
const project3Path: string = path.resolve(repoRoot, project3RelativePath);
const project4Path: string = path.resolve(repoRoot, project4RelativePath);
const project5Path: string = path.resolve(repoRoot, project5RelativePath);

function getDefaultProjectConfigurations(): IExtractorProjectConfiguration[] {
return [
Expand Down Expand Up @@ -619,4 +622,13 @@ describe(PackageExtractor.name, () => {
Sort.sortBy(metadata.projects, (x) => x.path);
expect(metadata).toMatchSnapshot();
});

it('should normalize and remove duplicate file paths', async () => {
await FileSystem.writeFileAsync(path.join(project5Path, 'dist', 'index.js'), '', {
ensureFolderExists: true
});
const result = await PackageExtractor.getPackageIncludedFilesAsync(project5Path);
// To make the test work on both Windows and *nix, need to normalize the paths to posix style
expect(result.map(path.posix.normalize)).toEqual(['dist/index.js', 'package.json']);
});
});
6 changes: 6 additions & 0 deletions rush.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -690,6 +690,12 @@
"reviewCategory": "tests",
"shouldPublish": false
},
{
"packageName": "package-extractor-test-05",
"projectFolder": "build-tests/package-extractor-test-05",
"reviewCategory": "tests",
"shouldPublish": false
},
{
"packageName": "rush-mcp-example-plugin",
"projectFolder": "build-tests/rush-mcp-example-plugin",
Expand Down