chore: add "gen" target as a pre build script #10648

NetaNir · 2020-10-02T05:13:50Z

What's new?

Added .scripts/gen.sh - execute this to execute gen all modules, for cfn modules this will usually execute cfn2ts. This also means that you can use it to generate all required files to build monocdk:

./scripts/gen.sh
lerna build --scope monocdk

cdk-build will now execute the command specified in the package script.gen directive.
cdk-build.pre - The commands in the pre array are now concatenated using &&.

This PR includes changes to many files the majority of them are package.json files. Summary of changes:

Added a gen as an npm script target to all cfn modules. The gen target is already used in our repo as a convention for executing pre build command for code generation. This change extends it to include executing cfn2ts. Prior to this change we were executing the pre commands in cdk-build, followed by executing cfn2ts on cfn modules. cdk-build now execute gen unless --skip-gen is passed.
All code generation command (non cfn2ts) were moved from pre in cdk-build to gen. The only remaining pre directives are those unrelated to code generation such as cleaning commands.
Why?
This will allow us to execute lerna run --stream gen to execute all required code generation scripts on all modules.

The cfn2ts script already includes the ability to auto detect the CloudFormation scope from cdk-build clause in the package.json:

aws-cdk/tools/cfn2ts/bin/cfn2ts.ts

Lines 43 to 46 in bc096f0

    
           async function tryAutoDetectScope(pkg: any): Promise<undefined | string[]> { 
        
             const value = pkg['cdk-build'] && pkg['cdk-build'].cloudformation; 
        
             return value && (typeof value === 'string' ? [value] : value); 
        
           }

which makes it redundant to execute cfn2ts --scope=scope .

Since the gen directive is now required in all cfn module, it was added to cfnspec so it will be added to every new module.
Added a gen.sh script in scripts, executing it will build cfn2ts and then execute lerna gen. This is useful if one wants to only get the .generated files without running a full build.

This PR originally included the changes require to split the monocdk build, these were removed from this PR to make it easier to review. The monocdk build related changes will be submitted in a different PR.

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

NetaNir · 2020-10-02T05:25:36Z

