Skip to content

Commit

Permalink
chore: fail early on impossible shrinkwrap (aws#13899)
Browse files Browse the repository at this point in the history
If the **resolved** version of a package (the one we force using Yarn)
does not match the **required** version of a package (the one in
`package.json`), then NPM ignores the resolved version and installs
the package from the `requires` list anyway, *even* if it's processing
a shrinkwrap file.

Unfortunately, sometimes we have to force this situation, such as with
the `netmask` issue, where the required version is declared at `^1.0.6`
but we need to force it to be resolved to `2.0.1`.

Detect this situation early and bail out, so that we don't try to ship something
that doesn't have the version resolution we expected to ship.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored and hollanddd committed Aug 26, 2021
1 parent 043326a commit 38bcccb
Show file tree
Hide file tree
Showing 5 changed files with 128 additions and 27 deletions.
2 changes: 1 addition & 1 deletion tools/yarn-cling/lib/hoisting.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { PackageLockPackage } from "./types";
import { PackageLockPackage } from './types';

/**
* Hoist package-lock dependencies in-place
Expand Down
80 changes: 79 additions & 1 deletion tools/yarn-cling/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { promises as fs, exists } from 'fs';
import * as path from 'path';
import * as lockfile from '@yarnpkg/lockfile';
import * as semver from 'semver';
import { hoistDependencies } from './hoisting';
import { PackageJson, PackageLock, PackageLockEntry, PackageLockPackage, YarnLock } from './types';

Expand Down Expand Up @@ -49,13 +50,17 @@ export async function generateShrinkwrap(options: ShrinkwrapOptions): Promise<Pa
}

async function generateLockFile(pkgJson: PackageJson, yarnLock: YarnLock, rootDir: string): Promise<PackageLock> {
return {
const lockFile = {
name: pkgJson.name,
version: pkgJson.version,
lockfileVersion: 1,
requires: true,
dependencies: await dependenciesFor(pkgJson.dependencies || {}, yarnLock, rootDir),
};

checkRequiredVersions(lockFile);

return lockFile;
}

// eslint-disable-next-line max-len
Expand Down Expand Up @@ -186,4 +191,77 @@ async function findPackageDir(depName: string, rootDir: string) {
}

throw new Error(`Did not find '${depName}' upwards of '${rootDir}'`);
}

/**
* We may sometimes try to adjust a package version to a version that's incompatible with the declared requirement.
*
* For example, this recently happened for 'netmask', where the package we
* depend on has `{ requires: { netmask: '^1.0.6', } }`, but we need to force-substitute in version `2.0.1`.
*
* If NPM processes the shrinkwrap and encounters the following situation:
*
* ```
* {
* netmask: { version: '2.0.1' },
* resolver: {
* requires: {
* netmask: '^1.0.6'
* }
* }
* }
* ```
*
* NPM is going to disregard the swhinkrwap and still give `resolver` its own private
* copy of netmask `^1.0.6`.
*
* We tried overriding the `requires` version, and that works for `npm install` (yay)
* but if anyone runs `npm ls` afterwards, `npm ls` is going to check the actual source
* `package.jsons` against the actual `node_modules` file tree, and complain that the
* versions don't match.
*
* We run `npm ls` in our tests to make sure our dependency tree is sane, and our customers
* might too, so this is not a great solution.
*
* To cut any discussion short in the future, we're going to detect this situation and
* tell our future selves that is cannot and will not work, and we should find another
* solution.
*/
export function checkRequiredVersions(root: PackageLock | PackageLockPackage) {
recurse(root, []);

function recurse(entry: PackageLock | PackageLockPackage, parentChain: PackageLockEntry[]) {
// On the root, 'requires' is the value 'true', for God knows what reason. Don't care about those.
if (typeof entry.requires === 'object') {

// For every 'requires' dependency, find the version it actually got resolved to and compare.
for (const [name, range] of Object.entries(entry.requires)) {
const resolvedPackage = findResolved(name, [entry, ...parentChain]);
if (!resolvedPackage) { continue; }

if (!semver.satisfies(resolvedPackage.version, range)) {
// Ruh-roh.
throw new Error(`Looks like we're trying to force '${name}' to version '${resolvedPackage.version}', but the dependency `
+ `is specified as '${range}'. This can never properly work via shrinkwrapping. Try vendoring a patched `
+ 'version of the intermediary dependencies instead.');
}
}
}

for (const dep of Object.values(entry.dependencies ?? {})) {
recurse(dep, [entry, ...parentChain]);
}
}

/**
* Find a package name in a package lock tree.
*/
function findResolved(name: string, chain: PackageLockEntry[]) {
for (const level of chain) {
if (level.dependencies?.[name]) {
return level.dependencies?.[name];
}
}
return undefined;
}
}
4 changes: 2 additions & 2 deletions tools/yarn-cling/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ export interface ResolvedYarnPackage {

export interface PackageLock extends PackageLockEntry {
name: string;
lockfileVersion: 1;
requires: true;
lockfileVersion: number;
requires: boolean;
}

export interface PackageLockEntry {
Expand Down
3 changes: 2 additions & 1 deletion tools/yarn-cling/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@
"typescript": "~3.9.9"
},
"dependencies": {
"@yarnpkg/lockfile": "^1.1.0"
"@yarnpkg/lockfile": "^1.1.0",
"semver": "^7.3.5"
},
"keywords": [
"aws",
Expand Down
66 changes: 44 additions & 22 deletions tools/yarn-cling/test/cling.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as path from 'path';
import { generateShrinkwrap } from "../lib";
import { checkRequiredVersions, generateShrinkwrap } from '../lib';

test('generate lock for fixture directory', async () => {
const lockFile = await generateShrinkwrap({
Expand All @@ -9,27 +9,27 @@ test('generate lock for fixture directory', async () => {

expect(lockFile).toEqual({
lockfileVersion: 1,
name: "package1",
name: 'package1',
requires: true,
version: "1.1.1",
version: '1.1.1',
dependencies: {
package2: {
version: "2.2.2",
version: '2.2.2',
},
registrydependency1: {
dependencies: {
registrydependency2: {
integrity: "sha512-pineapple",
resolved: "https://registry.bla.com/stuff",
version: "2.3.999",
integrity: 'sha512-pineapple',
resolved: 'https://registry.bla.com/stuff',
version: '2.3.999',
},
},
integrity: "sha512-banana",
integrity: 'sha512-banana',
requires: {
registrydependency2: "^2.3.4",
registrydependency2: '^2.3.4',
},
resolved: "https://registry.bla.com/stuff",
version: "1.2.999",
resolved: 'https://registry.bla.com/stuff',
version: '1.2.999',
},
},
});
Expand All @@ -43,26 +43,48 @@ test('generate hoisted lock for fixture directory', async () => {

expect(lockFile).toEqual({
lockfileVersion: 1,
name: "package1",
name: 'package1',
requires: true,
version: "1.1.1",
version: '1.1.1',
dependencies: {
package2: {
version: "2.2.2",
version: '2.2.2',
},
registrydependency1: {
integrity: "sha512-banana",
integrity: 'sha512-banana',
requires: {
registrydependency2: "^2.3.4",
registrydependency2: '^2.3.4',
},
resolved: "https://registry.bla.com/stuff",
version: "1.2.999",
resolved: 'https://registry.bla.com/stuff',
version: '1.2.999',
},
registrydependency2: {
integrity: "sha512-pineapple",
resolved: "https://registry.bla.com/stuff",
version: "2.3.999",
integrity: 'sha512-pineapple',
resolved: 'https://registry.bla.com/stuff',
version: '2.3.999',
},
},
});
});
});

test('fail when requires cannot be satisfied', async () => {
const lockFile = {
lockfileVersion: 1,
name: 'package1',
requires: true,
version: '1.1.1',
dependencies: {
package1: {
version: '2.2.2',
requires: {
package2: '^3.3.3', // <- this needs to be adjusted
},
},
package2: {
version: '4.4.4',
},
},
};

expect(() => checkRequiredVersions(lockFile)).toThrow(/This can never/);
});

0 comments on commit 38bcccb

Please sign in to comment.