fix(init-templates): csharp and fsharp app init fails when path conta…

…ins space (aws#21049) When running `cdk init --language=csharp` or `cdk init --language=fsharp` with one or more spaces in the path to the project, `init` will fail. This fix adds in handling for spaces and other special characters in the file path for both windows systems and posix systems. This PR moves the temporary hook directory to the same directory as the source directory so that it can use the local `os.ts` file and other dependencies. `ShellOptions` was also removed because it wasn't used. Tests have been added for posix and manual testing was performed on a windows machine. Closes issue aws#18803. ---- ### All Submissions: *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
DavidSchwarz2 · Sep 13, 2022 · 34e2828 · 34e2828
1 parent 7ae1cd8
commit 34e2828
Show file tree

Hide file tree

Showing 7 changed files with 99 additions and 137 deletions.
diff --git a/packages/aws-cdk/lib/init-templates/app/csharp/add-project.hook.ts b/packages/aws-cdk/lib/init-templates/app/csharp/add-project.hook.ts
@@ -1,33 +1,13 @@
-import * as child_process from 'child_process';
 import * as path from 'path';
 import { InvokeHook } from '../../../init';
+import { shell } from '../../../os';
 
 export const invoke: InvokeHook = async (targetDirectory: string) => {
   const slnPath = path.join(targetDirectory, 'src', '%name.PascalCased%.sln');
   const csprojPath = path.join(targetDirectory, 'src', '%name.PascalCased%', '%name.PascalCased%.csproj');
-
-  const child = child_process.spawn('dotnet', ['sln', slnPath, 'add', csprojPath], {
-    // Need this for Windows where we want .cmd and .bat to be found as well.
-    shell: true,
-    stdio: ['ignore', 'pipe', 'inherit'],
-  });
-
-  await new Promise<string>((resolve, reject) => {
-    const stdout = new Array<any>();
-
-    child.stdout.on('data', chunk => {
-      process.stdout.write(chunk);
-      stdout.push(chunk);
-    });
-
-    child.once('error', reject);
-
-    child.once('exit', code => {
-      if (code === 0) {
-        resolve(Buffer.concat(stdout).toString('utf-8'));
-      } else {
-        reject(new Error(`Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln. Error code: ${code}`));
-      }
-    });
-  });
+  try {
+    await shell(['dotnet', 'sln', slnPath, 'add', csprojPath]);
+  } catch (e) {
+    throw new Error(`Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln. ${e.message}`);
+  }
 };
diff --git a/packages/aws-cdk/lib/init-templates/app/fsharp/add-project.hook.ts b/packages/aws-cdk/lib/init-templates/app/fsharp/add-project.hook.ts
@@ -1,33 +1,13 @@
-import * as child_process from 'child_process';
 import * as path from 'path';
 import { InvokeHook } from '../../../init';
+import { shell } from '../../../os';
 
 export const invoke: InvokeHook = async (targetDirectory: string) => {
   const slnPath = path.join(targetDirectory, 'src', '%name.PascalCased%.sln');
   const fsprojPath = path.join(targetDirectory, 'src', '%name.PascalCased%', '%name.PascalCased%.fsproj');
-
-  const child = child_process.spawn('dotnet', ['sln', slnPath, 'add', fsprojPath], {
-    // Need this for Windows where we want .cmd and .bat to be found as well.
-    shell: true,
-    stdio: ['ignore', 'pipe', 'inherit'],
-  });
-
-  await new Promise<string>((resolve, reject) => {
-    const stdout = new Array<any>();
-
-    child.stdout.on('data', chunk => {
-      process.stdout.write(chunk);
-      stdout.push(chunk);
-    });
-
-    child.once('error', reject);
-
-    child.once('exit', code => {
-      if (code === 0) {
-        resolve(Buffer.concat(stdout).toString('utf-8'));
-      } else {
-        reject(new Error(`Could not add project %name.PascalCased%.fsproj to solution %name.PascalCased%.sln. Error code: ${code}`));
-      }
-    });
-  });
+  try {
+    await shell(['dotnet', 'sln', slnPath, 'add', fsprojPath]);
+  } catch (e) {
+    throw new Error(`Could not add project %name.PascalCased%.fsproj to solution %name.PascalCased%.sln. ${e.message}`);
+  }
 };
diff --git a/packages/aws-cdk/lib/init-templates/sample-app/csharp/add-project.hook.ts b/packages/aws-cdk/lib/init-templates/sample-app/csharp/add-project.hook.ts
@@ -1,33 +1,13 @@
-import * as child_process from 'child_process';
 import * as path from 'path';
 import { InvokeHook } from '../../../init';
+import { shell } from '../../../os';
 
 export const invoke: InvokeHook = async (targetDirectory: string) => {
   const slnPath = path.join(targetDirectory, 'src', '%name.PascalCased%.sln');
   const csprojPath = path.join(targetDirectory, 'src', '%name.PascalCased%', '%name.PascalCased%.csproj');
-
-  const child = child_process.spawn('dotnet', ['sln', slnPath, 'add', csprojPath], {
-    // Need this for Windows where we want .cmd and .bat to be found as well.
-    shell: true,
-    stdio: ['ignore', 'pipe', 'inherit'],
-  });
-
-  await new Promise<string>((resolve, reject) => {
-    const stdout = new Array<any>();
-
-    child.stdout.on('data', chunk => {
-      process.stdout.write(chunk);
-      stdout.push(chunk);
-    });
-
-    child.once('error', reject);
-
-    child.once('exit', code => {
-      if (code === 0) {
-        resolve(Buffer.concat(stdout).toString('utf-8'));
-      } else {
-        reject(new Error(`Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln. Error code: ${code}`));
-      }
-    });
-  });
+  try {
+    await shell(['dotnet', 'sln', slnPath, 'add', csprojPath]);
+  } catch (e) {
+    throw new Error(`Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln. ${e.message}`);
+  }
 };
diff --git a/packages/aws-cdk/lib/init-templates/sample-app/fsharp/add-project.hook.ts b/packages/aws-cdk/lib/init-templates/sample-app/fsharp/add-project.hook.ts
@@ -1,33 +1,13 @@
-import * as child_process from 'child_process';
 import * as path from 'path';
 import { InvokeHook } from '../../../init';
+import { shell } from '../../../os';
 
 export const invoke: InvokeHook = async (targetDirectory: string) => {
   const slnPath = path.join(targetDirectory, 'src', '%name.PascalCased%.sln');
   const fsprojPath = path.join(targetDirectory, 'src', '%name.PascalCased%', '%name.PascalCased%.fsproj');
-
-  const child = child_process.spawn('dotnet', ['sln', slnPath, 'add', fsprojPath], {
-    // Need this for Windows where we want .cmd and .bat to be found as well.
-    shell: true,
-    stdio: ['ignore', 'pipe', 'inherit'],
-  });
-
-  await new Promise<string>((resolve, reject) => {
-    const stdout = new Array<any>();
-
-    child.stdout.on('data', chunk => {
-      process.stdout.write(chunk);
-      stdout.push(chunk);
-    });
-
-    child.once('error', reject);
-
-    child.once('exit', code => {
-      if (code === 0) {
-        resolve(Buffer.concat(stdout).toString('utf-8'));
-      } else {
-        reject(new Error(`Could not add project %name.PascalCased%.fsproj to solution %name.PascalCased%.sln. Error code: ${code}`));
-      }
-    });
-  });
+  try {
+    await shell(['dotnet', 'sln', slnPath, 'add', fsprojPath]);
+  } catch (e) {
+    throw new Error(`Could not add project %name.PascalCased%.fsproj to solution %name.PascalCased%.sln. ${e.message}`);
+  }
 };
diff --git a/packages/aws-cdk/lib/init.ts b/packages/aws-cdk/lib/init.ts
@@ -69,11 +69,12 @@ function pythonExecutable() {
   return python;
 }
 const INFO_DOT_JSON = 'info.json';
+const HOOK_DIR_PREFIX = 'tmp';
 
 export class InitTemplate {
   public static async fromName(templatesDir: string, name: string) {
     const basePath = path.join(templatesDir, name);
-    const languages = (await listDirectory(basePath)).filter(f => f !== INFO_DOT_JSON);
+    const languages = (await listDirectory(basePath));
     const info = await fs.readJson(path.join(basePath, INFO_DOT_JSON));
     return new InitTemplate(basePath, name, languages, info);
   }
@@ -117,6 +118,9 @@ export class InitTemplate {
       name: decamelize(path.basename(path.resolve(targetDirectory))),
     };
 
+    const sourceDirectory = path.join(this.basePath, language);
+    const hookTempDirectory = path.join(this.basePath, `${HOOK_DIR_PREFIX}-${projectInfo.name}`);
+
     const hookContext: HookContext = {
       substitutePlaceholdersIn: async (...fileNames: string[]) => {
         for (const fileName of fileNames) {
@@ -127,28 +131,31 @@ export class InitTemplate {
       },
     };
 
-    const sourceDirectory = path.join(this.basePath, language);
-    const hookTempDirectory = path.join(targetDirectory, 'tmp');
-    await fs.mkdir(hookTempDirectory);
-    await this.installFiles(sourceDirectory, targetDirectory, language, projectInfo);
-    await this.applyFutureFlags(targetDirectory);
-    await this.invokeHooks(hookTempDirectory, targetDirectory, hookContext);
-    await fs.remove(hookTempDirectory);
+    try {
+      await fs.mkdir(hookTempDirectory);
+      await this.installFiles(sourceDirectory, targetDirectory, hookTempDirectory, language, projectInfo);
+      await this.applyFutureFlags(targetDirectory);
+      await this.invokeHooks(hookTempDirectory, targetDirectory, hookContext);
+    } catch (e) {
+      warning(`Unable to create ${projectInfo.name}: ${e.message}`);
+    } finally {
+      await fs.remove(hookTempDirectory);
+    }
   }
 
-  private async installFiles(sourceDirectory: string, targetDirectory: string, language:string, project: ProjectInfo) {
+  private async installFiles(sourceDirectory: string, targetDirectory: string, hookTempDirectory: string, language:string, project: ProjectInfo) {
     for (const file of await fs.readdir(sourceDirectory)) {
       const fromFile = path.join(sourceDirectory, file);
       const toFile = path.join(targetDirectory, this.expand(file, language, project));
       if ((await fs.stat(fromFile)).isDirectory()) {
         await fs.mkdir(toFile);
-        await this.installFiles(fromFile, toFile, language, project);
+        await this.installFiles(fromFile, toFile, hookTempDirectory, language, project);
         continue;
       } else if (file.match(/^.*\.template\.[^.]+$/)) {
         await this.installProcessed(fromFile, toFile.replace(/\.template(\.[^.]+)$/, '$1'), language, project);
         continue;
       } else if (file.match(/^.*\.hook\.(d.)?[^.]+$/)) {
-        await this.installProcessed(fromFile, path.join(targetDirectory, 'tmp', file), language, project);
+        await this.installProcessed(fromFile, path.join(hookTempDirectory, file), language, project);
         continue;
       } else {
         await fs.copy(fromFile, toFile);
@@ -272,6 +279,9 @@ async function listDirectory(dirPath: string) {
   return (await fs.readdir(dirPath))
     .filter(p => !p.startsWith('.'))
     .filter(p => !(p === 'LICENSE'))
+    // if, for some reason, the temp folder for the hook doesn't get deleted we don't want to display it in this list
+    .filter(p => !p.startsWith(HOOK_DIR_PREFIX))
+    .filter(p => !(p === INFO_DOT_JSON))
     .sort();
 }
 

diff --git a/packages/aws-cdk/lib/os.ts b/packages/aws-cdk/lib/os.ts
@@ -2,20 +2,18 @@ import * as child_process from 'child_process';
 import * as chalk from 'chalk';
 import { debug } from './logging';
 
-export interface ShellOptions extends child_process.SpawnOptions {
-  quiet?: boolean;
-}
-
 /**
  * OS helpers
  *
  * Shell function which both prints to stdout and collects the output into a
  * string.
  */
-export async function shell(command: string[], options: ShellOptions = {}): Promise<string> {
-  debug(`Executing ${chalk.blue(renderCommandLine(command))}`);
-  const child = child_process.spawn(command[0], command.slice(1), {
-    ...options,
+export async function shell(command: string[]): Promise<string> {
+  const commandLine = renderCommandLine(command);
+  debug(`Executing ${chalk.blue(commandLine)}`);
+  const child = child_process.spawn(command[0], renderArguments(command.slice(1)), {
+    // Need this for Windows where we want .cmd and .bat to be found as well.
+    shell: true,
     stdio: ['ignore', 'pipe', 'inherit'],
   });
 
@@ -24,30 +22,30 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom
 
     // Both write to stdout and collect
     child.stdout.on('data', chunk => {
-      if (!options.quiet) {
-        process.stdout.write(chunk);
-      }
+      process.stdout.write(chunk);
       stdout.push(chunk);
     });
 
     child.once('error', reject);
 
     child.once('exit', code => {
       if (code === 0) {
-        resolve(Buffer.concat(stdout).toString('utf-8'));
+        resolve(Buffer.from(stdout).toString('utf-8'));
       } else {
-        reject(new Error(`${renderCommandLine(command)} exited with error code ${code}`));
+        reject(new Error(`${commandLine} exited with error code ${code}`));
       }
     });
   });
 }
 
+function renderCommandLine(cmd: string[]) {
+  return renderArguments(cmd).join(' ');
+}
+
 /**
- * Render the given command line as a string
- *
- * Probably missing some cases but giving it a good effort.
+ * Render the arguments to include escape characters for each platform.
  */
-function renderCommandLine(cmd: string[]) {
+function renderArguments(cmd: string[]) {
   if (process.platform !== 'win32') {
     return doRender(cmd, hasAnyChars(' ', '\\', '!', '"', "'", '&', '$'), posixEscape);
   } else {
@@ -58,8 +56,8 @@ function renderCommandLine(cmd: string[]) {
 /**
  * Render a UNIX command line
  */
-function doRender(cmd: string[], needsEscaping: (x: string) => boolean, doEscape: (x: string) => string): string {
-  return cmd.map(x => needsEscaping(x) ? doEscape(x) : x).join(' ');
+function doRender(cmd: string[], needsEscaping: (x: string) => boolean, doEscape: (x: string) => string): string[] {
+  return cmd.map(x => needsEscaping(x) ? doEscape(x) : x);
 }
 
 /**
@@ -78,7 +76,7 @@ function hasAnyChars(...chars: string[]): (x: string) => boolean {
  */
 function posixEscape(x: string) {
   // Turn ' -> '"'"'
-  x = x.replace("'", "'\"'\"'");
+  x = x.replace(/'/g, "'\"'\"'");
   return `'${x}'`;
 }
 
@@ -95,4 +93,4 @@ function windowsEscape(x: string): string {
   // Now escape all special characters
   const shellMeta = new Set<string>(['"', '&', '^', '%']);
   return x.split('').map(c => shellMeta.has(x) ? '^' + c : c).join('');
-}
+}
diff --git a/packages/aws-cdk/test/init.test.ts b/packages/aws-cdk/test/init.test.ts
@@ -76,6 +76,28 @@ describe('constructs version', () => {
     expect(sln).toContainEqual(expect.stringMatching(/\"AwsCdkTest[a-zA-Z0-9]{6}\\AwsCdkTest[a-zA-Z0-9]{6}.fsproj\"/));
   });
 
+  cliTestWithDirSpaces('csharp app with spaces', async (workDir) => {
+    await cliInit('app', 'csharp', false, true, workDir);
+
+    const csprojFile = (await recursiveListFiles(workDir)).filter(f => f.endsWith('.csproj'))[0];
+    expect(csprojFile).toBeDefined();
+
+    const csproj = (await fs.readFile(csprojFile, 'utf8')).split(/\r?\n/);
+
+    expect(csproj).toContainEqual(expect.stringMatching(/\<PackageReference Include="Constructs" Version="\[10\..*,11\..*\)"/));
+  });
+
+  cliTestWithDirSpaces('fsharp app with spaces', async (workDir) => {
+    await cliInit('app', 'fsharp', false, true, workDir);
+
+    const fsprojFile = (await recursiveListFiles(workDir)).filter(f => f.endsWith('.fsproj'))[0];
+    expect(fsprojFile).toBeDefined();
+
+    const fsproj = (await fs.readFile(fsprojFile, 'utf8')).split(/\r?\n/);
+
+    expect(fsproj).toContainEqual(expect.stringMatching(/\<PackageReference Include="Constructs" Version="\[10\..*,11\..*\)"/));
+  });
+
   cliTest('create a Python app project', async (workDir) => {
     await cliInit('app', 'python', false, true, workDir);
 
@@ -147,7 +169,6 @@ describe('constructs version', () => {
 });
 
 test('when no version number is present (e.g., local development), the v2 templates are chosen by default', async () => {
-
   expect((await availableInitTemplates()).length).toBeGreaterThan(0);
 });
 
@@ -164,6 +185,19 @@ async function withTempDir(cb: (dir: string) => void | Promise<any>) {
   }
 }
 
+function cliTestWithDirSpaces(name: string, handler: (dir: string) => void | Promise<any>): void {
+  test(name, () => withTempDirWithSpaces(handler));
+}
+
+async function withTempDirWithSpaces(cb: (dir: string) => void | Promise<any>) {
+  const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), 'aws-cdk-test with-space'));
+  try {
+    await cb(tmpDir);
+  } finally {
+    await fs.remove(tmpDir);
+  }
+}
+
 /**
  * List all files underneath dir
  */