Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Assembly not found on Windows #1398

Merged
merged 6 commits into from
Apr 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions .github/workflows/build.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions .projenrc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,11 @@ const project = new CdklabsTypeScriptProject({
jestOptions: {
jestConfig: {
setupFilesAfterEnv: ['<rootDir>/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
'<rootDir>/test/**/*.test.ts',
],
},
},
tsconfig: {
Expand Down
1 change: 1 addition & 0 deletions package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 16 additions & 3 deletions projenrc/rosetta.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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: [{
Expand Down Expand Up @@ -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',
Expand All @@ -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';
}
}
2 changes: 1 addition & 1 deletion src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
39 changes: 32 additions & 7 deletions src/docgen/view/_npm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

// ensures npm does not insert anything in $PATH
'--no-bin-links',
// don't write or update a package-lock.json file
Expand Down Expand Up @@ -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);
Expand All @@ -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,
{
Expand All @@ -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;
}
Expand All @@ -188,7 +191,10 @@ export class Npm {
options?: SpawnOptionsWithoutStdio,
): Promise<CommandResult<T>> {
return new Promise<CommandResult<T>>((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<Buffer>();
child.stdout.on('data', (chunk) => {
stdout.push(Buffer.from(chunk));
Expand Down Expand Up @@ -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.')
Expand All @@ -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';
}
9 changes: 8 additions & 1 deletion src/docgen/view/documentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion test/docgen/view/documentation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down