From 7550410e3c477b053b9488978dc4e7fb6de140ae Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Mon, 29 Apr 2024 14:21:01 +0100 Subject: [PATCH] fix: Assembly not found on Windows (#1398) jsii-docgen fails in Windows with the error `Assembly "" not found`. The reason for this is that the path passed to the glob library we use must only use forward slashes. However on Windows when using jsii-docgen, it may contain backslashes. To fix the issue, we pass the assemblies directory as CWD to glob instead. The CWD doesn't have the same limitation in regards to backslashes. Other fixes involve making the npm helpers Windows aware. And finally this PR adds a Windows test to the compat build matrix. --- .github/workflows/build.yml | 9 ++++-- .projenrc.ts | 5 ++++ package.json | 1 + projenrc/rosetta.ts | 19 +++++++++++-- src/cli.ts | 2 +- src/docgen/view/_npm.ts | 39 +++++++++++++++++++++----- src/docgen/view/documentation.ts | 9 +++++- test/docgen/view/documentation.test.ts | 2 +- 8 files changed, 71 insertions(+), 15 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 4e3824fb..58f19862 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -79,7 +79,7 @@ jobs: git commit -s -m "chore: self mutation" git push origin HEAD:$PULL_REQUEST_REF rosetta-compat: - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} permissions: {} env: CI: "true" @@ -101,11 +101,14 @@ jobs: - name: compile+test run: |- npx projen compile - npx projen test + npx projen test --runInBand - name: Check Rosetta version run: test $(npx jsii-rosetta --version) = "${{ matrix.rosetta }}" strategy: matrix: + include: + - rosetta: 5.4.0 + os: windows-latest rosetta: - 1.85.0 - 5.0.14 @@ -113,3 +116,5 @@ jobs: - 5.2.0 - 5.3.0 - 5.4.0 + os: + - ubuntu-latest diff --git a/.projenrc.ts b/.projenrc.ts index bf78d740..9dd3f361 100644 --- a/.projenrc.ts +++ b/.projenrc.ts @@ -47,6 +47,11 @@ const project = new CdklabsTypeScriptProject({ jestOptions: { jestConfig: { setupFilesAfterEnv: ['/test/setup-jest.ts'], + testMatch: [ + // On Windows the standard tests paths are not matched + // Use this simplified version instead that works good enough in this repo + '/test/**/*.test.ts', + ], }, }, tsconfig: { diff --git a/package.json b/package.json index ecf09ac2..fc63d553 100644 --- a/package.json +++ b/package.json @@ -78,6 +78,7 @@ "/test/setup-jest.ts" ], "testMatch": [ + "/test/**/*.test.ts", "/src/**/__tests__/**/*.ts?(x)", "/(test|src)/**/*(*.)@(spec|test).ts?(x)" ], diff --git a/projenrc/rosetta.ts b/projenrc/rosetta.ts index 4d0c8c0c..229c3d74 100644 --- a/projenrc/rosetta.ts +++ b/projenrc/rosetta.ts @@ -25,11 +25,14 @@ export class RosettaPeerDependency extends Component { super(project); const constraint = this.calculateVersionConstraint(options.supportedVersions); + const minVersions = this.calculateMinimalVersions(options.supportedVersions); + const latestVersion = this.calculateLatestVersion(options.supportedVersions); + project.addDevDeps(constraint); project.addPeerDeps(constraint); project.github?.tryFindWorkflow('build')?.addJob('rosetta-compat', { - runsOn: ['ubuntu-latest'], + runsOn: ['${{ matrix.os }}'], permissions: {}, env: { CI: 'true', @@ -38,8 +41,13 @@ export class RosettaPeerDependency extends Component { strategy: { matrix: { domain: { - rosetta: this.calculateMinimalVersions(options.supportedVersions), + rosetta: minVersions, + os: ['ubuntu-latest'], }, + include: [{ + rosetta: latestVersion, + os: 'windows-latest', + }], }, }, steps: [{ @@ -68,7 +76,7 @@ export class RosettaPeerDependency extends Component { }, { name: 'compile+test', - run: ['npx projen compile', 'npx projen test'].join('\n'), + run: ['npx projen compile', 'npx projen test --runInBand'].join('\n'), }, { name: 'Check Rosetta version', @@ -95,4 +103,9 @@ export class RosettaPeerDependency extends Component { return discoveredVersions; } + + private calculateLatestVersion(versions: RosettaPeerDependencyOptions['supportedVersions']): string { + const discoveredVersions = this.calculateMinimalVersions(versions); + return discoveredVersions.sort().pop() ?? 'latest'; + } } diff --git a/src/cli.ts b/src/cli.ts index 5ecac7c4..eba98920 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -59,7 +59,7 @@ export async function main() { .option('readme', { alias: 'r', type: 'boolean', required: false, desc: 'Include the user defined README.md in the documentation.' }) .option('submodule', { alias: 's', type: 'string', required: false, desc: 'Generate docs for a specific submodule (or "root")' }) .option('split-by-submodule', { type: 'boolean', required: false, desc: 'Generate a separate file for each submodule' }) - .example('$0', 'Generate documentation for the current module as a single file (auto-resolves node depedencies)') + .example('$0', 'Generate documentation for the current module as a single file (auto-resolves node dependencies)') .argv; const submodule = args.submodule === 'root' ? undefined : args.submodule; diff --git a/src/docgen/view/_npm.ts b/src/docgen/view/_npm.ts index c0201a11..f81ce153 100644 --- a/src/docgen/view/_npm.ts +++ b/src/docgen/view/_npm.ts @@ -38,7 +38,7 @@ export class Npm { // code execution as part of the install command using npm hooks. (e.g postInstall) '--ignore-scripts', // save time by not running audit - '--no-autit', + '--no-audit', // ensures npm does not insert anything in $PATH '--no-bin-links', // don't write or update a package-lock.json file @@ -132,17 +132,20 @@ export class Npm { return this.#npmCommand; } + // Get the platform specific npm command + const npm = npmPlatformAwareCommand(); + try { // If the npm in $PATH is >= v7, we can use that directly. The // `npm version --json` command returns a JSON object containing the // versions of several components (npm, node, v8, etc...). We are only // interested in the `npm` key here. const { exitCode, stdout } = await this.runCommand( - 'npm', ['version', '--json'], + npm, ['version', '--json'], chunksToObject, ); if (exitCode === 0 && major((stdout as any).npm) >= 7) { - return this.#npmCommand = 'npm'; + return this.#npmCommand = npm; } } catch (e) { this.logger('Could not determine version of npm in $PATH:', e); @@ -152,7 +155,7 @@ export class Npm { // the full type system. this.logger('The npm in $PATH is not >= v7. Installing npm@8 locally...'); const result = await this.runCommand( - 'npm', + npm, ['install', 'npm@8', '--no-package-lock', '--no-save', '--json'], chunksToObject, { @@ -162,7 +165,7 @@ export class Npm { ); assertSuccess(result); - this.#npmCommand = join(this.workingDirectory, 'node_modules', '.bin', 'npm'); + this.#npmCommand = join(this.workingDirectory, 'node_modules', '.bin', npm); this.logger(`Done installing npm@8 at ${this.#npmCommand}`); return this.#npmCommand; } @@ -188,7 +191,10 @@ export class Npm { options?: SpawnOptionsWithoutStdio, ): Promise> { return new Promise>((ok, ko) => { - const child = spawn(command, args, { ...options, stdio: ['inherit', 'pipe', 'pipe'] }); + // On Windows, spawning a program ending in .cmd or .bat needs to run in a shell + // https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2 + const shell = onWindows() && (command.endsWith('.cmd') || command.endsWith('.bat')); + const child = spawn(command, args, { shell, ...options, stdio: ['inherit', 'pipe', 'pipe'] }); const stdout = new Array(); child.stdout.on('data', (chunk) => { stdout.push(Buffer.from(chunk)); @@ -310,7 +316,7 @@ function chunksToObject(chunks: readonly Buffer[], encoding: BufferEncoding = 'u // observed these log lines always start with 'npm', so we filter those out. // for example: "npm notice New patch version of npm available! 8.1.0 -> 8.1.3" // for example: "npm ERR! must provide string spec" - const onlyJson = raw.split(os.EOL) + const onlyJson = raw.split(/[\r\n]+/) // split on any newlines, because npm returns inconsistent newline characters on Windows .filter(l => !l.startsWith('npm')) // Suppress debugger messages, if present... .filter(l => l !== 'Debugger attached.') @@ -330,3 +336,22 @@ type ResponseObject = | { readonly error: { readonly code: string; readonly summary: string; readonly detail: string } } // The successful objects are treated as opaque blobs here | { readonly error: undefined; readonly [key: string]: unknown }; + +/** + * Helper to detect if we are running on Windows. + */ +function onWindows() { + return process.platform === 'win32'; +} + +/** + * Get the npm binary path depending on the platform. + * @returns "npm.cmd" on Windows, otherwise "npm" + */ +function npmPlatformAwareCommand() { + if (onWindows()) { + return 'npm.cmd'; + } + + return 'npm'; +} diff --git a/src/docgen/view/documentation.ts b/src/docgen/view/documentation.ts index 5942b2cc..c915c61a 100644 --- a/src/docgen/view/documentation.ts +++ b/src/docgen/view/documentation.ts @@ -407,7 +407,14 @@ export class Documentation { }); const ts = new reflect.TypeSystem(); - for (let dotJsii of await glob.promise(`${this.assembliesDir}/**/${SPEC_FILE_NAME}`)) { + + // assembliesDir might include backslashes on Windows. + // The glob pattern must only used forward slashes, so we pass the assembliesDir as CWD which does not have this restriction + const assemblies = await glob.promise(`**/${SPEC_FILE_NAME}`, { + cwd: path.normalize(this.assembliesDir), + absolute: true, + }); + for (let dotJsii of assemblies) { // we only transliterate the top level assembly and not the entire type-system. // note that the only reason to translate dependant assemblies is to show code examples // for expanded python arguments - which we don't to right now anyway. diff --git a/test/docgen/view/documentation.test.ts b/test/docgen/view/documentation.test.ts index fb6104fd..00d0e961 100644 --- a/test/docgen/view/documentation.test.ts +++ b/test/docgen/view/documentation.test.ts @@ -76,7 +76,7 @@ test('package installation does not run lifecycle hooks, includes optional depen } }); -// This test is only revent when running on non-macOS platforms. +// This test is only relevant when running on non-macOS platforms. (process.platform === 'darwin' ? test.skip : test)('package installation uses --force when EBADPLATFORM is encountered', async () => { // The package has a hard dependency on fsevents, which only supports macOS platforms. const docs = await Documentation.forPackage(