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

feat(rosetta): support "strict" assemblies #2253

Merged
merged 9 commits into from
Nov 23, 2020
Merged

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Nov 16, 2020

A "strict" assembly carries a special metadata entry
(jsii.rosetta.strict = true) that instructs jsii-rosetta to always
handle that as if --compile and --fail were provided. This allows
opting specific modules from a mono-repository into "enforced compiling"
operation while retaining the ability to run jsii-rosetta only once
from the root of the repository. Concretely this means one can validate
examples from a single package are valid by simply running jsii-rosetta
(strict mode will imply --compile --fail), while a continuous integration
pipeline can continue to use jsii-rosetta on a whole mono-repo in a single
command (which is typically a lot faster).

Also added a --strict option to jsii-rosetta in order to have
consistent semantics here. And added a sub-command of jsii-rosetta
to actually set the metadata entry correctly for the user.


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

A "strict" assembly carries a special metadata entry
(`jsii.rosetta.strict = true`) that instructs `jsii-rosetta` to always
handle that as if `--compile` and `--fail` were provided. This allows
opting specific modules from a mono-repository into "enforced compiling"
operation while retaining the ability to run `jsii-rosetta` only once
from the root of the repository.
@RomainMuller RomainMuller added effort/small Small work item – less than a day of effort contribution/core This is a PR that came from AWS. labels Nov 16, 2020
@RomainMuller RomainMuller requested a review from a team November 16, 2020 15:48
@RomainMuller RomainMuller self-assigned this Nov 16, 2020
@nija-at
Copy link
Contributor

nija-at commented Nov 23, 2020

This allows opting specific modules from a mono-repository into "enforced compiling"
operation while retaining the ability to run jsii-rosetta only once
from the root of the repository.

Can't seem to follow what you're saying here. Can you re-state simply?

Do you mean the samples will always be compiled as part of jsii?

@RomainMuller
Copy link
Contributor Author

This allows opting specific modules from a mono-repository into "enforced compiling"
operation while retaining the ability to run jsii-rosetta only once
from the root of the repository.

Can't seem to follow what you're saying here. Can you re-state simply?

Do you mean the samples will always be compiled as part of jsii?

I've attempted rephrasing (actually added more text).
We still don't run Rosetta as part of jsii, no...

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

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Small changes, but overall okay.

I'd like to implore you to see if this can be arranged differently from pushing down the strict parameter all the way into the bowels of the code (can't remember offhand what a better place would be to make the cut though).

If not, then I suppose this needs to be it...

packages/jsii-rosetta/bin/jsii-rosetta.ts Outdated Show resolved Hide resolved
packages/jsii-rosetta/bin/jsii-rosetta.ts Outdated Show resolved Hide resolved
packages/jsii-rosetta/bin/jsii-rosetta.ts Outdated Show resolved Hide resolved
packages/jsii-rosetta/bin/jsii-rosetta.ts Show resolved Hide resolved
packages/jsii-rosetta/lib/commands/convert.ts Outdated Show resolved Hide resolved
packages/jsii-rosetta/lib/translate.ts Show resolved Hide resolved
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.

Couple of questions that aren't clear from the code -

If the sample uses a package/code that is not declared as a dependency of this package (either because it's a downstream dependency or a utility package that a customer can choose to use), how does the --strict option check this?

Concretely this means one can validate
examples from a single package are valid by simply running jsii-rosetta
(strict mode will imply --compile --fail), while a continuous integration
pipeline can continue to use jsii-rosetta on a whole mono-repo in a single
command (which is typically a lot faster).

If I'm interepreting this correctly, the workflow you're suggesting will be that developers can choose to run jsii-rosetta on the individual package (in a monorepo) during dev time but it won't be run as part of CI. Instead, the CI system will run it in a single command for the whole monorepo. Did I get that correctly?
Will the single command also interpret the jsii.rosetta.strict and fail for packages that have this flag set and don't compile?

packages/jsii-rosetta/README.md Outdated Show resolved Hide resolved
const snippet = typeScriptSnippetFromSource(
example,
'example',
enforcesStrictMode(this.assembly),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you chosen for the DocGenerator classes to now depend on the jsii assembly? Is this intentional?

Can be avoided if the enforceStrictAssembly is called at the layer above. This will also avoid code duplication across the different language targets.

Copy link
Contributor Author

@RomainMuller RomainMuller Nov 23, 2020

Choose a reason for hiding this comment

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

jsii-pacmak right now uses a single Rosetta instance for all packages it generates (there could be multiple from --recurse). Some may be strict, some may be "loose", and I don't want a single strict package to cause failures in "loose" packages...

Creating multiple instances of Rosetta means creating multiple TypeScript contexts, which I decided against.

@RomainMuller
Copy link
Contributor Author

If the sample uses a package/code that is not declared as a dependency of this package (either because it's a downstream dependency or a utility package that a customer can choose to use), how does the --strict option check this?

Not certain I understand the question well, but here's some answers nevertheless:

  • The strict enabling metadata is recorded on the jsii Assembly. Whenever jsii-rosetta translates samples from a repository with that metadata, it will be compiled, and jsii-rosetta will fail on compilation errors (as opposed to failing only on attempting to transliterate things it does not support)
  • Any dependencies required in order for the compilation to succeed must be present and available at the time jsii-rosetta is run... This in particular is why it's not possible to perform the "compile check" at time of package compilation - the code examples may depend on packages that are not yet built... Consider the case of @aws-cdk/core - it's examples heavily use other libraries such as @aws-cdk/aws-lambda, etc... because they are otherwise not really helpful. As a consequence, a "local iteration workflow" would be something along the lines of:
    1. Compile current package & any package needed by code samples... (This could mean compiling a whole monorepo, to simplify)
    2. Edit the documentation you're looking to iterate on
    3. Re-compile (run jsii on the package where edits were made)
    4. Run jsii-rosetta to validate code samples are all compiling
    5. Goto step 2 ad libitum (until you're happy about the result)
    6. Make pull request

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-5lHf64IXfvmr
  • Commit ID: e805655
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@RomainMuller
Copy link
Contributor Author

@Mergifyio refresh

@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2020

Command refresh: success

Pull request refreshed

@RomainMuller RomainMuller removed the pr/do-not-merge This PR should not be merged at this time. label Nov 23, 2020
@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2020

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Nov 23, 2020
@mergify mergify bot merged commit 6cbde78 into main Nov 23, 2020
@mergify mergify bot deleted the rmuller/rosetta-strict branch November 23, 2020 16:33
@mergify
Copy link
Contributor

mergify bot commented Nov 23, 2020

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Nov 23, 2020
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. effort/small Small work item – less than a day of effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants