Skip to content

Conversation

@robin-aws
Copy link
Contributor

@robin-aws robin-aws commented Mar 6, 2024

Description of changes:

Replaces nearly all of the SharedMakefile.mk with the common smithy-dafny/SmithyDafnyMakefile.mk makefile, just retaining configuration variables specific to this repo (such as the path to the smithy-dafny submodule). Uses the new features in that makefile and smithy-dafny itself to make the projects forwards-compatible with the latest Dafny nightly prerelease, and hence will fix the nightly build once merged - see here for a manual run.

Highlights of the changes:

  • Apply the same workflow changes as chore: Forwards compatibility with Dafny > 4.2 (including pending 4.5) aws-cryptographic-material-providers-library#195 to use smithy-dafny to regenerate code, either to check that the output matches what's checked in (in a new separate codegen workflow) or to be compatible with newer versions of Dafny in the nightly build (in existing workflows).
    • In this case we also have to locally update the MPL submodule to the latest, so that we can pick up the forwards-compatible changes to that repo, and regenerate code transitively.
  • Because the code in this repo wasn't formatted already, but applying newer smithy-dafny code generation automatically formats, all the generated code has trivial layout changes.
  • Also extracted a manual patch.
  • Applied a lot of explicit client downcasting to account for the change in smithy-dafny

Note that this changes the Makefile targets as a result:

  • There is a default value for CODEGEN_CLI_ROOT (the smithy-dafny submodule copy) so that variable is now optional
  • Targets like polymorph_codegen also now apply to dependencies transitively, just like targets such as transpile_java. Because the current state of the MPL submodule doesn't use the same smithy-dafny version or Makefile targets, this doesn't work by default, so for working locally it will generally be better to only generate for the current library via make polymorph_codegen PROJECT_DEPENDENCIES=

Open questions for reviewer:

  1. Is bumping the AWSSDK.Core version slightly to make this work allowed? Or is there a more conservative way to deal with the version conflict you get when using the head of MPL?
  2. Transitively regenerating code for this repo AND the MPL is quite slow, up to 10 mins. This is partially because the Makefile build targets aren't sophisticated enough to avoid rebuilding libraries in the graph. Since this is only in CI in the nightly build this doesn't seem unreasonable for now, but any concerns about the development workflow?

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

@robin-aws robin-aws marked this pull request as ready for review March 11, 2024 22:23
@robin-aws robin-aws requested a review from a team as a code owner March 11, 2024 22:23
Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

I think that the biggest changes are in the C# proj file.
We need to make sure that local development and published artifacts are both easy and safe.

Co-authored-by: seebees <ryanemer@amazon.com>
Copy link
Contributor

@seebees seebees left a comment

Choose a reason for hiding this comment

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

LKTM

@robin-aws robin-aws merged commit cd19979 into mainline Mar 18, 2024
rishav-karanjit added a commit that referenced this pull request May 9, 2024
## 4.1.0

### Notes
- [(#646)](10daadf) Enforces input constraints.

Prior to this fix, the AWS Encryption SDK in .NET (ESDK-NET) failed to enforce user input constraints. Input shapes without required members set would always result in a `NullReferenceException`.
Now, the ESDK-NET will throw its own Exceptions when illegal user input
is submitted.

### Fixes

* fix: throw an exception when MemoryStream instance has an empty backing array [(#633)](550c714)

### Features

* feat: enforce input constraints [(#646)](10daadf)

### Maintenance

* fix(CI): Daily CI uses correct workflow [(#641)](771835e)
* chore(ci): fix role to assume [(#622)](c1f04fc)
* chore(CI/CD): add semantic release automation [(#647)](e7b5392)
* chore: Adopt SmithyDafnyMakefile.mk, fix nightly build [(#638)](cd19979)
* chore(CI): add interop tests to daily ci [(#640)](c9ad018)
* chore: only run net48 on windows and use node 17 to run integration-node [(#639)](d6c62fb)
* chore(.NET): Add ESDK-Net v4.0.1 generated vectors[(#636)](efef497)
* chore(NET-SupportPolicy): Mark 3.x as Support [(#631)](3c36f7a)
* chore: Add manual trigger for nightly_dafny.yml [(#629)](419b1cb)
* chore: split vc gen on some methods to migrate to Dafny 4.4 [(#627)](fdc65ca)
* test: restore CODEOWNERS and daily CI [(#624)](ff823ac)
* chore: update template to point to public repo [(#626)](2b07a39)
* chore: remove unused release step in test-prod [(#623)](9883933)
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.

3 participants