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

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

Merged
merged 1 commit into from
Jul 13, 2020

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jul 13, 2020

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.

@jogold @misterjoshua please revisit.

This reverts commit 31d6e65.


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

@eladb eladb changed the title Revert "chore(core): allow the bundler to re-use pre-existing bundler… Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916) Jul 13, 2020
@eladb eladb changed the title Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916) "Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916) Jul 13, 2020
@eladb eladb changed the title "Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916) Revert "chore(core): allow the bundler to re-use pre-existing bundler output (#8916)" Jul 13, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 13, 2020
@mergify
Copy link
Contributor

mergify bot commented Jul 13, 2020

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

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Jul 13, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure that 'Revert' is not picked up by our bump script into our changelog.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@jogold
Copy link
Contributor

jogold commented Jul 13, 2020

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.

Agree, but it still covers the case described in @misterjoshua's issue? Re-using assets during a synth/deploy not between synths/deploys (shown to be working in the following test https://github.com/aws/aws-cdk/pull/8916/files#diff-f22402543bd7a255f41d5bf7d711a835R152)

This is a problem for me when assets are slow to bundle and when there are several stacks (say, one per deployment environment) in a single app.

@eladb eladb merged commit 2cc46d5 into master Jul 13, 2020
@eladb eladb deleted the benisrae/revert-8916 branch July 13, 2020 14:58
@eladb
Copy link
Contributor Author

eladb commented Jul 13, 2020

Agree, but it still covers the case described in @misterjoshua's issue? Re-using assets during a synth/deploy not between synths/deploys (shown to be working in the following test https://github.com/aws/aws-cdk/pull/8916/files#diff-f22402543bd7a255f41d5bf7d711a835R152)

Okay, I am now realizing that I missed some context. We are in the process of completely deprecating prepare() and synthesize(). As part of this effort, I've changed the way asset staging works such that it will stage the asset during initialization.

@eladb
Copy link
Contributor Author

eladb commented Jul 13, 2020

This means that this optimization no longer holds because the fs.moveSync happens before the constructor finishes.

@jogold
Copy link
Contributor

jogold commented Jul 13, 2020

OK, this is #8962 I presume? Will have look.

@eladb
Copy link
Contributor Author

eladb commented Jul 13, 2020

Yes.

curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this pull request Aug 11, 2020
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants