fix(init-templates): csharp and fsharp app init fails when path contains space #21049

TheRealAmazonKendra · 2022-07-08T05:21:29Z

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 #18803.

All Submissions:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2022-07-08T05:21:35Z

packages/aws-cdk/lib/init.ts

rix0rrr · 2022-07-08T08:40:23Z

packages/aws-cdk/lib/init-templates/app/util/os.ts

      } else {
-        reject(new Error(`${renderCommandLine(command)} exited with error code ${code}`));
+        reject(new Error(`${options.errorMessage} exited with error code ${code}`));


This used to say something like

dotnet add bla exited with error code 1

And will now say

Could not add project project.fsproj to solution project.sln exited with code 1

This is hiding information now, we can't tell the command line anymore.

I pulled this from what we were actually outputting in the original version of the hook file, which was

reject(new Error(`Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln. Error code: ${code}`));

Both pieces of context are probably helpful in this case so I'll combine the two for the error message.

rix0rrr · 2022-07-08T08:40:41Z

packages/aws-cdk/lib/init-templates/app/util/os.ts

@@ -24,30 +22,26 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom

    // Both write to stdout and collect
    child.stdout.on('data', chunk => {
-      if (!options.quiet) {


Why did the quiet feature get removed?

(If it's because it was unused and you took the opportunity to clean up a bit, please describe that in the PR so reviewers don't have to guess. I usually add an "Also in this PR..." section for those)

rix0rrr · 2022-07-08T08:42:43Z

packages/aws-cdk/lib/init-templates/app/csharp/add-project.hook.ts

-    });
-  });
-};
+  await shell(['dotnet', 'sln', slnPath, 'add', csprojPath], { errorMessage: 'Could not add project %name.PascalCased%.csproj to solution %name.PascalCased%.sln.' });


What's the advantage of adding the errorMessage feature over just adding a try/catch here?

Seems more orthogonal to leave it to consumer to decide how to handle errors.

packages/aws-cdk/test/init.test.ts

rix0rrr · 2022-07-08T08:44:21Z

packages/aws-cdk/lib/init-templates/app/util/os.ts

-  const child = child_process.spawn(command[0], command.slice(1), {
+export async function shell(command: string[], options: ShellOptions): Promise<string> {
+  const child = child_process.spawn(command[0], renderArguments(command.slice(1)), {
+    shell: true,


Why shell: true ?

What happens if we don't pass this command through the shell?

packages/aws-cdk/lib/init-templates/app/util/os.ts

…space When running or with a space in the path to the project, init will fail. This fix adds in handling for spaces and other special characters in the filepath for both windows systems and posix systems. Tests have been added for posix and manual testing was performed on a windows machine. Closes issue #18803.

TheRealAmazonKendra · 2022-09-09T00:50:58Z

@Mergifyio update

mergify · 2022-09-09T00:51:31Z

update

✅ Branch has been successfully updated

mergify · 2022-09-09T08:15:01Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TheRealAmazonKendra · 2022-09-09T11:43:30Z

@Mergifyio update

mergify · 2022-09-09T11:44:08Z

update

☑️ Nothing to do

#commits-behind>0 [:pushpin: update requirement]
-closed [:pushpin: update requirement]

aws-cdk-automation · 2022-09-09T12:29:53Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a2089d1
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-09-09T12:30:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…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*

Historically, `cdk init` used to create a dedicated temporary directory for hook scripts and copy `*.hook.*` scripts into there. In #21049, the logic was changed to create that temporary directory inside the CLI source directory. If that CLI source directory is mounted in a read-only location (say, `/usr/lib/node_modules`) then that directory could not be created and `cdk init` would fail. It looks like historically we might copy and postprocess hook scripts so that they could have variables replaced... but given that hook scripts are code, they could just read the variables directly, so we don't have to copy them into a temporary directory at all: we can directly run them from the source location. Fixes #22090.

…th contains space (#21049)" This reverts commit 79c9ca1.

…ntains space (#22112) Reverts #21049 Resolves #22090

Historically, `cdk init` used to create a dedicated temporary directory for hook scripts and copy `*.hook.*` scripts into there. In #21049, the logic was changed to create that temporary directory inside the CLI source directory. If that CLI source directory is mounted in a read-only location (say, `/usr/lib/node_modules`) then that directory could not be created and `cdk init` would fail. It looks like historically we might copy and postprocess hook scripts so that they could have variables replaced... but given that hook scripts are code, they could just read the variables directly, so we don't have to copy them into a temporary directory at all: we can directly run them from the source location. Fixes #22090.

…ntains space (#22112) Reverts #21049 Resolves #22090

Historically, `cdk init` used to create a dedicated temporary directory for hook scripts and copy `*.hook.*` scripts into there. In #21049, the logic was changed to create that temporary directory inside the CLI source directory. If that CLI source directory is mounted in a read-only location (say, `/usr/lib/node_modules`) then that directory could not be created and `cdk init` would fail. Historically, hook scripts were arbitrary scripts outside the scope of the CLI, but the previous change tried to reuse code from the CLI. That does not work because the CLI is now being bundled (all code and dependencies in one giant `.js` file), so reusing from the outside using a different entry point cannot work. (It's not clear that this is happening because we leave the source files in the original location inside the NPM package, to try and halfway not break people using the CLI in ways that are unsupported but happen to work). Instead, bundle the hook logic into the CLI itself, so it all uses the same mechanism. Fixes #22090. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Historically, `cdk init` used to create a dedicated temporary directory for hook scripts and copy `*.hook.*` scripts into there. In aws#21049, the logic was changed to create that temporary directory inside the CLI source directory. If that CLI source directory is mounted in a read-only location (say, `/usr/lib/node_modules`) then that directory could not be created and `cdk init` would fail. Historically, hook scripts were arbitrary scripts outside the scope of the CLI, but the previous change tried to reuse code from the CLI. That does not work because the CLI is now being bundled (all code and dependencies in one giant `.js` file), so reusing from the outside using a different entry point cannot work. (It's not clear that this is happening because we leave the source files in the original location inside the NPM package, to try and halfway not break people using the CLI in ways that are unsupported but happen to work). Instead, bundle the hook logic into the CLI itself, so it all uses the same mechanism. Fixes aws#22090. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot added the p2 label Jul 8, 2022

aws-cdk-automation requested a review from a team July 8, 2022 05:21

mergify bot added the contribution/core This is a PR that came from AWS. label Jul 8, 2022

TheRealAmazonKendra added p1 and removed p2 labels Jul 8, 2022

rix0rrr requested changes Jul 8, 2022

View reviewed changes

TheRealAmazonKendra force-pushed the TheRealAmazonKendra/init-template-fix branch from 09b7e2f to 0960e90 Compare September 3, 2022 02:08

mergify bot and others added 6 commits September 9, 2022 00:51

Merge branch 'main' into TheRealAmazonKendra/init-template-fix

da6bd5e

move temp hook folder to source folder

62a4205

formatting and missing shell line

fa02577

add more context to error message

8a7d16b

render whole line

4929d3e

update sample apps as well

1f4a16b

rix0rrr approved these changes Sep 9, 2022

View reviewed changes

Merge branch 'main' into TheRealAmazonKendra/init-template-fix

a2089d1

mergify bot merged commit 79c9ca1 into main Sep 9, 2022

mergify bot deleted the TheRealAmazonKendra/init-template-fix branch September 9, 2022 12:30

rix0rrr mentioned this pull request Sep 19, 2022

fix(cli): "EACCES: Permission denied" on 'cdk init' #22111

Merged

iliapolo added a commit that referenced this pull request Sep 19, 2022

Revert "fix(init-templates): csharp and fsharp app init fails when pa…

da56be9

…th contains space (#21049)" This reverts commit 79c9ca1.

iliapolo mentioned this pull request Sep 19, 2022

revert(init-templates): csharp and fsharp app init fails when path contains space #22112

Merged

mergify bot pushed a commit that referenced this pull request Sep 19, 2022

revert(init-templates): csharp and fsharp app init fails when path co…

205e493

…ntains space (#22112) Reverts #21049 Resolves #22090

iliapolo added a commit that referenced this pull request Sep 19, 2022

revert(init-templates): csharp and fsharp app init fails when path co…

89f64d4

…ntains space (#22112) Reverts #21049 Resolves #22090

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(init-templates): csharp and fsharp app init fails when path contains space #21049

fix(init-templates): csharp and fsharp app init fails when path contains space #21049

TheRealAmazonKendra commented Jul 8, 2022 •

edited

Loading

gitpod-io bot commented Jul 8, 2022

rix0rrr Jul 8, 2022

TheRealAmazonKendra Sep 9, 2022

rix0rrr Jul 8, 2022

rix0rrr Jul 8, 2022

rix0rrr Jul 8, 2022

rix0rrr Jul 8, 2022

TheRealAmazonKendra commented Sep 9, 2022

mergify bot commented Sep 9, 2022

mergify bot commented Sep 9, 2022

TheRealAmazonKendra commented Sep 9, 2022

mergify bot commented Sep 9, 2022

aws-cdk-automation commented Sep 9, 2022

mergify bot commented Sep 9, 2022

fix(init-templates): csharp and fsharp app init fails when path contains space #21049

fix(init-templates): csharp and fsharp app init fails when path contains space #21049

Conversation

TheRealAmazonKendra commented Jul 8, 2022 • edited Loading

All Submissions:

gitpod-io bot commented Jul 8, 2022

rix0rrr Jul 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra Sep 9, 2022

Choose a reason for hiding this comment

rix0rrr Jul 8, 2022

Choose a reason for hiding this comment

rix0rrr Jul 8, 2022

Choose a reason for hiding this comment

rix0rrr Jul 8, 2022

Choose a reason for hiding this comment

rix0rrr Jul 8, 2022

Choose a reason for hiding this comment

TheRealAmazonKendra commented Sep 9, 2022

mergify bot commented Sep 9, 2022

✅ Branch has been successfully updated

mergify bot commented Sep 9, 2022

TheRealAmazonKendra commented Sep 9, 2022

mergify bot commented Sep 9, 2022

☑️ Nothing to do

aws-cdk-automation commented Sep 9, 2022

AWS CodeBuild CI Report

mergify bot commented Sep 9, 2022

TheRealAmazonKendra commented Jul 8, 2022 •

edited

Loading