From 433d1f870e23d9881c847d79834ebd27cd640061 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 7 Nov 2019 11:47:25 +0100 Subject: [PATCH] fix(pacmak): .NET build downloading packages from NuGet (#949) Fix situation where the .NET build accidentally downloads dependency packages from NuGet, instead of using the locally built packages. Caused by packages being built in the order given on the command line, and if a consumer is built before a dependency, the NuGet build would fall back to loading the package from the online repository. Apply a topological sort to package building to prevent this from happening. --- packages/jsii-pacmak/lib/npm-modules.ts | 20 ++++-- packages/jsii-pacmak/lib/packaging.ts | 7 ++ packages/jsii-pacmak/lib/toposort.ts | 48 ++++++++++++++ packages/jsii-pacmak/package.json | 2 + packages/jsii-pacmak/test/npm-modules.test.ts | 65 +++++++++++++++++++ yarn.lock | 12 ++++ 6 files changed, 148 insertions(+), 6 deletions(-) create mode 100644 packages/jsii-pacmak/lib/toposort.ts create mode 100644 packages/jsii-pacmak/test/npm-modules.test.ts diff --git a/packages/jsii-pacmak/lib/npm-modules.ts b/packages/jsii-pacmak/lib/npm-modules.ts index f42e3e668f..705a94d136 100644 --- a/packages/jsii-pacmak/lib/npm-modules.ts +++ b/packages/jsii-pacmak/lib/npm-modules.ts @@ -5,13 +5,15 @@ import { resolveDependencyDirectory } from './util'; import logging = require('../lib/logging'); import { JsiiModule } from './packaging'; +import { topologicalSort } from './toposort'; /** * Find all modules that need to be packagerd * - * If the input list is empty, include the current directory. The result - * is NOT topologically sorted. + * If the input list is empty, include the current directory. + * + * The result is topologically sorted. */ export async function findJsiiModules(directories: string[], recurse: boolean) { const ret: JsiiModule[] = []; @@ -19,7 +21,7 @@ export async function findJsiiModules(directories: string[], recurse: boolean) { for (const dir of directories.length > 0 ? directories : ['.']) { await visitPackage(dir, true); } - return ret; + return topologicalSort(ret, m => m.name, m => m.dependencyNames); async function visitPackage(dir: string, isRoot: boolean) { const realPath = await fs.realpath(dir); @@ -35,9 +37,15 @@ export async function findJsiiModules(directories: string[], recurse: boolean) { } } + if (!pkg.name) { + throw new Error(`package.json does not have a 'name' field: ${JSON.stringify(pkg, undefined, 2)}`); + } + + const dependencyNames = Object.keys(pkg.dependencies || {}); + // if --recurse is set, find dependency dirs and build them. if (recurse) { - for (const dep of Object.keys(pkg.dependencies || {})) { + for (const dep of dependencyNames) { const depDir = resolveDependencyDirectory(realPath, dep); await visitPackage(depDir, false); } @@ -51,10 +59,10 @@ export async function findJsiiModules(directories: string[], recurse: boolean) { name: pkg.name, moduleDirectory: realPath, defaultOutputDirectory: outputDirectory, - availableTargets: targets + availableTargets: targets, + dependencyNames })); } - } export async function updateAllNpmIgnores(packages: JsiiModule[]) { diff --git a/packages/jsii-pacmak/lib/packaging.ts b/packages/jsii-pacmak/lib/packaging.ts index c7d8040876..24d054cecc 100644 --- a/packages/jsii-pacmak/lib/packaging.ts +++ b/packages/jsii-pacmak/lib/packaging.ts @@ -26,9 +26,15 @@ export interface JsiiModuleOptions { * Output directory where to package everything */ defaultOutputDirectory: string; + + /** + * Names of packages this package depends on, if any + */ + dependencyNames?: string[]; } export class JsiiModule { public readonly name: string; + public readonly dependencyNames: string[]; public readonly moduleDirectory: string; public readonly availableTargets: string[]; public outputDirectory: string; @@ -41,6 +47,7 @@ export class JsiiModule { this.moduleDirectory = options.moduleDirectory; this.availableTargets = options.availableTargets; this.outputDirectory = options.defaultOutputDirectory; + this.dependencyNames = options.dependencyNames || []; } /** diff --git a/packages/jsii-pacmak/lib/toposort.ts b/packages/jsii-pacmak/lib/toposort.ts new file mode 100644 index 0000000000..165cb88561 --- /dev/null +++ b/packages/jsii-pacmak/lib/toposort.ts @@ -0,0 +1,48 @@ +export type KeyFunc = (x: T) => string; +export type DepFunc = (x: T) => string[]; + +/** + * Return a topological sort of all elements of xs, according to the given dependency functions + * + * Dependencies outside the referenced set are ignored. + * + * Not a stable sort, but in order to keep the order as stable as possible, we'll sort by key + * among elements of equal precedence. + * + * @param xs - The elements to sort + * @param keyFn - Return an element's identifier + * @param depFn - Return the identifiers of an element's dependencies + */ +export function topologicalSort(xs: Iterable, keyFn: KeyFunc, depFn: DepFunc): T[] { + const remaining = new Map>(); + for (const element of xs) { + const key = keyFn(element); + remaining.set(key, { key, element, dependencies: depFn(element) }); + } + + const ret = new Array(); + while (remaining.size > 0) { + // All elements with no more deps in the set can be ordered + const selectable = Array.from(remaining.values()).filter(e => e.dependencies.every(d => !remaining.has(d))); + + selectable.sort((a, b) => a.key < b.key ? -1 : b.key < a.key ? 1 : 0); + + for (const selected of selectable) { + ret.push(selected.element); + remaining.delete(selected.key); + } + + // If we didn't make any progress, we got stuck + if (selectable.length === 0) { + throw new Error(`Could not determine ordering between: ${Array.from(remaining.keys()).join(', ')}`); + } + } + + return ret; +} + +interface TopoElement { + key: string; + dependencies: string[]; + element: T; +} \ No newline at end of file diff --git a/packages/jsii-pacmak/package.json b/packages/jsii-pacmak/package.json index 1c5ab417d1..a7073a1ae6 100644 --- a/packages/jsii-pacmak/package.json +++ b/packages/jsii-pacmak/package.json @@ -53,6 +53,8 @@ "@types/fs-extra": "^8.0.1", "@types/jest": "^24.0.22", "@types/node": "^10.17.4", + "mock-fs": "^4.10.2", + "@types/mock-fs": "^4.10.0", "@types/yargs": "^13.0.3", "@typescript-eslint/eslint-plugin": "^2.6.1", "@typescript-eslint/parser": "^2.6.1", diff --git a/packages/jsii-pacmak/test/npm-modules.test.ts b/packages/jsii-pacmak/test/npm-modules.test.ts new file mode 100644 index 0000000000..1b3414073f --- /dev/null +++ b/packages/jsii-pacmak/test/npm-modules.test.ts @@ -0,0 +1,65 @@ +import mockfs = require('mock-fs'); +import { findJsiiModules } from '../lib/npm-modules'; + +test('findJsiiModules is sorted topologically', async () => { + mockfs({ + '/packageA/package.json': JSON.stringify({ + name: 'packageA', + jsii: { + outdir: 'dist', + targets: { + python: {} + } + }, + dependencies: { + packageB: '*' + } + }), + '/packageB/package.json': JSON.stringify({ + name: 'packageB', + jsii: { + outdir: 'dist', + targets: { + python: {} + } + } + }), + }); + + try { + const mods = await findJsiiModules(['/packageA', '/packageB'], false); + expect(mods.map(m => m.name)).toEqual(['packageB', 'packageA']); + } finally { + mockfs.restore(); + } +}); + +test('findJsiiModules without deps loads packages in given order', async () => { + mockfs({ + '/packageA/package.json': JSON.stringify({ + name: 'packageA', + jsii: { + outdir: 'dist', + targets: { + python: {} + } + }, + }), + '/packageB/package.json': JSON.stringify({ + name: 'packageB', + jsii: { + outdir: 'dist', + targets: { + python: {} + } + } + }), + }); + + try { + const mods = await findJsiiModules(['/packageA', '/packageB'], false); + expect(mods.map(m => m.name)).toEqual(['packageA', 'packageB']); + } finally { + mockfs.restore(); + } +}); diff --git a/yarn.lock b/yarn.lock index 7332ca69e5..2fe6f4646b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1225,6 +1225,13 @@ dependencies: "@types/node" "*" +"@types/mock-fs@^4.10.0": + version "4.10.0" + resolved "https://registry.yarnpkg.com/@types/mock-fs/-/mock-fs-4.10.0.tgz#460061b186993d76856f669d5317cda8a007c24b" + integrity sha512-FQ5alSzmHMmliqcL36JqIA4Yyn9jyJKvRSGV3mvPh108VFatX7naJDzSG4fnFQNZFq9dIx0Dzoe6ddflMB2Xkg== + dependencies: + "@types/node" "*" + "@types/node@*": version "12.11.1" resolved "https://registry.yarnpkg.com/@types/node/-/node-12.11.1.tgz#1fd7b821f798b7fa29f667a1be8f3442bb8922a3" @@ -5414,6 +5421,11 @@ mkdirp@*, mkdirp@^0.5.0, mkdirp@^0.5.1: dependencies: minimist "0.0.8" +mock-fs@^4.10.2: + version "4.10.3" + resolved "https://registry.yarnpkg.com/mock-fs/-/mock-fs-4.10.3.tgz#d0550663dd2b5d33a7c1b8713c6925aab07a04ae" + integrity sha512-bcukePBvuA3qovmq0Qtqu9+1APCIGkFHnsozrPIVromt5XFGGgkQSfaN0H6RI8gStHkO/hRgimvS3tooNes4pQ== + modify-values@^1.0.0: version "1.0.1" resolved "https://registry.yarnpkg.com/modify-values/-/modify-values-1.0.1.tgz#b3939fa605546474e3e3e3c63d64bd43b4ee6022"