-
Notifications
You must be signed in to change notification settings - Fork 245
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
fix(rosetta): extract does not respect strict metadata entry #2863
Conversation
I opted for a simpler fix than described in the issue: just changed the "strict brand" from a symbol to a string with a "jsii" prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you summarize the issue and the fix in the PR description?
Would be nice to get that into the git commit message with the ability to read directly on the IDE/console, rather than have to click into a Github issue.
There's a ton of material you'll find on the internet on the value of good commit messages. Here's one that's standard token - https://medium.com/swlh/why-should-i-write-good-commit-messages-e15d37bf45cb.
Change look good to me.
Added 'do-not-merge' in case @RomainMuller wants to take another look.
@Mergifyio refresh |
Command
|
@Mergifyio refresh |
Command
|
Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it! |
Merging (with squash)... |
@RomainMuller for some reason, the jsii-pacmak test:build is failing with error:
I don't understand why this would suddenly be a problem and why other builds are succeeding. The go.sum in question does in fact have entries for Masterminds/semver/v3. I've run the tests locally in the superchain image and there aren't any problems. |
The build problem is apparently caused by a bug in |
Merging (with squash)... |
A bugfix in jsii-rosetta (aws/jsii#2863) now enforces the `jsii.rosetta.strict` jsii assembly metadata entry, meaning modules with `jsii.metadata.jsii.rosetta.strict` set to `true` in their `package.json` will need their snippets to compile or builds will fail. Because this entry was not respected previously, two modules (`core` and `aws-iam`) have snippets that fail to compile. This means that future builds will start failing once the changes to jsii are released. Further, jsii integration tests are failing due to this breakage. This change simply disables strict compilation for these modules since fixed snippets may be overwritten before the jsii changes are released, causing future builds to fail. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
A bugfix in jsii-rosetta (aws/jsii#2863) now enforces the `jsii.rosetta.strict` jsii assembly metadata entry, meaning modules with `jsii.metadata.jsii.rosetta.strict` set to `true` in their `package.json` will need their snippets to compile or builds will fail. Because this entry was not respected previously, two modules (`core` and `aws-iam`) have snippets that fail to compile. This means that future builds will start failing once the changes to jsii are released. Further, jsii integration tests are failing due to this breakage. This change simply disables strict compilation for these modules since fixed snippets may be overwritten before the jsii changes are released, causing future builds to fail. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
The
jsii.rosetta.strict
assembly metadata entry was not being respected by theextract
command due to two bugs:worker_threads
node module involves sending compiler diagnostics across the wire. These diagnostics should be annotated with a "strict brand" that marks them as failing compilation and allows further steps to fail if such a diagnostic exist. Because the brand was a Symbol, it is not correctly serialized. Fix: make the brand a string so it can be properly serialized.fixes #2861
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.