fix(cli): large context causes E2BIG error during synthesis on Linux #21373

otaviomacedo · 2022-07-29T11:03:30Z

Linux systems don't support environment variables larger than 128KiB. This change splits the context into two if it's larger than that and stores the overflow into a temporary file. The application then re-constructs the original context from these two sources.

A special case is when this new version of the CLI is used to synthesize an application that depends on an old version of the framework. The application will still consume part of the context, but the CLI warns the user that some of it has been lost.

Since the tree manipulation logic is basically the same as the one used for displaying notices, it was extracted into its own file.

Re-roll #21230
Fixes #19261

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Instead of passing the context in an environment variable, the CLI now writes the context to a temporary file and sets an environment variable only with the location. The app then uses that location to read from the file.

Also tested manually on a Linux machine.

Re-roll #21230
Fixes #19261

Instead of passing the context in an environment variable, the CLI now writes the context to a temporary file and sets an environment variable only with the location. The app then uses that location to read from the file. Also tested manually on a Linux machine. Re-roll #21230 Fixes #19261

gitpod-io · 2022-07-29T11:03:38Z

comcalvi

just one small comment. This is pretty much perfect.

comcalvi · 2022-08-04T22:05:22Z

packages/aws-cdk/lib/tree.ts

+  readonly constructInfo?: ConstructInfo;
+}
+
+export function some(node: ConstructTreeNode, predicate: (n: ConstructTreeNode) => boolean): boolean {


could we get a docstring for this one?

rix0rrr · 2022-08-08T07:48:41Z

packages/aws-cdk/lib/api/cxapp/exec.ts

 import { versionNumber } from '../../version';
 import { SdkProvider } from '../aws-auth';

+const ENV_VARIABLE_SIZE_LIMIT = 131072; // 128KiB


Linux systems don't support environment variables larger than 128KiB.

We also have to think about Windows. In Windows, the maximum size of the WHOLE environment together is 32KiB. We can do some calculation based on the current environment to come up with a max for the context value, or do a rough estimation and say we'll cap it at 16KiB or something.

Good call out!

rix0rrr

I see you've fully switched to the lower of the 2 bounds.

That makes sense, but this will now have the downside that large context setups (anywhere between 32 and 128KiB) and that used to work fine on Linux now stop working in the case of a new CLI and an old framework (which I'm pretty sure will be a large segment of the population).

I think there's not going to be much of a way around having 2 different limits, one for *NIX-based systems and one for Windows, and deciding between them based on the platform we're running on.

Naumel · 2022-08-31T09:53:36Z

@Mergifyio update

mergify · 2022-08-31T09:54:07Z

update

✅ Branch has been successfully updated

Stale

mergify · 2022-09-22T14:58:58Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions

The Pull Request Linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/21373/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

PRs must pass status checks before we can provide a meaningful review.

Pull Request updated. Dissmissing previous PRLinter Review.

mergify · 2022-09-26T07:59:08Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

TheRealAmazonKendra · 2022-10-05T02:13:36Z

@Mergifyio update

mergify · 2022-10-05T02:15:07Z

update

✅ Branch has been successfully updated

aws-cdk-automation · 2022-10-05T02:58:30Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 61fb148
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-10-05T02:58:39Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…ws#21373) Linux systems don't support environment variables larger than 128KiB. This change splits the context into two if it's larger than that and stores the overflow into a temporary file. The application then re-constructs the original context from these two sources. A special case is when this new version of the CLI is used to synthesize an application that depends on an old version of the framework. The application will still consume part of the context, but the CLI warns the user that some of it has been lost. Since the tree manipulation logic is basically the same as the one used for displaying notices, it was extracted into its own file. Re-roll aws#21230 Fixes aws#19261 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Instead of passing the context in an environment variable, the CLI now writes the context to a temporary file and sets an environment variable only with the location. The app then uses that location to read from the file. Also tested manually on a Linux machine. Re-roll aws#21230 Fixes aws#19261

otaviomacedo added 4 commits July 27, 2022 15:25

Overflow

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

def5d35

Context cleanup and refactoring

709a588

Clean up

3f51589

aws-cdk-automation requested a review from a team July 29, 2022 11:03

github-actions bot added bug effort/medium p1 labels Jul 29, 2022

mergify bot added the contribution/core label Jul 29, 2022

otaviomacedo added 2 commits July 29, 2022 12:04

Added comment

5c476f6

Updated latest unsupported version to 2.35.0

e2ab756

comcalvi previously requested changes Aug 4, 2022

View reviewed changes

Added docstring and tests

894e1ca

otaviomacedo requested a review from comcalvi August 5, 2022 10:22

rix0rrr requested changes Aug 8, 2022

View reviewed changes

Taking into account the Windows constraint

b2cdb63

otaviomacedo requested a review from rix0rrr August 8, 2022 09:34

otaviomacedo added 4 commits August 9, 2022 14:19

Updated latest unsupported version

1c5ee51

Version comparison is dynamic

ee5b90c

Back to fixed version

8aefca2

Updated latest unsupported version

4118c16

rix0rrr requested changes Aug 12, 2022

View reviewed changes

Space available varies according to the OS

c97a2f7

otaviomacedo requested a review from rix0rrr August 17, 2022 15:15

Merge branch 'main' into otaviom/context-temp-file

fe186a5

rix0rrr approved these changes Sep 22, 2022

View reviewed changes

Merge branch 'main' into otaviom/context-temp-file

c1dcba9

github-actions bot previously requested changes Sep 22, 2022

View reviewed changes

rix0rrr added pr-linter/exempt-integ-test pr-linter/cli-integ-tested labels Sep 26, 2022

Merge branch 'main' into otaviom/context-temp-file

5a60a6b

Merge branch 'main' into otaviom/context-temp-file

61fb148

mergify bot merged commit 7040168 into main Oct 5, 2022

mergify bot deleted the otaviom/context-temp-file branch October 5, 2022 02:58

mrgrain mentioned this pull request Jan 10, 2025

(cli): context overflow calculations are not correct #32833

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): large context causes E2BIG error during synthesis on Linux #21373

fix(cli): large context causes E2BIG error during synthesis on Linux #21373

otaviomacedo commented Jul 29, 2022

gitpod-io bot commented Jul 29, 2022

comcalvi left a comment

comcalvi Aug 4, 2022

rix0rrr Aug 8, 2022

otaviomacedo Aug 8, 2022

rix0rrr left a comment •

edited

Loading

Naumel commented Aug 31, 2022

mergify bot commented Aug 31, 2022

mergify bot commented Sep 22, 2022

github-actions bot left a comment

mergify bot commented Sep 26, 2022

TheRealAmazonKendra commented Oct 5, 2022

mergify bot commented Oct 5, 2022

aws-cdk-automation commented Oct 5, 2022

mergify bot commented Oct 5, 2022

fix(cli): large context causes E2BIG error during synthesis on Linux #21373

fix(cli): large context causes E2BIG error during synthesis on Linux #21373

Conversation

otaviomacedo commented Jul 29, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Jul 29, 2022

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Aug 4, 2022

Choose a reason for hiding this comment

rix0rrr Aug 8, 2022

Choose a reason for hiding this comment

otaviomacedo Aug 8, 2022

Choose a reason for hiding this comment

rix0rrr left a comment • edited Loading

Choose a reason for hiding this comment

Naumel commented Aug 31, 2022

mergify bot commented Aug 31, 2022

✅ Branch has been successfully updated

mergify bot commented Sep 22, 2022

github-actions bot left a comment

Choose a reason for hiding this comment

mergify bot commented Sep 26, 2022

TheRealAmazonKendra commented Oct 5, 2022

mergify bot commented Oct 5, 2022

✅ Branch has been successfully updated

aws-cdk-automation commented Oct 5, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 5, 2022

rix0rrr left a comment •

edited

Loading