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.

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

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

Merged
merged 35 commits into from
Aug 26, 2020

Conversation

misterjoshua
Copy link
Contributor

@misterjoshua misterjoshua commented Aug 10, 2020

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

@mergify
Copy link
Contributor

mergify bot commented Aug 10, 2020

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

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 10, 2020

@jogold Here's that PR for the progress I promised this afternoon. It probably looks familiar in a lot of ways. :)

@misterjoshua misterjoshua changed the title chore(core): allow the bundler to re-use pre-existing bundler output (v2) chore(core): allow the bundler to re-use pre-existing bundler output (revisited) Aug 10, 2020
@adamdottv
Copy link
Contributor

@misterjoshua I think this will close #9406? cc @jogold

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 11, 2020

@misterjoshua I think this will close #9406? cc @jogold

This PR will help to speed up the creation of separate assets and lambda layers that might be bundled with pip install, but I don't think this PR will close #9406 outright. We need some way to separate the assets. Otherwise, any small changes to the lambda's code will change the source hash and re-signal a complete pip install.

After this PR is merged, I imagine that it would be possible to change PythonFunction so that it does pip install in a separate lambda layer if it detects the requirements.txt. But, we should attack this in a different PR.

@jogold
Copy link
Contributor

jogold commented Aug 12, 2020

This looks good! Can you merge the latest changes from master? Local bundling has been added.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 12, 2020

@jogold I've merged changes from master and resolved a small conflict. CI is green.

I'm looking over the /// !cdk-integ pragma:ignore-assets stuff right now - these pragmas aren't sitting well with me. I'm going to see about updating the expectations in these files rather than using the pragma.

@misterjoshua
Copy link
Contributor Author

misterjoshua commented Aug 12, 2020

@jogold I've merged changes from master and resolved a small conflict. CI is green.

I'm looking over the /// !cdk-integ pragma:ignore-assets stuff right now - these pragmas aren't sitting well with me. I'm going to see about updating the expectations in these files rather than using the pragma.

It looks like there's interesting behaviour surrounding /// !cdk-integ pragma:ignore-assets in integ.assets.bundling.lit.ts. The asset hashes are unstable in this test. The image is being built on-demand, so timestamps and the Docker layer cache are in play for the resulting Image ID. I'm working on a solution to this problem now.

@misterjoshua misterjoshua marked this pull request as ready for review August 12, 2020 22:54
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/asset-staging.ts Outdated Show resolved Hide resolved
misterjoshua and others added 6 commits August 13, 2020 09:44
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
Co-authored-by: Jonathan Goldwasser <jogold@users.noreply.github.com>
@mergify mergify bot dismissed eladb’s stale review August 19, 2020 20:14

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2020

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

@mergify mergify bot merged commit f5c9124 into aws:master Aug 26, 2020
@misterjoshua
Copy link
Contributor Author

@eladb I see that in master, this PR's associated commit has a red X for failing build. Is there anything I need to do to help?

@BryanPan342
Copy link
Contributor

seems like the the error is in aws-logs.. i have a PR that's failing after merging master too

@BryanPan342
Copy link
Contributor

specifically

lib/log-retention-provider/index.ts:6:10 - error TS2305: Module '"../../node_modules/aws-sdk/lib/config"' has no exported member 'RetryDelayOptions'.

@misterjoshua
Copy link
Contributor Author

specifically

lib/log-retention-provider/index.ts:6:10 - error TS2305: Module '"../../node_modules/aws-sdk/lib/config"' has no exported member 'RetryDelayOptions'.

@BryanPan342 I see that as well, but while building locally - this is new. I'm not sure how this PR could have impacted that. AWS SDK just released a new version, however, and TypeScript types were referenced. I wonder if this is related?

bugfix: TypeScript type definitions: Deep imports of clients no longer reference entire SDK type definitions

@BryanPan342
Copy link
Contributor

#9994 i updated the dependency it seems to work locally now but we shall see what happens with code-build runs it

@BryanPan342
Copy link
Contributor

ah @misterjoshua i think ur right

https://github.com/aws/aws-sdk-js/blob/9bd7fdbda9929d4f259996d468de80e210a4ce78/lib/config-base.d.ts

they moved the RetryDelayOptions to config-base and had the config class extend it but im assuming the versioning must messed up somewhere

jogold added a commit to jogold/aws-cdk that referenced this pull request Sep 24, 2020
The change introduced in aws#9576 did not handle the "staging disabled"
case. As a consequence, when bundling the staged path was always
relative.

Revert to the behavior that was present before this change: when staging
is disabled the staged path is absolute (whether bundling or not).

Closes aws#10367
jogold added a commit to jogold/aws-cdk that referenced this pull request Sep 24, 2020
The change introduced in aws#9576 did not handle the "staging disabled"
case. As a consequence, when bundling the staged path was always
relative.

Revert to the behavior that was present before this change: when staging
is disabled the staged path is absolute (whether bundling or not).

Closes aws#10367
mergify bot pushed a commit that referenced this pull request Sep 24, 2020
…0507)

The change introduced in #9576 did not handle the "staging disabled"
case. As a consequence, when bundling the staged path was always
relative.

Revert to the behavior that was present before this change: when staging
is disabled the staged path is absolute (whether bundling or not).

Closes #10367


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] make the asset bundler able to re-use assets
6 participants