Skip to content

Commit a02bea0

Browse files
authored
fix: use lstatSync in generatePosixLink (nodejs#767)
Using `fs.promises.realpath` in `generatePosixLink` returns the fully resolved, absolute path of the symlink target. However, the symlink variable used in the comparison is a relative path calculated by `path.relative`. This results in a comparison between an absolute path and a relative path, causing the `currentSymlink !== symlink` check to always evaluate to true.
1 parent ac518c4 commit a02bea0

File tree

2 files changed

+55
-12
lines changed

2 files changed

+55
-12
lines changed

sources/commands/Enable.ts

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -81,20 +81,22 @@ export class EnableCommand extends Command<Context> {
8181
async generatePosixLink(installDirectory: string, distFolder: string, binName: string) {
8282
const file = path.join(installDirectory, binName);
8383
const symlink = path.relative(installDirectory, path.join(distFolder, `${binName}.js`));
84+
const stats = fs.lstatSync(file, {throwIfNoEntry: false});
8485

85-
if (fs.existsSync(file)) {
86-
const currentSymlink = await fs.promises.realpath(file);
86+
if (stats) {
87+
if (stats.isSymbolicLink()) {
88+
const currentSymlink = await fs.promises.readlink(file);
8789

88-
if (binName.includes(`yarn`) && corepackUtils.isYarnSwitchPath(currentSymlink)) {
89-
console.warn(`${binName} is already installed in ${file} and points to a Yarn Switch install - skipping`);
90-
return;
91-
}
90+
if (binName.includes(`yarn`) && corepackUtils.isYarnSwitchPath(await fs.promises.realpath(file))) {
91+
console.warn(`${binName} is already installed in ${file} and points to a Yarn Switch install - skipping`);
92+
return;
93+
}
9294

93-
if (currentSymlink !== symlink) {
94-
await fs.promises.unlink(file);
95-
} else {
96-
return;
95+
if (currentSymlink === symlink) {
96+
return;
97+
}
9798
}
99+
await fs.promises.unlink(file);
98100
}
99101

100102
await fs.promises.symlink(symlink, file);

tests/Enable.test.ts

Lines changed: 43 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {Filename, ppath, xfs, npath} from '@yarnpkg/fslib';
22
import {delimiter} from 'node:path';
33
import process from 'node:process';
4+
import {setTimeout} from 'node:timers/promises';
45
import {describe, beforeEach, it, expect, test} from 'vitest';
56

67
import {Engine} from '../sources/Engine';
@@ -92,7 +93,6 @@ describe(`EnableCommand`, () => {
9293
await xfs.mktempPromise(async cwd => {
9394
await xfs.writeFilePromise(ppath.join(cwd, `yarn`), `hello`);
9495

95-
process.env.PATH = `${npath.fromPortablePath(cwd)}${delimiter}${process.env.PATH}`;
9696
await expect(runCli(cwd, [`enable`, `--install-directory`, npath.fromPortablePath(cwd)])).resolves.toMatchObject({
9797
stdout: ``,
9898
stderr: ``,
@@ -114,7 +114,6 @@ describe(`EnableCommand`, () => {
114114
ppath.join(cwd, `yarn`),
115115
);
116116

117-
process.env.PATH = `${npath.fromPortablePath(cwd)}${delimiter}${process.env.PATH}`;
118117
await expect(runCli(cwd, [`enable`, `--install-directory`, npath.fromPortablePath(cwd)])).resolves.toMatchObject({
119118
stdout: ``,
120119
stderr: expect.stringMatching(/^yarn is already installed in .+ and points to a Yarn Switch install - skipping\n$/),
@@ -125,4 +124,46 @@ describe(`EnableCommand`, () => {
125124
expect(file).toBe(`hello`);
126125
});
127126
});
127+
128+
test.skipIf(process.platform === `win32`)(`should not re-link if binaries are already correct`, async () => {
129+
await xfs.mktempPromise(async cwd => {
130+
await makeBin(cwd, `corepack` as Filename);
131+
132+
await expect(runCli(cwd, [`enable`, `--install-directory`, npath.fromPortablePath(cwd)])).resolves.toMatchObject({
133+
stdout: ``,
134+
stderr: ``,
135+
exitCode: 0,
136+
});
137+
const yarnStat1 = await xfs.lstatPromise(ppath.join(cwd, `yarn`));
138+
139+
await setTimeout(10);
140+
141+
await expect(runCli(cwd, [`enable`, `--install-directory`, npath.fromPortablePath(cwd)])).resolves.toMatchObject({
142+
stdout: ``,
143+
stderr: ``,
144+
exitCode: 0,
145+
});
146+
const yarnStat2 = await xfs.lstatPromise(ppath.join(cwd, `yarn`));
147+
148+
expect(yarnStat2.mtimeMs).toBe(yarnStat1.mtimeMs);
149+
});
150+
});
151+
152+
test.skipIf(process.platform === `win32`)(`should overwrite existing symlinks if they are incorrect`, async () => {
153+
await xfs.mktempPromise(async cwd => {
154+
await makeBin(cwd, `corepack` as Filename);
155+
156+
await xfs.writeFilePromise(ppath.join(cwd, `dummy-target`), `hello`);
157+
await xfs.symlinkPromise(ppath.join(cwd, `dummy-target`), ppath.join(cwd, `yarn`));
158+
159+
await expect(runCli(cwd, [`enable`, `--install-directory`, npath.fromPortablePath(cwd)])).resolves.toMatchObject({
160+
stdout: ``,
161+
stderr: ``,
162+
exitCode: 0,
163+
});
164+
165+
const newLink = await xfs.readlinkPromise(ppath.join(cwd, `yarn`));
166+
expect(newLink).toContain(`yarn.js`);
167+
});
168+
});
128169
});

0 commit comments

Comments
 (0)