feat(cli): added `build` field to cdk.json #17176

comcalvi · 2021-10-27T00:08:05Z

Adds a build field to cdk.json. The command specified in the build will be executed before synthesis. This can be used to build any code that needs to be built before synthesis (for example, CDK App code or Lambda Function code).

This is part of the changes needed for the cdk watch command
(aws/aws-cdk-rfcs#383).

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

gitpod-io · 2021-10-27T00:08:07Z

skinny85

Looks good! Some minor details.

Can you also add a PR description stating what is this change accomplishing, and why is it being added?

packages/aws-cdk/README.md

packages/aws-cdk/lib/api/cxapp/exec.ts

packages/aws-cdk/test/api/exec.test.ts

packages/aws-cdk/lib/api/cxapp/exec.ts

skinny85 · 2021-10-27T00:22:12Z

@eladb can you also take a look? Thanks!

packages/aws-cdk/README.md

packages/aws-cdk/test/api/exec.test.ts

eladb · 2021-10-28T11:18:42Z

packages/aws-cdk/lib/api/cxapp/exec.ts


+  const build = config.settings.get(['build']);
+  if (build) {
+    await exec([build]);


What happens if "build" is set to something like "mvn package"? How does this split into program and args?

exec() now splits its argument into command and args before spawning the process.

Seems like exec() treats the first item in the array as the command but this is not true in the case I described above.

Can you add a test to verify ?

You're right, the split doesn't happen in the case of the build key. It winds up working because the whole command just get put into command and spawn() happily accepts it.

eladb · 2021-11-02T06:44:28Z

packages/aws-cdk/lib/api/cxapp/exec.ts

      //   user gets to see it sooner. Plus, capturing doesn't interact nicely with some
      //   processes like Maven.
-      const proc = childProcess.spawn(commandLine[0], commandLine.slice(1), {
+      const command = commandAndArgs[0];


What happens if commandAndArgs[0] is mvn package

I think we just need to accept a single string and pass it down to spawn(). Since you use shell:true this should just work without splitting to arguments imho

We need to split the arguments because of the other usage of exec(), namely this line:

await exec(commandLine);

commandLine is defined by

const commandLine = await guessExecutable(appToArray(app));

guessExecutable() needs a string[], not a string. Do you want me to rework this so that we don't need to have any string[]s passed to exec(), and instead make exec() take just a string? exec() previously operated on a string[].

@eladb if commandAndArgs[0] is mvn package then spawn() will still correctly start the process.

Seems a bit messy but if this works as is i am okay with that

@eladb just made this cleaner, now mvn package will be split into ["mvn", "package"] instead of the previous ["mvn package"]. Would appreciate another pass on the PR!

eladb · 2021-11-03T19:10:17Z

packages/aws-cdk/lib/api/cxapp/exec.ts

  }

-  const commandLine = await guessExecutable(appToArray(app));
+  const commandLine = await guessExecutable(commandToArray(app));


Not sure this is better TBH...

What happens if the command has quotes in it?

For example "mvn package"?

Bottom line, I think we should simply spawn this command without splitting it into arguments and with shell:true. There's a variant of spawn I believe that just accepts a single string and passes it to the shell.

There are also intricacies related to Windows/POSIX here that can blow up in 5,000 ways, so I rather we avoid any parsing of the command line if possible.

@eladb just to be clear - you also want to change how we handle the "app" key, right? And no longer do any splitting there?

skinny85

One small comment.

packages/aws-cdk/lib/api/cxapp/exec.ts

eladb

Looks good

packages/aws-cdk/lib/api/cxapp/exec.ts

mergify · 2021-11-04T16:28:25Z

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

aws-cdk-automation · 2021-11-04T19:14:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: f64725f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-11-04T19:15:09Z

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

Adds a `build` field to `cdk.json`. The command specified in the `build` will be executed before synthesis. This can be used to build any code that needs to be built before synthesis (for example, CDK App code or Lambda Function code). This is part of the changes needed for the `cdk watch` command (aws/aws-cdk-rfcs#383). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

comcalvi added 2 commits October 26, 2021 17:02

added support for the field in cdk.json

f675f56

updated README

62b813d

comcalvi requested a review from skinny85 October 27, 2021 00:08

github-actions bot assigned rix0rrr Oct 27, 2021

github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Oct 27, 2021

comcalvi changed the title ~~feat (cli): added build field to cdk.json~~ feat(cli): added build field to cdk.json Oct 27, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 27, 2021

skinny85 suggested changes Oct 27, 2021

View reviewed changes

skinny85 assigned comcalvi and unassigned rix0rrr Oct 27, 2021

skinny85 requested a review from eladb October 27, 2021 00:22

github-actions bot assigned rix0rrr Oct 27, 2021

addressed PR review

dad551a

comcalvi requested a review from skinny85 October 27, 2021 01:13

comcalvi unassigned rix0rrr Oct 27, 2021

github-actions bot assigned rix0rrr Oct 27, 2021

comcalvi unassigned rix0rrr Oct 27, 2021

removed blank line

52099e1

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Oct 28, 2021

eladb suggested changes Oct 28, 2021

View reviewed changes

comcalvi added 3 commits October 28, 2021 10:27

updated test

697b016

build key cannot be specified in user config

f9fda40

update readme

7263fc5

comcalvi requested a review from eladb November 1, 2021 16:16

skinny85 mentioned this pull request Nov 1, 2021

feat(cli): introduce the 'watch' command #17240

Merged

eladb suggested changes Nov 2, 2021

View reviewed changes

comcalvi added 2 commits November 3, 2021 11:33

string is now split on whitespace

ec29717

updated docs

bee321a

comcalvi requested a review from eladb November 3, 2021 18:47

eladb suggested changes Nov 3, 2021

View reviewed changes

comcalvi added 8 commits November 3, 2021 14:08

reworked exec to take a single string

55c348a

exec and spawn now deal with a single string, instead of a string[]

6716482

removed unused prefix property from interface

d0691cc

Merge branch 'master' of https://github.com/aws/aws-cdk into build

3ec1110

removed commented code

44d49f8

removed uneeded []

3949ae6

updated arrayToString() to be cleaner

1be1185

added comment to arrayToString()

d0c0be6

skinny85 reviewed Nov 3, 2021

View reviewed changes

packages/aws-cdk/lib/api/cxapp/exec.ts Outdated Show resolved Hide resolved

removed unndeeded function, added Array.join()

eef80b9

eladb approved these changes Nov 4, 2021

View reviewed changes

skinny85 reviewed Nov 4, 2021

View reviewed changes

packages/aws-cdk/lib/api/cxapp/exec.ts Show resolved Hide resolved

skinny85 approved these changes Nov 4, 2021

View reviewed changes

packages/aws-cdk/lib/api/cxapp/exec.ts Show resolved Hide resolved

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Nov 4, 2021

Merge branch 'master' into build

f64725f

mergify bot merged commit 57ad1e0 into aws:master Nov 4, 2021

skinny85 mentioned this pull request Feb 18, 2022

(hotswap, watch): skip synthesis if the change is only to an Asset aws/aws-cdk-cli#884

Open

2 tasks

edwardaskew mentioned this pull request Nov 30, 2022

aws-cdk: cdk synth/deploy command fails with "C:\Program" is not recognized as an internal or external command aws/aws-cdk-cli#636

Open

decompil3d mentioned this pull request Feb 3, 2023

fix(cli): spawning node fails when node path contains spaces #24001

Closed

tmokmss mentioned this pull request Nov 30, 2024

cli: Respect --build option in cdk.json as well #32329

Closed

2 tasks

feat(cli): added build field to cdk.json #17176

feat(cli): added build field to cdk.json #17176

Uh oh!

Conversation

comcalvi commented Oct 27, 2021 • edited by skinny85 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Oct 27, 2021

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Oct 27, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Nov 4, 2021

Uh oh!

aws-cdk-automation commented Nov 4, 2021

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Nov 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat(cli): added `build` field to cdk.json #17176

feat(cli): added `build` field to cdk.json #17176

comcalvi commented Oct 27, 2021 •

edited by skinny85

Loading