chore(core): allow the bundler to re-use pre-existing bundler output #8916

misterjoshua · 2020-07-06T22:54:00Z

This PR changes AssetStaging so that the bundler will re-use pre-existing output. Before, the bundler would re-run Docker without considering pre-existing assets, which was slow. Now, when handling a SOURCE hash type, the bundler detects and returns pre-existing asset output without re-running Docker. For all other hash types, the bundler outputs to a temp directory before calculating asset hashes.

Closes #8882

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

mergify · 2020-07-06T22:54:34Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

jogold

Looks good

packages/@aws-cdk/core/lib/asset-staging.ts

packages/@aws-cdk/core/test/test.staging.ts

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

misterjoshua · 2020-07-07T17:13:22Z

@jogold After your review, I was thinking about the failure modes and looking closely at the throws when bundling fails test. I think we need to remove the bundleDir if the bundler fails. Otherwise, subsequent runs may falsely believe that the bundleDir can be re-used. I'll push up a commit that does as much in a few minutes.

jogold · 2020-07-08T08:13:21Z

One more thing that comes to mind: what if the BundlingOptions are updated? I would expect the asset not be re-used if I change the command for example.

Shouldn't we hash those options together with the custom or source hash and use this final hash in the bundleDir name?

cc @eladb

eladb · 2020-07-08T12:08:04Z

Shouldn't we hash those options together with the custom or source hash and use this final hash in the bundleDir name?

Makes sense

packages/@aws-cdk/core/lib/asset-staging.ts

eladb · 2020-07-08T12:11:45Z

packages/@aws-cdk/core/lib/asset-staging.ts

