Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[core] AssetStaging by bundling not using custom assetHash when doing quick 'skip' hash #11459

Closed
mrgrain opened this issue Nov 13, 2020 · 1 comment · Fixed by #11440
Closed

[core] AssetStaging by bundling not using custom assetHash when doing quick 'skip' hash #11459

mrgrain opened this issue Nov 13, 2020 · 1 comment · Fixed by #11440
Assignees
@rix0rrr
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. in-progress This issue is being actively worked on. needs-triage This issue or PR still needs to be triaged.

Comments

@mrgrain
Copy link
Contributor

mrgrain commented Nov 13, 2020 •
edited
Loading

AssetStaging provides a shortcut when using custom bundling interface which allows to skip the normal hashing method. Instead it falls back to AssetHashType.SOURCE with a comment implying it would be quicker. This code path seems to be only invoked when cdk list is called, but I might have misunderstood the condition.

Code Reference:

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

Lines 240 to 248 in 21ccfce

private stageByBundling(bundling: BundlingOptions, skip: boolean): StagedAsset {
if (skip) {
// We should have bundled, but didn't to save time. Still pretend to have a hash,
// but always base it on sources.
return {
assetHash: this.calculateHash(AssetHashType.SOURCE),
stagedPath: this.sourcePath,
};
}

This shortcut is also taken when a custom assetHash is provided and assetHashType set to CUSTOM.
Besides from calculating a different hash, fingerprinting source files will always be slower than using a pre-computed hash.

Reproduction Steps

Apologies for not providing a code sample at this point. But it can be recreated with a simple setup:

  • Add a S3.Asset to a stack pointing to a large folder (e.g. one that contains a lot of node_modules)
  • Provide a custom assetHash and type
  • Run time cdk synth, then run time cdk ls

What did you expect to happen?

cdk ls should be quicker or at leat taking the same time as cdk synth.

What actually happened?

cdk ls takes significantly longer than cdk synth, since calculating the assetHash from a large source takes much longer than using a pre-provided hash.

Environment

  • CDK CLI Version: 1.73.0
  • Framework Version: 1.73.0
  • Node.js Version: v12.18.0
  • OS : macOS 10.15.7 (19H2)
  • Language (Version): TypeScript 4.0.5

Other

This is related to the sibling issue: #11460 : [core] AssetStaging by bundling ignoring AssetHashType.OUTPUT when doing quick 'skip' hash

I'm happy to work on this issue, it seems very straight forward. However as pointed out in the other issue there might be a more generic or better solution to it.

This is 🐛 Bug Report
@mrgrain mrgrain added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Nov 13, 2020
@mrgrain mrgrain mentioned this issue Nov 13, 2020
Closed
@jogold jogold mentioned this issue Nov 13, 2020
Merged
@SomayaB SomayaB added the @aws-cdk/core Related to core CDK functionality label Nov 13, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Nov 26, 2020
@mergify mergify bot closed this as completed in #11440 Dec 1, 2020
mergify bot pushed a commit that referenced this issue Dec 1, 2020
@jogold
fix(core): regression: source directory is fingerprinted even if bund…
3cbc7fa 
…ling is skipped (#11440)

If the asset uses OUTPUT or BUNDLE it's generally because its source is
a very large directory. This is the case for the `NodejsFunction` which
mounts the project root (along with the `node_modules` folder!).

Use a custom hash in this case when skipping bundling. Otherwise running
`cdk ls` can result in heavy fingerprinting operations (again this is
the case for the `NodejsFunction`) and can be much slower than running
`cdk synth` or `cdk diff`, making it pointless to skip bundling.

Regression introduced in #11008
(https://github.com/aws/aws-cdk/pull/11008/files#diff-62eef996be8abeb157518522c3cbf84a33dd4751c103304df87b04eb6d7bbab6L160)

Before #11008:
https://github.com/aws/aws-cdk/blob/c1453147e7c484c54f1380d2fa9ba0cdf2859002/packages/%40aws-cdk/core/lib/asset-staging.ts#L159-L160

Closes #11459 
Closes #11460 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Dec 1, 2020

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rix0rrr rix0rrr

Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. in-progress This issue is being actively worked on. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(core): regression: source directory is fingerprinted even if bundling is skipped jogold/aws-cdk
3 participants
@mrgrain @rix0rrr @SomayaB