Skip to content

fix: CDK app fails to launch if paths contain spaces #645

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/@aws-cdk/node-bundle/src/api/_attributions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,9 @@ export class Attributions {
// we don't use the programmatic API since it only offers an async API.
// prefer to stay sync for now since its easier to integrate with other tooling.
// will offer an async API further down the road.
const command = `${require.resolve('license-checker/bin/license-checker')} --json --packages "${_packages.join(';')}"`;
const output = shell(command, { cwd: _cwd, quiet: true });
const licenseCheckerPath = require.resolve('license-checker/bin/license-checker');
const args = ['--json', '--packages', `${_packages.join(';')}`];
const output = shell(licenseCheckerPath, args, { cwd: _cwd, quiet: true });
return JSON.parse(output);
}

Expand Down
11 changes: 9 additions & 2 deletions packages/@aws-cdk/node-bundle/src/api/_shell.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,16 @@ export interface ShellOptions {
readonly quiet?: boolean;
}

export function shell(command: string, options: ShellOptions = {}): string {
/**
* Execute a shell command with proper cross-platform support
* @param command The command to execute
* @param args The arguments to pass to the command
* @param options Additional options
* @returns The command output
*/
export function shell(command: string, args: string[] = [], options: ShellOptions = {}): string {
const stdio: child_process.StdioOptions = options.quiet ? ['ignore', 'pipe', 'pipe'] : ['ignore', 'inherit', 'inherit'];
const buffer = child_process.execSync(command, {
const buffer = child_process.execFileSync(command, args, {
cwd: options.cwd,
stdio: stdio,
});
Expand Down
13 changes: 7 additions & 6 deletions packages/@aws-cdk/node-bundle/src/api/bundle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -331,14 +331,14 @@ export class Bundle {
const bundleDir = this.write();
try {
if (this.test) {
const command = `${path.join(bundleDir, this.test)}`;
console.log(`Running sanity test: ${command}`);
shell(command, { cwd: bundleDir });
const testPath = path.join(bundleDir, this.test);
console.log(`Running sanity test: ${testPath}`);
shell(testPath, [], { cwd: bundleDir });
}

// create the tarball
console.log('Packing');
const tarball = shell('npm pack', { quiet: true, cwd: bundleDir }).trim();
const tarball = shell('npm', ['pack'], { quiet: true, cwd: bundleDir }).trim();
const dest = path.join(realTarget, tarball);
fs.copySync(path.join(bundleDir, tarball), dest, { recursive: true });
return dest;
Expand Down Expand Up @@ -481,8 +481,9 @@ export class Bundle {
// we don't use the programmatic API since it only offers an async API.
// prefer to stay sync for now since its easier to integrate with other tooling.
// will offer an async API further down the road.
const command = `${require.resolve('madge/bin/cli.js')} --json --warning --no-color --no-spinner --circular --extensions js ${packages.join(' ')}`;
shell(command, { quiet: true });
const madgePath = require.resolve('madge/bin/cli.js');
const args = ['--json', '--warning', '--no-color', '--no-spinner', '--circular', '--extensions', 'js', ...packages];
shell(madgePath, args, { quiet: true });
} catch (e: any) {
const imports: string[][] = JSON.parse(e.stdout.toString().trim());
for (const imp of imports) {
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/node-bundle/test/_package.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ export class Package {
* Create an npm tarballs of this package.
*/
public pack() {
shell('npm pack', { cwd: this.dir, quiet: true });
shell('npm', ['pack'], { cwd: this.dir, quiet: true });
}

/**
* Install package dependencies.
*/
public install() {
shell('npm install', { cwd: this.dir, quiet: true });
shell('npm', ['install'], { cwd: this.dir, quiet: true });
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@

/**
* Execute a command and args in a child process
* @param command The command to execute
* @param args Optional arguments for the command
* @param options Additional options for execution
*/
export async function execInChildProcess(commandAndArgs: string, options: ExecOptions = {}) {
export async function execInChildProcess(command: string, args: string[] = [], options: ExecOptions = {}) {
return new Promise<void>((ok, fail) => {
// We use a slightly lower-level interface to:
//
Expand All @@ -24,14 +27,15 @@
//
// - We have to capture any output to stdout and stderr sp we can pass it on to the IoHost
// To ensure messages get to the user fast, we will emit every full line we receive.
const proc = child_process.spawn(commandAndArgs, {
const proc = child_process.spawn(command, args, {
stdio: ['ignore', 'pipe', 'pipe'],
detached: false,
shell: true,
shell: true, // Keep shell: true for now to maintain compatibility with tests
cwd: options.cwd,
env: options.env,
});

const commandDisplay = `${command} ${args.join(' ')}`;
const eventPublisher: EventPublisher = options.eventPublisher ?? ((type, line) => {
switch (type) {
case 'data_stdout':
Expand All @@ -54,7 +58,7 @@
if (code === 0) {
return ok();
} else {
return fail(new ToolkitError(`${commandAndArgs}: Subprocess exited with error ${code}`));
return fail(new ToolkitError(`${commandDisplay}: Subprocess exited with error ${code}`));
}
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,7 @@ export abstract class CloudAssemblySourceBuilder {
});
const cleanupTemp = writeContextToEnv(env, fullContext);
try {
await execInChildProcess(commandLine.join(' '), {
await execInChildProcess(commandLine[0], commandLine.slice(1), {
eventPublisher: async (type, line) => {
switch (type) {
case 'data_stdout':
Expand Down
57 changes: 51 additions & 6 deletions packages/aws-cdk/lib/cli/util/npm.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,13 @@
import { exec as _exec } from 'child_process';
import { promisify } from 'util';
import { spawn } from 'child_process';
import { ToolkitError } from '@aws-cdk/toolkit-lib';

const exec = promisify(_exec);

/* c8 ignore start */
export async function execNpmView(currentVersion: string) {
try {
// eslint-disable-next-line @cdklabs/promiseall-no-unbounded-parallelism
const [latestResult, currentResult] = await Promise.all([
exec('npm view aws-cdk@latest version', { timeout: 3000 }),
exec(`npm view aws-cdk@${currentVersion} name version deprecated --json`, { timeout: 3000 }),
execCommand('npm', ['view', 'aws-cdk@latest', 'version'], { timeout: 3000 }),
execCommand('npm', ['view', `aws-cdk@${currentVersion}`, 'name', 'version', 'deprecated', '--json'], { timeout: 3000 }),
]);

if (latestResult.stderr && latestResult.stderr.trim().length > 0) {
Expand All @@ -31,4 +28,52 @@ export async function execNpmView(currentVersion: string) {
throw err;
}
}

interface ExecResult {
stdout: string;
stderr: string;
}

interface ExecOptions {
timeout?: number;
}

function execCommand(command: string, args: string[], options: ExecOptions = {}): Promise<ExecResult> {
return new Promise((resolve, reject) => {
const stdout: Buffer[] = [];
const stderr: Buffer[] = [];

const proc = spawn(command, args, {
shell: false,
});

let timeoutId: NodeJS.Timeout | undefined;
if (options.timeout) {
timeoutId = setTimeout(() => {
proc.kill();
reject(new Error(`Command timed out after ${options.timeout}ms: ${command} ${args.join(' ')}`));
}, options.timeout);
}

proc.stdout.on('data', (chunk) => stdout.push(Buffer.from(chunk)));
proc.stderr.on('data', (chunk) => stderr.push(Buffer.from(chunk)));

proc.on('error', (err) => {
if (timeoutId) clearTimeout(timeoutId);
reject(err);
});

proc.on('close', (code) => {
if (timeoutId) clearTimeout(timeoutId);
if (code === 0) {
resolve({
stdout: Buffer.concat(stdout).toString().trim(),
stderr: Buffer.concat(stderr).toString().trim(),
});
} else {
reject(new Error(`Command failed with exit code ${code}: ${command} ${args.join(' ')}`));
}
});
});
}
/* c8 ignore stop */
10 changes: 5 additions & 5 deletions packages/aws-cdk/lib/cxapp/exec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export async function execProgram(aws: SdkProvider, ioHelper: IoHelper, config:

const cleanupTemp = writeContextToEnv(env, context);
try {
await exec(commandLine.join(' '));
await exec(commandLine[0], commandLine.slice(1));

const assembly = createAssembly(outdir);

Expand All @@ -98,7 +98,7 @@ export async function execProgram(aws: SdkProvider, ioHelper: IoHelper, config:
await cleanupTemp();
}

async function exec(commandAndArgs: string) {
async function exec(command: string, args: string[] = []) {
try {
await new Promise<void>((ok, fail) => {
// We use a slightly lower-level interface to:
Expand All @@ -111,7 +111,7 @@ export async function execProgram(aws: SdkProvider, ioHelper: IoHelper, config:
// anyway, and if the subprocess is printing to it for debugging purposes the
// user gets to see it sooner. Plus, capturing doesn't interact nicely with some
// processes like Maven.
const proc = childProcess.spawn(commandAndArgs, {
const proc = childProcess.spawn(command, args, {
stdio: ['ignore', 'inherit', 'inherit'],
detached: false,
shell: true,
Expand All @@ -127,12 +127,12 @@ export async function execProgram(aws: SdkProvider, ioHelper: IoHelper, config:
if (code === 0) {
return ok();
} else {
return fail(new ToolkitError(`${commandAndArgs}: Subprocess exited with error ${code}`));
return fail(new ToolkitError(`${command} ${args.join(' ')}: Subprocess exited with error ${code}`));
}
});
});
} catch (e: any) {
await debugFn(`failed command: ${commandAndArgs}`);
await debugFn(`failed command: ${command} ${args.join(' ')}`);
throw e;
}
}
Expand Down
6 changes: 4 additions & 2 deletions packages/aws-cdk/test/_helpers/mock-child_process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@ export function mockSpawn(...invocations: Invocation[]) {
let mock = (child_process.spawn as any);
for (const _invocation of invocations) {
const invocation = _invocation; // Mirror into variable for closure
mock = mock.mockImplementationOnce((binary: string, options: child_process.SpawnOptions) => {
expect(binary).toEqual(invocation.commandLine);
mock = mock.mockImplementationOnce((command: string, args: string[] = [], options: child_process.SpawnOptions) => {
// Handle both old and new calling conventions
const fullCommand = args.length > 0 ? `${command} ${args.join(' ')}` : command;
expect(fullCommand).toEqual(invocation.commandLine);

if (invocation.cwd != null) {
expect(options.cwd).toBe(invocation.cwd);
Expand Down
Loading