@@ -179,6 +200,7 @@ export class AssetStaging extends Construct {
        workingDirectory: options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR,
      });
    } catch (err) {
+      fs.removeSync(bundleDir);


This is unrelated to this PR, right?
I am also not sure we actually want to delete the bundleDir for diagnosability (I would even include it in the error message).

It is, see #8916 (comment)

Otherwise, subsequent runs may falsely believe that the bundleDir can be re-used.

Rename it in case of failure instead (e.g. append -error)?

Rename it in case of failure instead (e.g. append -error)?

This seems good to me. But, if we go this route, we might consider when to remove -error directories if they already exist, such as if docker fails twice for the same bundleDir.

Alternatively, is there a unique number or code I can access for each CDK run? If there were, I could append that number and the error output can be held indefinitely for diagnosability.

For now I've implemented the -error suffix. I'm happy to circle back around.

packages/@aws-cdk/core/lib/asset-staging.ts

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

misterjoshua · 2020-07-08T17:36:40Z

@jogold Any idea why the test throws with assetHash and not CUSTOM hash type expects Docker to have been invoked even though it's an error state? What am I missing? Given that the test seems to be to check for a configuration error, the code I've committed assumes this docker run isn't necessary. However, I can circle around to handle this nuance if needed.

jogold · 2020-07-08T20:11:30Z

@jogold Any idea why the test throws with assetHash and not CUSTOM hash type expects Docker to have been invoked even though it's an error state? What am I missing? Given that the test seems to be to check for a configuration error, the code I've committed assumes this docker run isn't necessary. However, I can circle around to handle this nuance if needed.

It passed because hash calculation always occurred after bundling, it can be safely removed from the expectations. Looks like it was wrongly introduced here #8481.

packages/@aws-cdk/core/test/test.staging.ts

jogold

@misterjoshua 👍

To fix the build error you can run the integ tests in s3-assets and aws-lambda (the failing tests will be integ.assets.bundling.lit.js and integ.bundling.js respectively) or merge from master (see #8969).

@eladb LGTM

We should maybe add a note about this new "behavior" here?

aws-cdk/packages/@aws-cdk/core/lib/assets.ts

Lines 35 to 44 in 0a3ae47

    
             /** 
        
              * Specifies the type of hash to calculate for this asset. 
        
              * 
        
              * If `assetHash` is configured, this option must be `undefined` or 
        
              * `AssetHashType.CUSTOM`. 
        
              * 
        
              * @default - the default is `AssetHashType.SOURCE`, but if `assetHash` is 
        
              * explicitly specified this value defaults to `AssetHashType.CUSTOM`. 
        
              */ 
        
             readonly assetHashType?: AssetHashType;

misterjoshua · 2020-07-09T17:21:24Z

To fix the build error you can run the integ tests in s3-assets and aws-lambda (the failing tests will be integ.assets.bundling.lit.js and integ.bundling.js respectively) or merge from master (see #8969).

@jogold I merged from master but the integration tests still failed. I looked at #8969 and added the new pragma to the top of each offending test file to enable the feature. I butchered my commit message, but in any case, those tests seem to work now. I hope that was the right move.

mergify · 2020-07-13T11:30:54Z

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-07-13T11:52:19Z

AWS CodeBuild CI Report

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

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

mergify · 2020-07-13T11:52:40Z

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

… output (#8916)" This reverts commit 31d6e65.

eladb · 2020-07-13T14:36:13Z

Hey @misterjoshua @jogold, we are reverting this PR through #9037 - please revisit.

… output (#8916)" (#9037) This logic to reuse bundler output is faulty. Once an asset is bundled, the bundle directory is moved into the cloud assembly (cdk.out) and therefore the logic that attempts to reuse the bundle directory only materializes in the case where moveSync decides to copy the directory and not just move it. This reverts commit 31d6e65.

jogold · 2020-07-13T15:12:06Z

@misterjoshua see #9037 (comment)

I suggest waiting for #8962 before revisiting this.

Actually I think it will be easier to implement if the assembly dir is know at construction time.

jogold · 2020-08-10T09:49:25Z

@misterjoshua do you want to give this another try? We can now do everything in cdk.out, no need for .cdk.staging.

Let me know.

misterjoshua · 2020-08-10T19:44:59Z

@misterjoshua do you want to give this another try? We can now do everything in cdk.out, no need for .cdk.staging.

Let me know.

Hey @jogold. Thanks for letting me know. I'll open a new PR and mention you in it after I've made some progress.

…ws#8916) This PR changes `AssetStaging` so that the bundler will re-use pre-existing output. Before, the bundler would re-run Docker without considering pre-existing assets, which was slow. Now, when handling a `SOURCE` hash type, the bundler detects and returns pre-existing asset output without re-running Docker. For all other hash types, the bundler outputs to a temp directory before calculating asset hashes. Closes aws#8882 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

… output (aws#8916)" (aws#9037) This logic to reuse bundler output is faulty. Once an asset is bundled, the bundle directory is moved into the cloud assembly (cdk.out) and therefore the logic that attempts to reuse the bundle directory only materializes in the case where moveSync decides to copy the directory and not just move it. This reverts commit 31d6e65.

…(revisited) (#9576) This PR changes `AssetStaging` so that the bundler will re-use pre-existing output. Before, the bundler would re-run Docker without considering pre-existing assets, which was slow. Now, when handling a `SOURCE` hash type, the bundler detects and returns pre-existing asset output without re-running Docker. For all other hash types, the bundler outputs to an intermediate directory before calculating asset hashes, then renames the intermediate directory into its final location. This PR revisits #8916 which originally closed #8882. Here are some details from the previous PR which have been addressed in this PR: - The bundler now outputs directly into the assembly directory - The bundler's assets can be reused between multiple syntheses - The bundler keeps output from failed bundling attempts for diagnosability purposes (renamed with an `-error` suffix) - Bundler options are hashed together with custom and source hashes - Removed the check for a docker run from `throws with assetHash and not CUSTOM hash type` as docker is no longer run before the AssetStaging props are validated. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Allow the bundler to re-use assets aws#8882

cee16cf

misterjoshua changed the title ~~feature(core): allow the bundler to re-use pre-existing bundler output~~ feat(core): allow the bundler to re-use pre-existing bundler output Jul 6, 2020

Resolve path for known-assetHash bundler staging directories

583353c

misterjoshua changed the title ~~feat(core): allow the bundler to re-use pre-existing bundler output~~ chore(core): allow the bundler to re-use pre-existing bundler output Jul 6, 2020

Merge branch 'master' into bundling-asset-reuse

9fc2f58

jogold reviewed Jul 7, 2020

View reviewed changes

SomayaB assigned eladb Jul 7, 2020

misterjoshua and others added 2 commits July 7, 2020 10:18

Apply suggestions from code review

eff0e6e

Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>

Undo the change to 'throws when bundling fails' test

e958eb8

Make the bundler remove its bundleDir if bundling fails

62f47a3

eladb reviewed Jul 8, 2020

View reviewed changes

Accept rename of assetHashIfKnownInAdvance to sourceHash

17a3cc2

Co-authored-by: Elad Ben-Israel <benisrae@amazon.com>

misterjoshua and others added 4 commits July 8, 2020 13:08

Add comment explaining what assetHashIfKnownInAdvance is all about

6798588

Implement review suggestions

f5c4fd8

Merge branch 'master' into bundling-asset-reuse

e2da8b5

Preserve failed bundling by renaming bundleDirs to an -error suffix

fb416a3

Consolidate my bundling-failure assertions, fix a typo

ad98555

jogold reviewed Jul 9, 2020

View reviewed changes

packages/@aws-cdk/core/test/test.staging.ts Show resolved Hide resolved

Fix synthesis targetPath issue

8a67806

jogold approved these changes Jul 9, 2020

View reviewed changes

misterjoshua and others added 3 commits July 9, 2020 09:53

Merge branch 'master' into bundling-asset-reuse

ad5fe03

Add aws#8969's /// cdk init app -linteg pragma:ignore-assets

bc9dd56

Merge branch 'master' into bundling-asset-reuse

56dc8d5

Resolve conflict

31edc7a

eladb approved these changes Jul 13, 2020

View reviewed changes

Merge branch 'master' into bundling-asset-reuse

7a5bc94

mergify bot merged commit 31d6e65 into aws:master Jul 13, 2020

eladb pushed a commit that referenced this pull request Jul 13, 2020

Revert "chore(core): allow the bundler to re-use pre-existing bundler…

c418fc9

… output (#8916)" This reverts commit 31d6e65.

eladb mentioned this pull request Jul 13, 2020

Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916)" #9037

Merged

misterjoshua mentioned this pull request Aug 10, 2020

chore(core): allow the bundler to re-use pre-existing bundler output (revisited) #9576

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(core): allow the bundler to re-use pre-existing bundler output #8916

chore(core): allow the bundler to re-use pre-existing bundler output #8916

misterjoshua commented Jul 6, 2020 •

edited

Loading

mergify bot commented Jul 6, 2020

jogold left a comment

misterjoshua commented Jul 7, 2020 •

edited

Loading

jogold commented Jul 8, 2020 •

edited

Loading

eladb commented Jul 8, 2020

eladb Jul 8, 2020

jogold Jul 8, 2020

misterjoshua Jul 8, 2020 •

edited

Loading

misterjoshua Jul 8, 2020

misterjoshua commented Jul 8, 2020 •

edited

Loading

jogold commented Jul 8, 2020

jogold left a comment

misterjoshua commented Jul 9, 2020

mergify bot commented Jul 13, 2020

aws-cdk-automation commented Jul 13, 2020

mergify bot commented Jul 13, 2020

eladb commented Jul 13, 2020

jogold commented Jul 13, 2020

jogold commented Aug 10, 2020

misterjoshua commented Aug 10, 2020

	/**
	* Specifies the type of hash to calculate for this asset.
	*
	* If `assetHash` is configured, this option must be `undefined` or
	* `AssetHashType.CUSTOM`.
	*
	* @default - the default is `AssetHashType.SOURCE`, but if `assetHash` is
	* explicitly specified this value defaults to `AssetHashType.CUSTOM`.
	*/
	readonly assetHashType?: AssetHashType;

chore(core): allow the bundler to re-use pre-existing bundler output #8916

chore(core): allow the bundler to re-use pre-existing bundler output #8916

Conversation

misterjoshua commented Jul 6, 2020 • edited Loading

mergify bot commented Jul 6, 2020

jogold left a comment

Choose a reason for hiding this comment

misterjoshua commented Jul 7, 2020 • edited Loading

jogold commented Jul 8, 2020 • edited Loading

eladb commented Jul 8, 2020

eladb Jul 8, 2020

Choose a reason for hiding this comment

jogold Jul 8, 2020

Choose a reason for hiding this comment

misterjoshua Jul 8, 2020 • edited Loading

Choose a reason for hiding this comment

misterjoshua Jul 8, 2020

Choose a reason for hiding this comment

misterjoshua commented Jul 8, 2020 • edited Loading

jogold commented Jul 8, 2020

jogold left a comment

Choose a reason for hiding this comment

misterjoshua commented Jul 9, 2020

mergify bot commented Jul 13, 2020

aws-cdk-automation commented Jul 13, 2020

AWS CodeBuild CI Report

mergify bot commented Jul 13, 2020

eladb commented Jul 13, 2020

jogold commented Jul 13, 2020

jogold commented Aug 10, 2020

misterjoshua commented Aug 10, 2020

misterjoshua commented Jul 6, 2020 •

edited

Loading

misterjoshua commented Jul 7, 2020 •

edited

Loading

jogold commented Jul 8, 2020 •

edited

Loading

misterjoshua Jul 8, 2020 •

edited

Loading

misterjoshua commented Jul 8, 2020 •

edited

Loading