From c6d4534e71ba6575bc9f3c851527b10a04d008a9 Mon Sep 17 00:00:00 2001 From: Carlos Ravelo Date: Thu, 26 Oct 2017 10:35:35 -0400 Subject: [PATCH] fix(linker): Fix yarn removing linked deps during link stage (#4757) **Summary** Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating. Fixes #3288, fixes #4770, fixes #4635, fixes #4603. Potential fix for #3202. **Test plan** See https://github.com/yarnpkg/yarn/issues/3288#issuecomment-335955366 for repro steps. See https://github.com/yarnpkg/yarn/issues/3288#issuecomment-338503103 for my explanation of the problem. With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test. --- __tests__/commands/_helpers.js | 35 +++++--- __tests__/commands/install/integration.js | 76 ++++++++++++------ __tests__/commands/link.js | 14 +--- .../.npmrc | 1 - .../dir-to-link/package.json | 7 -- .../@fakescope-fake-dependency-1.0.1.tgz | Bin 576 -> 0 bytes .../package.json | 5 -- .../yarn.lock | 5 -- .../package.json | 8 ++ .../package-with-name-scoped/package.json | 3 + src/package-linker.js | 19 ++++- src/reporters/lang/en.js | 1 + 12 files changed, 106 insertions(+), 68 deletions(-) delete mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked-scoped/.npmrc delete mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked-scoped/dir-to-link/package.json delete mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked-scoped/mirror-for-offline/@fakescope-fake-dependency-1.0.1.tgz delete mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked-scoped/package.json delete mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked-scoped/yarn.lock create mode 100644 __tests__/fixtures/install/install-dont-overwrite-linked/package.json create mode 100644 __tests__/fixtures/link/package-with-name-scoped/package.json diff --git a/__tests__/commands/_helpers.js b/__tests__/commands/_helpers.js index b73f39ed1c..d8fd782b8f 100644 --- a/__tests__/commands/_helpers.js +++ b/__tests__/commands/_helpers.js @@ -10,16 +10,17 @@ import * as fs from '../../src/util/fs.js'; import {Install} from '../../src/cli/commands/install.js'; import Config from '../../src/config.js'; import parsePackagePath from '../../src/util/parse-package-path.js'; - +import type {CLIFunctionReturn} from '../../src/types.js'; +import {run as link} from '../../src/cli/commands/link.js'; const stream = require('stream'); const path = require('path'); -const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'install'); +const installFixturesLoc = path.join(__dirname, '..', 'fixtures', 'install'); export const runInstall = run.bind( null, ConsoleReporter, - fixturesLoc, + installFixturesLoc, async (args, flags, config, reporter, lockfile): Promise => { const install = new Install(flags, config, reporter, lockfile); await install.init(); @@ -30,6 +31,17 @@ export const runInstall = run.bind( [], ); +const linkFixturesLoc = path.join(__dirname, '..', 'fixtures', 'link'); + +export const runLink = run.bind( + null, + ConsoleReporter, + linkFixturesLoc, + (args, flags, config, reporter): CLIFunctionReturn => { + return link(config, reporter, flags, args); + }, +); + export async function createLockfile(dir: string): Promise { const lockfileLoc = path.join(dir, constants.LOCKFILE_FILENAME); let lockfile; @@ -94,7 +106,7 @@ export async function run( ) => Promise | T, args: Array, flags: Object, - name: string | {source: string, cwd: string}, + name: string | {source?: string, cwd: string}, checkInstalled: ?(config: Config, reporter: R, install: T, getStdout: () => string) => ?Promise, beforeInstall: ?(cwd: string) => ?Promise, ): Promise { @@ -113,11 +125,16 @@ export async function run( if (fixturesLoc) { const source = typeof name === 'string' ? name : name.source; - const dir = path.join(fixturesLoc, source); - cwd = await fs.makeTempDir(path.basename(dir)); - await fs.copy(dir, cwd, reporter); - if (typeof name !== 'string') { - cwd = path.join(cwd, name.cwd); + // if source wasn't set then assume we were given a complete path + if (typeof source === 'undefined') { + cwd = typeof name !== 'string' ? name.cwd : await fs.makeTempDir(); + } else { + const dir = path.join(fixturesLoc, source); + cwd = await fs.makeTempDir(path.basename(dir)); + await fs.copy(dir, cwd, reporter); + if (typeof name !== 'string') { + cwd = path.join(cwd, name.cwd); + } } } else { // if fixture loc is not set then CWD is some empty temp dir diff --git a/__tests__/commands/install/integration.js b/__tests__/commands/install/integration.js index 695e1eb91a..c8523bf2f5 100644 --- a/__tests__/commands/install/integration.js +++ b/__tests__/commands/install/integration.js @@ -11,7 +11,7 @@ import {parse} from '../../../src/lockfile'; import {Install, run as install} from '../../../src/cli/commands/install.js'; import Lockfile from '../../../src/lockfile'; import * as fs from '../../../src/util/fs.js'; -import {getPackageVersion, explodeLockfile, runInstall, createLockfile, run as buildRun} from '../_helpers.js'; +import {getPackageVersion, explodeLockfile, runInstall, runLink, createLockfile, run as buildRun} from '../_helpers.js'; jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000; @@ -866,29 +866,6 @@ test('install a scoped module from authed private registry with a missing traili }); }); -test.concurrent('install will not overwrite files in symlinked scoped directories', async (): Promise => { - await runInstall( - {}, - 'install-dont-overwrite-linked-scoped', - async (config): Promise => { - const dependencyPath = path.join(config.cwd, 'node_modules', '@fakescope', 'fake-dependency'); - expect('Symlinked scoped package test').toEqual( - (await fs.readJson(path.join(dependencyPath, 'package.json'))).description, - ); - expect(await fs.exists(path.join(dependencyPath, 'index.js'))).toEqual(false); - }, - async cwd => { - const dirToLink = path.join(cwd, 'dir-to-link'); - - await fs.mkdirp(path.join(cwd, '.yarn-link', '@fakescope')); - await fs.symlink(dirToLink, path.join(cwd, '.yarn-link', '@fakescope', 'fake-dependency')); - - await fs.mkdirp(path.join(cwd, 'node_modules', '@fakescope')); - await fs.symlink(dirToLink, path.join(cwd, 'node_modules', '@fakescope', 'fake-dependency')); - }, - ); -}); - test.concurrent('install of scoped package with subdependency conflict should pass check', (): Promise => { return runInstall({}, 'install-scoped-package-with-subdependency-conflict', async (config, reporter) => { let allCorrect = true; @@ -1134,3 +1111,54 @@ test.concurrent('warns for missing bundledDependencies', (): Promise => { 'missing-bundled-dep', ); }); + +test.concurrent('install will not overwrite linked scoped dependencies', async (): Promise => { + // install only dependencies + await runInstall({production: true}, 'install-dont-overwrite-linked', async (installConfig): Promise => { + // link our fake dep to the registry + await runLink([], {}, 'package-with-name-scoped', async (linkConfig): Promise => { + // link our fake dependency in our node_modules + await runLink( + ['@fakescope/a-package'], + {linkFolder: linkConfig.linkFolder}, + {cwd: installConfig.cwd}, + async (): Promise => { + // check that it exists (just in case) + const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', '@fakescope', 'a-package')); + expect(existed).toEqual(true); + + // run install to install dev deps which would remove the linked dep if the bug was present + await runInstall({linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise => { + // if the linked dep is still there is a win :) + const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', '@fakescope', 'a-package')); + expect(existed).toEqual(true); + }); + }, + ); + }); + }); +}); + +test.concurrent('install will not overwrite linked dependencies', async (): Promise => { + // install only dependencies + await runInstall({production: true}, 'install-dont-overwrite-linked', async (installConfig): Promise => { + // link our fake dep to the registry + await runLink([], {}, 'package-with-name', async (linkConfig): Promise => { + // link our fake dependency in our node_modules + await runLink(['a-package'], {linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise< + void, + > => { + // check that it exists (just in case) + const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', 'a-package')); + expect(existed).toEqual(true); + + // run install to install dev deps which would remove the linked dep if the bug was present + await runInstall({linkFolder: linkConfig.linkFolder}, {cwd: installConfig.cwd}, async (): Promise => { + // if the linked dep is still there is a win :) + const existed = await fs.exists(path.join(installConfig.cwd, 'node_modules', 'a-package')); + expect(existed).toEqual(true); + }); + }); + }); + }); +}); diff --git a/__tests__/commands/link.js b/__tests__/commands/link.js index 59fd7db353..a3c1129615 100644 --- a/__tests__/commands/link.js +++ b/__tests__/commands/link.js @@ -1,24 +1,12 @@ /* @flow */ -import {run as buildRun} from './_helpers.js'; -import {run as link} from '../../src/cli/commands/link.js'; +import {runLink} from './_helpers.js'; import {ConsoleReporter} from '../../src/reporters/index.js'; -import type {CLIFunctionReturn} from '../../src/types.js'; import mkdir from './../_temp.js'; import * as fs from '../../src/util/fs.js'; const path = require('path'); -const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'link'); -const runLink = buildRun.bind( - null, - ConsoleReporter, - fixturesLoc, - (args, flags, config, reporter): CLIFunctionReturn => { - return link(config, reporter, flags, args); - }, -); - test.concurrent('creates folder in linkFolder', async (): Promise => { const linkFolder = await mkdir('link-folder'); await runLink([], {linkFolder}, 'package-with-name', async (config, reporter): Promise => { diff --git a/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/.npmrc b/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/.npmrc deleted file mode 100644 index 9465b97ac3..0000000000 --- a/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/.npmrc +++ /dev/null @@ -1 +0,0 @@ -yarn-offline-mirror=./mirror-for-offline diff --git a/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/dir-to-link/package.json b/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/dir-to-link/package.json deleted file mode 100644 index 5de00bef5d..0000000000 --- a/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/dir-to-link/package.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "name": "@fakescope/fake-dependency", - "description": "Symlinked scoped package test", - "version": "1.0.1", - "dependencies": {}, - "license": "MIT" -} diff --git a/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/mirror-for-offline/@fakescope-fake-dependency-1.0.1.tgz b/__tests__/fixtures/install/install-dont-overwrite-linked-scoped/mirror-for-offline/@fakescope-fake-dependency-1.0.1.tgz deleted file mode 100644 index 29e48dd0cd7719c87d0a4387dfaa3512de5a828b..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 576 zcmV-G0>AwqiwFSLef?Je1MQW~Zks?5fOF<4CLH3#N-YF@x%;O2woOg(WP1$xfTi=Px`r@9|asB^0LqDNk_Y+?U6= zL+2wLjh?T6SfTPfJuwlprKp4Ml=*%|bvqRDStm01ralREEC%(AR;8(aIa_4D=b=HE#D1rYpq z_t=l-f9M~4|D&Ov|G@QW{{IM8-DNgsIoelXU(eYJ%UBdRs8_ENEKvV3@OW!q)^>@0 zDAS^D(%Ss(65|!5^MeW2Em)x$-=;-d$2H2*IC+lic&&DY+s{HKrB+XEGYIx2*jNU& zXvW{3nVU@{Dc0t@eWT6N7