Skip to content

Commit

Permalink
chore(yarn-cling): breaks on dependency cycle (#18188)
Browse files Browse the repository at this point in the history
In a recent upgrade, one of our dependencies (`json-diff`) has taken
a dependency on a cluster of packages that have dependency cycles
between them. Specifically:

```
json-diff => cli-color => [ d => es5-ext => es6-iterator => d ].
json-diff => cli-color => d => [ es5-ext => es6-iterator => es5-ext ].
json-diff => cli-color => [ d => es5-ext => es6-iterator => es6-symbol => d ].
json-diff => cli-color => [ d => es5-ext => es6-symbol => d ].
json-diff => cli-color => [ es5-ext => es6-iterator => d => es5-ext ].
json-diff => cli-color => [ es5-ext => es6-iterator => es6-symbol => d => es5-ext ].
json-diff => cli-color => [ es5-ext => es6-symbol => d => es5-ext ].
json-diff => cli-color => [ es6-iterator => d => es5-ext => es6-iterator ].
json-diff => cli-color => [ es6-iterator => es5-ext => es6-iterator ].
json-diff => cli-color => [ es6-iterator => es6-symbol => d => es5-ext => es6-iterator ].
json-diff => cli-color => es6-iterator => [ es6-symbol => d => es5-ext => es6-symbol ].
json-diff => cli-color => memoizee => es6-weak-map => [ es6-symbol => d => es5-ext => es6-iterator => es6-symbol ].
```

`yarn-cling` used to go into infinite recursion trying to resolve this
dependency tree, as it was not prepared to handle cycles.

Add a dependency breaker. Since I wasn't sure whether or not this
might break the dependency tree, add a validation step as well.


----

*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 Dec 27, 2021
1 parent be7acfd commit 63330ad
Showing 1 changed file with 63 additions and 4 deletions.
67 changes: 63 additions & 4 deletions tools/@aws-cdk/yarn-cling/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ export async function generateShrinkwrap(options: ShrinkwrapOptions): Promise<Pa
hoistDependencies({ version: '*', dependencies: lock.dependencies });
}

validateTree(lock);

if (options.outputFile) {
// Write the shrinkwrap file
await fs.writeFile(options.outputFile, JSON.stringify(lock, undefined, 2), { encoding: 'utf8' });
Expand All @@ -55,22 +57,37 @@ async function generateLockFile(pkgJson: PackageJson, yarnLock: YarnLock, rootDi
version: pkgJson.version,
lockfileVersion: 1,
requires: true,
dependencies: await dependenciesFor(pkgJson.dependencies || {}, yarnLock, rootDir),
dependencies: await dependenciesFor(pkgJson.dependencies || {}, yarnLock, rootDir, [pkgJson.name]),
};

checkRequiredVersions(lockFile);

return lockFile;
}

const CYCLES_REPORTED = new Set<string>();

// eslint-disable-next-line max-len
async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock, rootDir: string): Promise<Record<string, PackageLockPackage>> {
async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock, rootDir: string, dependencyPath: string[]): Promise<Record<string, PackageLockPackage>> {
const ret: Record<string, PackageLockPackage> = {};

// Get rid of any monorepo symlinks
rootDir = await fs.realpath(rootDir);

for (const [depName, versionRange] of Object.entries(deps)) {
if (dependencyPath.includes(depName)) {
const index = dependencyPath.indexOf(depName);
const beforeCycle = dependencyPath.slice(0, index);
const inCycle = [...dependencyPath.slice(index), depName];
const cycleString = inCycle.join(' => ');
if (!CYCLES_REPORTED.has(cycleString)) {
// eslint-disable-next-line no-console
console.warn(`Dependency cycle: ${beforeCycle.join(' => ')} => [ ${cycleString} ]. Dropping dependency '${inCycle.slice(-2).join(' => ')}'.`);
CYCLES_REPORTED.add(cycleString);
}
continue;
}

const depDir = await findPackageDir(depName, rootDir);
const depPkgJsonFile = path.join(depDir, 'package.json');
const depPkgJson = await loadPackageJson(depPkgJsonFile);
Expand All @@ -89,14 +106,14 @@ async function dependenciesFor(deps: Record<string, string>, yarnLock: YarnLock,
integrity: yarnResolved.integrity,
resolved: yarnResolved.resolved,
requires: depPkgJson.dependencies,
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir),
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir, [...dependencyPath, depName]),
};
} else {
// Comes from monorepo, just use whatever's in package.json
ret[depName] = {
version: depPkgJson.version,
requires: depPkgJson.dependencies,
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir),
dependencies: await dependenciesFor(depPkgJson.dependencies || {}, yarnLock, depDir, [...dependencyPath, depName]),
};
}

Expand Down Expand Up @@ -264,4 +281,46 @@ export function checkRequiredVersions(root: PackageLock | PackageLockPackage) {
}
return undefined;
}
}

/**
* Check that all packages still resolve their dependencies to the right versions
*
* We have manipulated the tree a bunch. Do a sanity check to ensure that all declared
* dependencies are satisfied.
*/
function validateTree(lock: PackageLock) {
let failed = false;
recurse(lock, [lock]);
if (failed) {
throw new Error('Could not satisfy one or more dependencies');
}

function recurse(pkg: PackageLockEntry, rootPath: PackageLockEntry[]) {
for (const pack of Object.values(pkg.dependencies ?? {})) {
const p = [pack, ...rootPath];
checkRequiresOf(pack, p);
recurse(pack, p);
}
}

// rootPath: most specific one first
function checkRequiresOf(pack: PackageLockPackage, rootPath: PackageLockEntry[]) {
for (const [name, declaredRange] of Object.entries(pack.requires ?? {})) {
const foundVersion = rootPath.map((p) => p.dependencies?.[name]?.version).find(isDefined);
if (!foundVersion) {
// eslint-disable-next-line no-console
console.error(`Dependency on ${name} not satisfied: not found`);
failed = true;
} else if (!semver.satisfies(foundVersion, declaredRange)) {
// eslint-disable-next-line no-console
console.error(`Dependency on ${name} not satisfied: declared range '${declaredRange}', found '${foundVersion}'`);
failed = true;
}
}
}
}

function isDefined<A>(x: A): x is NonNullable<A> {
return x !== undefined;
}

0 comments on commit 63330ad

Please sign in to comment.