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

Implements content addressing inside copyPromise #4586

Merged
merged 3 commits into from
Jul 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 37 additions & 0 deletions .yarn/versions/bf825c43.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
releases:
"@yarnpkg/cli": major
"@yarnpkg/fslib": minor
"@yarnpkg/plugin-pnpm": major

declined:
- "@yarnpkg/plugin-compat"
- "@yarnpkg/plugin-constraints"
- "@yarnpkg/plugin-dlx"
- "@yarnpkg/plugin-essentials"
- "@yarnpkg/plugin-exec"
- "@yarnpkg/plugin-file"
- "@yarnpkg/plugin-git"
- "@yarnpkg/plugin-github"
- "@yarnpkg/plugin-init"
- "@yarnpkg/plugin-interactive-tools"
- "@yarnpkg/plugin-link"
- "@yarnpkg/plugin-nm"
- "@yarnpkg/plugin-npm"
- "@yarnpkg/plugin-npm-cli"
- "@yarnpkg/plugin-pack"
- "@yarnpkg/plugin-patch"
- "@yarnpkg/plugin-pnp"
- "@yarnpkg/plugin-stage"
- "@yarnpkg/plugin-typescript"
- "@yarnpkg/plugin-version"
- "@yarnpkg/plugin-workspace-tools"
- vscode-zipfs
- "@yarnpkg/builder"
- "@yarnpkg/core"
- "@yarnpkg/doctor"
- "@yarnpkg/json-proxy"
- "@yarnpkg/nm"
- "@yarnpkg/pnp"
- "@yarnpkg/pnpify"
- "@yarnpkg/sdks"
- "@yarnpkg/shell"
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
import {Filename, PortablePath, ppath, xfs} from '@yarnpkg/fslib';

describe(`Features`, () => {
describe(`Content-Addressed Index`, () => {
test(
`it should use the exact same device/inode for the same file from the same package`,
makeTemporaryEnv({
dependencies: {
[`no-deps`]: `1.0.0`,
},
}, {
nodeLinker: `pnpm`,
}, async ({path, run, source}) => {
await xfs.mktempPromise(async path2 => {
await xfs.writeJsonPromise(ppath.join(path2, Filename.manifest), {
name: `my-package`,
dependencies: {
[`no-deps`]: `1.0.0`,
},
});

await run(`install`, {cwd: path});
await run(`install`, {cwd: path2});

const statA = await xfs.statPromise(ppath.join(path, `node_modules/no-deps/package.json` as PortablePath));
const statB = await xfs.statPromise(ppath.join(path, `node_modules/no-deps/package.json` as PortablePath));

expect({
dev: statA.dev,
ino: statA.ino,
}).toEqual({
dev: statB.dev,
ino: statB.ino,
});
});
}),
);

test(
`it should use the exact same device/inode for the same file from different packages`,
makeTemporaryEnv({
dependencies: {
[`no-deps`]: `1.0.0`,
},
}, {
nodeLinker: `pnpm`,
}, async ({path, run, source}) => {
await xfs.mktempPromise(async path2 => {
await xfs.writeJsonPromise(ppath.join(path2, Filename.manifest), {
name: `my-package`,
dependencies: {
[`no-deps`]: `2.0.0`,
},
});

await run(`install`, {cwd: path});
await run(`install`, {cwd: path2});

const statA = await xfs.statPromise(ppath.join(path, `node_modules/no-deps/index.js` as PortablePath));
const statB = await xfs.statPromise(ppath.join(path, `node_modules/no-deps/index.js` as PortablePath));

expect({
dev: statA.dev,
ino: statA.ino,
}).toEqual({
dev: statB.dev,
ino: statB.ino,
});
});
}),
);

test(
`it should detect when an index file was modified, and automatically repair it`,
makeTemporaryEnv({
dependencies: {
[`no-deps`]: `1.0.0`,
},
}, {
nodeLinker: `pnpm`,
}, async ({path, run, source}) => {
await xfs.mktempPromise(async path2 => {
await run(`install`);

const referenceFile = ppath.join(path, `node_modules/no-deps/index.js` as PortablePath);

const originalContent = await xfs.readFilePromise(referenceFile, `utf8`);
const newContent = `${originalContent}// oh no, modified\n`;

await xfs.writeFilePromise(referenceFile, newContent);

await run(`install`);

await expect(xfs.readFilePromise(referenceFile, `utf8`)).resolves.toEqual(originalContent);
});
}),
);

test(
`it should repair the index across all projects, not only the current one`,
makeTemporaryEnv({
dependencies: {
[`no-deps`]: `1.0.0`,
},
}, {
nodeLinker: `pnpm`,
}, async ({path, run, source}) => {
await xfs.mktempPromise(async path2 => {
await xfs.writeJsonPromise(ppath.join(path2, Filename.manifest), {
name: `my-package`,
dependencies: {
[`no-deps`]: `2.0.0`,
},
});

await run(`install`, {cwd: path});
await run(`install`, {cwd: path2});

const referenceFileA = ppath.join(path, `node_modules/no-deps/index.js` as PortablePath);
const referenceFileB = ppath.join(path, `node_modules/no-deps/index.js` as PortablePath);

const originalContent = await xfs.readFilePromise(referenceFileA, `utf8`);
const newContent = `${originalContent}// oh no, modified\n`;

await xfs.writeFilePromise(referenceFileA, newContent);

await run(`install`, {cwd: path});

await expect(xfs.readFilePromise(referenceFileA, `utf8`)).resolves.toEqual(originalContent);
await expect(xfs.readFilePromise(referenceFileB, `utf8`)).resolves.toEqual(originalContent);
});
}),
);
});
});
12 changes: 10 additions & 2 deletions packages/plugin-pnpm/sources/PnpmLinker.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Descriptor, FetchResult, formatUtils, Installer, InstallPackageExtraApi, Linker, LinkOptions, LinkType, Locator, LocatorHash, Manifest, MessageName, MinimalLinkOptions, Package, Project, miscUtils, structUtils} from '@yarnpkg/core';
import {Dirent, Filename, PortablePath, ppath, xfs} from '@yarnpkg/fslib';
import {Dirent, Filename, PortablePath, setupCopyIndex, ppath, xfs} from '@yarnpkg/fslib';
import {jsInstallUtils} from '@yarnpkg/plugin-pnp';
import {UsageError} from 'clipanion';

