Skip to content

chore: Forwards compatibility with Dafny > 4.2 (including pending 4.5)#195

Merged
robin-aws merged 128 commits into
mainfrom
robin-aws/fix-nightly-dafny-build
Mar 1, 2024
Merged

chore: Forwards compatibility with Dafny > 4.2 (including pending 4.5)#195
robin-aws merged 128 commits into
mainfrom
robin-aws/fix-nightly-dafny-build

Conversation

@robin-aws
Copy link
Copy Markdown
Contributor

@robin-aws robin-aws commented Feb 3, 2024

Description of changes:

Applies several fixes/improvements in order to work with newer Dafny versions. This PR is currently using an unmerged branch of smithy-dafny to test the fixes - smithy-lang/smithy-dafny#321 needs to be merged before this one.

  • Adds smithy-dafny as a submodule so that we can lock down the exact commit used to generate code, and use the tool in CI.
  • Updates the shared makefile with similar improvements to smithy-dafny's (hook up --library-root and --patch-files-dir, generate dependencies first)
  • Regenerates the checked-in code using a newer smithy-dafny to abstract away from the changes in Java TypeDescriptors when constructing datatypes in Dafny 4.3. This includes adding some helper methods to Dafny code for the benefit of Java external code, in the same style as feat: add type descriptors for data constructors in Java, when necessary smithy-lang/smithy-dafny#301 did.
  • Leverage the smithy-dafny <library-root>/codegen-patches[/<service>] feature to extract out manual patch files.
    • This allows building with a newer version of Dafny to work despite having to regenerate code differently, in some cases by providing different patch files for different version ranges.
    • Cheated slightly by using this to conditionally remove an instance of {:vcs_split_on_every_assert} on AwsKmsKeyring.OnDecrypt' that is necessary on Dafny 4.2 but makes things work on Dafny 4.4 (which was not the intention of the patching feature, but also solves this problem much more cheaply than having to refactor the code to work in both versions :)
  • Add regenerating code to CI, either to verify that it matches what's checked in, or to pick up the necessary changes to work with newer Dafny versions.

Manually verified the CI passes on the source branch with the latest Dafny nightly prerelease: https://github.com/aws/aws-cryptographic-material-providers-library/actions/runs/8039889665

Note that CI will now reject making further manual edits to generated files without capturing those edits in patch files. The error message will suggest how to update the patch files accordingly. See also https://github.com/smithy-lang/smithy-dafny/tree/main-1.x/TestModels/CodegenPatches

Squash/merge commit message, if applicable:

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

For better documentation and CI efficiency
@robin-aws
Copy link
Copy Markdown
Contributor Author

After offline conversation, it seems wise to factor out the code generation steps somehow, so that there can be a large comment in one spot explaining why this is necessary when testing on other Dafny versions. This looks doable with a local composite action.

Closing this PR temporarily while I get that working though, to avoid rerunning PR CI constantly while I'm pushing to this branch and manually testing manual.yml.

@robin-aws robin-aws closed this Feb 28, 2024
@robin-aws robin-aws reopened this Feb 29, 2024
@robin-aws
Copy link
Copy Markdown
Contributor Author

Fresh manual run on the refactored workflows against nightly-latest Dafny: https://github.com/aws/aws-cryptographic-material-providers-library/actions/runs/8088855919

Copy link
Copy Markdown
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.

LGTM,
thanks for the CI consolidation

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