packages/@aws-cdk/core/package.json

  },
  "cdk-build": {
    "cloudformation": "AWS::CloudFormation",
    "cfn2ts-core-import": ".",
    "pre": [
-      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz"
+      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz && npm run gen"


@rix0rrr since pre is an array I assumed I can simply add npm run gen as a separate item in the array:

"pre": [ "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz", "npm run gen" ]

This does not work in cdk-build as the array of commands is concatenated without an &&:

rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz npm run gen

Which fails. Is this a bug?

Oh yes, that sounds like a big bad bug.

So, this is by design, we might still want to change it, but first let me describe the current API. The shell function accepts an array of strings, and assumes that commands[0] includes the actual command, and the rest of the elements in the array are the args to that command:

aws-cdk/packages/aws-cdk/lib/os.ts

Lines 17 to 20 in 37fdc94

const child = child_process.spawn(command[0], command.slice(1), {

...options,

stdio: ['ignore', 'pipe', 'inherit'],

});

This is leveraged in several locations in the code, for example, in cdk-watch:

aws-cdk/tools/cdk-build-tools/bin/cdk-watch.ts

Line 26 in 37fdc94

await shell(packageCompiler({ jsii: args.jsii, tsc: args.tsc }).concat(['-w']));

Usage:

aws-cdk/tools/cdk-build-tools/lib/package-info.ts

Lines 84 to 90 in 37fdc94

export function packageCompiler(compilers: CompilerOverrides): string[] {

if (isJsii()) {

return [compilers.jsii || require.resolve('jsii/bin/jsii'), '--silence-warnings=reserved-word'];

} else {

return [compilers.tsc || require.resolve('typescript/bin/tsc'), '--build'];

}

}

It feels unintuitive, but maybe it is? I think we can keep shell as is, but change pre to accept a string instead of an array. Call for suggestions!

aws-cdk/tools/cdk-build-tools/lib/os.ts

Lines 22 to 24 in d412bb6

const child = child_process.spawn(command[0], command.slice(1), {

// Need this for Windows where we want .cmd and .bat to be found as well.

shell: true,

I see that the implementation of shell is a little weird, probably because Node.js is just slightly different from other programming languages that have similar interfaces. Normally it's:

Run a command through the shell, argument should be a string (string can contain special shell characters like &, > and they will have their usual shell meaning)

Run a command directly, argument should be an array (shell is not involved, &, > and will just be interpreted as characters in a file name).

However, in inimitable "I'll try to do the best I can" Node.js fashion, the underlying child_process.spawn function takes an array yet runs it through the shell, and it's fine if the whole string is in command[0]. I guess in an attempt to be broadly compatible, it just won't escape command[0] but it will escape command[1..]. Something we need to verify.

I consider that flavor of API to be a feature! It's much easier to get wrong by accidentally having shell metacharacters
interpreted, or having to quote arguments (maybe there are filenames with spaces), etc. If I can, I will always design
my subprocess API to take an array instead of a string. You can still get the original behavior by calling
subprocess(['bash', '-c', originalCommandStringWithSpecialCharacters]), so it's not a loss of functionality.
It's a pity it's so confusing here.

In any case, knowing what we're knowing, and even leaving the API of the shell function unchanged, we can change the API exposed via the package.json file easily. If we detect that package['cdk-build'].pre is an array, we can either run multiple shells, or just join everything together with a && before we run it through the shell.

aws-cdk/tools/cdk-build-tools/bin/cdk-build.ts

Line 33 in d412bb6

await shell(options.pre, { timers, env });

if (Array.isArray(options.pre)) { for (const cmd of options.pre) { await shell(cmd, { timers, env }); } } else { await shell(options.pre, { timers, env }); }

rix0rrr

I didn't see the change to cfn2ts in here?

(Which is great, because I don't want it anyway :) )

rix0rrr · 2020-10-02T07:54:33Z

packages/@aws-cdk/alexa-ask/package.json

@@ -50,11 +50,15 @@
    "cfn2ts": "cfn2ts",
    "build+test+package": "npm run build+test && npm run package",
    "build+test": "npm run build && npm test",
-    "compat": "cdk-compat"
+    "compat": "cdk-compat",
+    "gen": "cfn2ts"


Why don't we put the arguments to cfn2ts here as well? Let's make this

Suggested change

"gen": "cfn2ts"

"gen": "cfn2ts --scope=Alexa::Ask"

etc.

We don't need it, cfn2ts already gets this from the pacakge.json file:

aws-cdk/tools/cfn2ts/bin/cfn2ts.ts

Lines 43 to 46 in bc096f0

async function tryAutoDetectScope(pkg: any): Promise<undefined | string[]> {

const value = pkg['cdk-build'] && pkg['cdk-build'].cloudformation;

return value && (typeof value === 'string' ? [value] : value);

}

I know, but it gets it from the cdk-build section. Don't you agree that's kinda confusing because we're not using cdk-build to invoke cfn2ts anymore?

In any case, fine.

I agree it's confusing, as evident by having to read through cfn2ts code to understand why calling it without the scope name even works. I think we can add it to the gen directive for clarity, not sure about removing the logic from cfn2ts, it is a private package so technically we wont be breaking anyone.

rix0rrr · 2020-10-02T07:54:55Z

packages/@aws-cdk/alexa-ask/package.json

  },
  "cdk-build": {
    "cloudformation": "Alexa::ASK",
    "jest": true,
+    "pre": [


This feels unnecessary. We can just code knowledge of the gen target into cdk-build, and get rid of cdk-build.pre altogether.

(Yes, that means moving everything that's currently in cdk-build.pre into scripts.gen. If you don't want to that, I'm okay with leaving cdk-build.pre in, but I'd still prefer that the tool knows about genning so we don't have to repeat this in every package file).

Good call. I removed npm run gen from all pre directive. cdk-build will now execute gen unless --skip-gen was provided.

I moved all code generation logic to gen, this include code generation commands in non cfn modules as well. I didn't remove pre completely since there are cases in which it is used for things like cleaning folders, which are not code generation related.

packages/@aws-cdk/cfnspec/build-tools/create-missing-libraries.ts

rix0rrr · 2020-10-02T07:57:25Z

packages/@aws-cdk/core/package.json

  },
  "cdk-build": {
    "cloudformation": "AWS::CloudFormation",
    "cfn2ts-core-import": ".",
    "pre": [
-      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz"
+      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz && npm run gen"


Oh yes, that sounds like a big bad bug.

NetaNir · 2020-10-11T22:17:03Z

packages/@aws-cdk/core/package.json

  },
  "cdk-build": {
    "cloudformation": "AWS::CloudFormation",
    "cfn2ts-core-import": ".",
    "pre": [
-      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz"
+      "rm -rf test/fs/fixtures && cd test/fs && tar -xzf fixtures.tar.gz && npm run gen"


So, this is by design, we might still want to change it, but first let me describe the current API. The shell function accepts an array of strings, and assumes that commands[0] includes the actual command, and the rest of the elements in the array are the args to that command:

aws-cdk/packages/aws-cdk/lib/os.ts

Lines 17 to 20 in 37fdc94

const child = child_process.spawn(command[0], command.slice(1), {

...options,

stdio: ['ignore', 'pipe', 'inherit'],

});

This is leveraged in several locations in the code, for example, in cdk-watch:

aws-cdk/tools/cdk-build-tools/bin/cdk-watch.ts

Line 26 in 37fdc94

await shell(packageCompiler({ jsii: args.jsii, tsc: args.tsc }).concat(['-w']));

Usage:

aws-cdk/tools/cdk-build-tools/lib/package-info.ts

Lines 84 to 90 in 37fdc94

export function packageCompiler(compilers: CompilerOverrides): string[] {

if (isJsii()) {

return [compilers.jsii || require.resolve('jsii/bin/jsii'), '--silence-warnings=reserved-word'];

} else {

return [compilers.tsc || require.resolve('typescript/bin/tsc'), '--build'];

}

}

It feels unintuitive, but maybe it is? I think we can keep shell as is, but change pre to accept a string instead of an array. Call for suggestions!

NetaNir · 2020-10-12T00:58:30Z

tools/cdk-build-tools/bin/cdk-build.ts

@@ -24,22 +24,22 @@ async function main() {
      desc: 'Specify a different eslint executable',
      defaultDescription: 'eslint provided by node dependencies',
    })
+    .option('skip-gen', {


Note to self(and to the interested reviewer): NEVER EVER use no-something for a boolean argument when using yargs, it will be interrupted as something=false.

Yep. Every boolean argument you can pass like this on the command line:

$ command --flag $ command --no-flag

So if you call your option gen and default it to true, you can call it with:

$ cdk-build --no-gen

Which seems excellent.

I guess it was only a surprise to me :) If that's the case I'll change it to gen

NetaNir · 2020-10-12T01:08:00Z

build.sh

+if ! [ -x "$(command -v yarn)" ]; then
+  echo "yarn is not installed. Install it from here- https://yarnpkg.com/en/docs/install."
+  exit 1
+fi
+


Hoisted this check so it will happen before executing yarn install

rix0rrr · 2020-10-14T07:21:23Z

packages/@monocdk-experiment/assert/clone.sh

@@ -13,4 +13,5 @@ for file in ${files}; do
  cp $src/$file .
 done

-npx rewrite-imports "**/*.ts"
+NAME=${1:-}
+npx rewrite-imports "**/*.ts" ${NAME}


Probably could have done "$@" here?

rix0rrr · 2020-10-14T07:22:39Z

tools/cdk-build-tools/lib/package-info.ts

+ * Return the command defined in scripts.gen if exists
+ */
+export function genScript(): string | undefined {
+  return currentPackageJson().scripts.gen;


Suggested change

return currentPackageJson().scripts.gen;

return currentPackageJson().scripts?.gen;

Not likely but possible that scripts is empty.

rix0rrr

Excited about this change. Provisional approval to give you a chance to consider some final tweaks, other than that: ship it!

* cdk-build.pre now support array of commands

mergify · 2020-10-17T18:03:44Z

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).

gitpod-io · 2020-10-17T18:04:14Z

mergify · 2020-10-17T19:02:10Z

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 · 2020-10-17T19:02:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e6c684d
Result: SUCCEEDED
Build Logs (available for 30 days)

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

chore: add "gen" target as a pre build script

89b3ef7

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

fix typo

54f5362

NetaNir commented Oct 2, 2020

View reviewed changes

revert rules

9712bfa

NetaNir requested review from rix0rrr and RomainMuller October 2, 2020 05:31

NetaNir added 2 commits October 1, 2020 22:49

Merge branch 'master' into neta/monocdk-build

16bbec7

appflow was missing gen

f3d75ee

rix0rrr requested changes Oct 2, 2020

View reviewed changes

SomayaB assigned NetaNir Oct 2, 2020

NetaNir added 2 commits October 3, 2020 20:44

Merge branch 'master' into neta/monocdk-build

5799458

draft

056eee7

NetaNir marked this pull request as draft October 5, 2020 05:37

NetaNir added 6 commits October 4, 2020 23:01

missing file

f259487

fix build

51d3a24

cleanup

02a6a66

Merge branch 'master' into neta/monocdk-build

3e0d2d6

add mono-build

794b996

use gen

ff2f119

NetaNir commented Oct 12, 2020

View reviewed changes

NetaNir added 2 commits October 11, 2020 19:34

remove monobuild changes

8af1c35

added new line

dfcb714

NetaNir requested a review from rix0rrr October 12, 2020 02:48

NetaNir added 3 commits October 11, 2020 19:59

fixup

c8a944b

fixx diff

0f737ce

new line

8e1cfc3

NetaNir marked this pull request as ready for review October 12, 2020 03:05

Merge branch 'master' into neta/monocdk-build

3f5b4bf

rix0rrr reviewed Oct 14, 2020

View reviewed changes

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Oct 14, 2020

rix0rrr approved these changes Oct 14, 2020

View reviewed changes

NetaNir added 2 commits October 17, 2020 10:06

* add 'no-gen'

cf245a4

* cdk-build.pre now support array of commands

move rewrite gen to pre build

5a717f0

NetaNir removed the pr/do-not-merge This PR should not be merged at this time. label Oct 17, 2020

Merge branch 'master' into neta/monocdk-build

8980c3e

add gen to new modules

e6c684d

mergify bot merged commit d943d26 into master Oct 17, 2020

mergify bot deleted the neta/monocdk-build branch October 17, 2020 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: add "gen" target as a pre build script #10648

chore: add "gen" target as a pre build script #10648

NetaNir commented Oct 2, 2020 •

edited

Loading

NetaNir Oct 2, 2020 •

edited

Loading

rix0rrr Oct 2, 2020

NetaNir Oct 11, 2020

rix0rrr Oct 14, 2020

rix0rrr left a comment

rix0rrr Oct 2, 2020

NetaNir Oct 2, 2020 •

edited

Loading

rix0rrr Oct 14, 2020

NetaNir Oct 15, 2020

rix0rrr Oct 2, 2020

NetaNir Oct 12, 2020

rix0rrr Oct 2, 2020

NetaNir Oct 11, 2020

NetaNir Oct 12, 2020

rix0rrr Oct 14, 2020

NetaNir Oct 15, 2020

NetaNir Oct 12, 2020

rix0rrr Oct 14, 2020

rix0rrr Oct 14, 2020

rix0rrr left a comment

mergify bot commented Oct 17, 2020

gitpod-io bot commented Oct 17, 2020 •

edited

Loading

mergify bot commented Oct 17, 2020

aws-cdk-automation commented Oct 17, 2020

	async function tryAutoDetectScope(pkg: any): Promise<undefined \| string[]> {
	const value = pkg['cdk-build'] && pkg['cdk-build'].cloudformation;
	return value && (typeof value === 'string' ? [value] : value);
	}

	const child = child_process.spawn(command[0], command.slice(1), {
	...options,
	stdio: ['ignore', 'pipe', 'inherit'],
	});

	export function packageCompiler(compilers: CompilerOverrides): string[] {
	if (isJsii()) {
	return [compilers.jsii \|\| require.resolve('jsii/bin/jsii'), '--silence-warnings=reserved-word'];
	} else {
	return [compilers.tsc \|\| require.resolve('typescript/bin/tsc'), '--build'];
	}
	}

	const child = child_process.spawn(command[0], command.slice(1), {
	// Need this for Windows where we want .cmd and .bat to be found as well.
	shell: true,

	return currentPackageJson().scripts.gen;
	return currentPackageJson().scripts?.gen;

chore: add "gen" target as a pre build script #10648

chore: add "gen" target as a pre build script #10648

Conversation

NetaNir commented Oct 2, 2020 • edited Loading

What's new?

NetaNir Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NetaNir Oct 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 17, 2020

gitpod-io bot commented Oct 17, 2020 • edited Loading

mergify bot commented Oct 17, 2020

aws-cdk-automation commented Oct 17, 2020

AWS CodeBuild CI Report

NetaNir commented Oct 2, 2020 •

edited

Loading

NetaNir Oct 2, 2020 •

edited

Loading

NetaNir Oct 2, 2020 •

edited

Loading

gitpod-io bot commented Oct 17, 2020 •

edited

Loading