Expand Down Expand Up @@ -79,9 +79,12 @@ export class PnpmLinker implements Linker {

class PnpmInstaller implements Installer {
private readonly asyncActions = new miscUtils.AsyncActions(10);
private readonly indexFolderPromise: Promise<PortablePath>;

constructor(private opts: LinkOptions) {
// Nothing to do
this.indexFolderPromise = setupCopyIndex(xfs, {
indexPath: ppath.join(opts.project.configuration.get(`globalFolder`), `index` as Filename),
});
Comment on lines +85 to +87
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that the constructor has side-effects, can't this happen later when it's actually needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did it early to avoid having to either do IO calls for each package, or having to keep a boolean state (which I feel like would add complexity to avoid a situation - a pnpm install on an empty project without any other install - that doesn't happen much, and doesn't cost much even if it happens).

Copy link
Member

Choose a reason for hiding this comment

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

I would rather have that boolean state, this will do 257 mkdir calls every time PnpmInstaller is constructed, regardless of which linker is in use and if the directories already exists.

}

private customData: PnpmCustomData = {
Expand Down Expand Up @@ -127,6 +130,11 @@ class PnpmInstaller implements Installer {
await xfs.copyPromise(pkgPath, fetchResult.prefixPath, {
baseFs: fetchResult.packageFs,
overwrite: false,
linkStrategy: {
type: `HardlinkFromIndex`,
indexPath: await this.indexFolderPromise,
autoRepair: true,
},
});
}));

Expand Down
26 changes: 23 additions & 3 deletions packages/yarnpkg-fslib/sources/FakeFS.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {createHash} from 'crypto';
import {EventEmitter} from 'events';
import {Dirent as NodeDirent, ReadStream} from 'fs';
import {Stats as NodeStats, WriteStream} from 'fs';
Expand Down Expand Up @@ -271,6 +272,25 @@ export abstract class FakeFS<P extends Path> {
}
}

async checksumFilePromise(path: P, {algorithm = `sha512`}: {algorithm?: string} = {}) {
const fd = await this.openPromise(path, `r`);

try {
const CHUNK_SIZE = 65536;
const chunk = Buffer.allocUnsafeSlow(CHUNK_SIZE);

const hash = createHash(algorithm);

let bytesRead = 0;
while ((bytesRead = await this.readPromise(fd, chunk, 0, CHUNK_SIZE)) !== 0)
hash.update(bytesRead === CHUNK_SIZE ? chunk : chunk.slice(0, bytesRead));

return hash.digest(`hex`);
} finally {
await this.closePromise(fd);
}
}
Comment on lines +275 to +292
Copy link
Member

Choose a reason for hiding this comment

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

Why is this duplicated in here instead of using the one in hashUtils?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because otherwise fslib would depend on the core, which isn't necessary. The best would be to change the core to use this new FakeFS function rather than the hashUtils one, but I wanted to avoid touching the core in this PR 🤔


async removePromise(p: P, {recursive = true, maxRetries = 5}: {recursive?: boolean, maxRetries?: number} = {}) {
let stat;
try {
Expand Down Expand Up @@ -413,9 +433,9 @@ export abstract class FakeFS<P extends Path> {
return createdDirectory;
}

copyPromise(destination: P, source: P, options?: {baseFs?: undefined, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy}): Promise<void>;
copyPromise<P2 extends Path>(destination: P, source: P2, options: {baseFs: FakeFS<P2>, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy}): Promise<void>;
async copyPromise<P2 extends Path>(destination: P, source: P2, {baseFs = this as any, overwrite = true, stableSort = false, stableTime = false, linkStrategy = null}: {baseFs?: FakeFS<P2>, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy | null} = {}) {
copyPromise(destination: P, source: P, options?: {baseFs?: undefined, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy<P> | null}): Promise<void>;
copyPromise<P2 extends Path>(destination: P, source: P2, options: {baseFs: FakeFS<P2>, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy<P> | null}): Promise<void>;
async copyPromise<P2 extends Path>(destination: P, source: P2, {baseFs = this as any, overwrite = true, stableSort = false, stableTime = false, linkStrategy = null}: {baseFs?: FakeFS<P2>, overwrite?: boolean, stableSort?: boolean, stableTime?: boolean, linkStrategy?: LinkStrategy<P> | null} = {}) {
return await copyPromise(this, destination, baseFs, source, {overwrite, stableSort, stableTime, linkStrategy});
}

Expand Down
Loading