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

Add backwards-compatible validation for RawModuleDefV8. #1606

Merged
merged 2 commits into from
Aug 29, 2024

Conversation

kazimuth
Copy link
Contributor

Description of Changes

Plus unit and integration tests.
I'm still working through errors in the integration tests -- I think I'm going to need to something slightly more clever in a few places -- but generally this is ready for review.

API and ABI breaking changes

N/A

Expected complexity level and risk

This is tricky because we're adding a new code path that's supposed to replicate the behavior of another complex code path, but I think once we add the other end (ModuleDef -> TableSchema) comparing the results will be enough to validate that stuff works.

Testing

I test on both synthetic v8 module definitions and actual module definitions from the repo.

@kazimuth kazimuth requested a review from cloutiertyler August 16, 2024 21:35
@bfops bfops added the release-any To be landed in any release window label Aug 19, 2024
@kazimuth kazimuth force-pushed the jgilles/validation branch from 2dba1cf to 058f3d7 Compare August 23, 2024 18:28
@kazimuth kazimuth force-pushed the jgilles/v8validation branch 4 times, most recently from c8465d8 to af07662 Compare August 23, 2024 21:02
@kazimuth kazimuth force-pushed the jgilles/validation branch from e1afd15 to f695d0d Compare August 23, 2024 22:13
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from af07662 to de74b8d Compare August 23, 2024 22:21
@kazimuth kazimuth requested a review from Centril August 23, 2024 23:07
@kazimuth
Copy link
Contributor Author

@Centril @cloutiertyler I consider this ready for merge.

@kazimuth kazimuth force-pushed the jgilles/v8validation branch 2 times, most recently from ab49537 to a8e90e1 Compare August 26, 2024 18:03
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from a8e90e1 to 06a9e70 Compare August 27, 2024 19:00
Copy link
Contributor

@cloutiertyler cloutiertyler left a comment

Choose a reason for hiding this comment

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

In principle this all seems fine to me.

@kazimuth kazimuth changed the base branch from jgilles/validation to master August 28, 2024 16:26
Whoops, that didn't need to be there

Placate clippy

Paragraph comments

Break tests, then fix them

Address review comments + fix clippy

Update to deal with accessor_name

Fix import

Update for removed structural_ty

Add lifecycle validation

Validate scheduled reducers, whoops
@kazimuth kazimuth force-pushed the jgilles/v8validation branch from 3ad5756 to 8834292 Compare August 28, 2024 16:30
@kazimuth kazimuth added this pull request to the merge queue Aug 29, 2024
Merged via the queue into master with commit 98faf7d Aug 29, 2024
8 checks passed
@kazimuth kazimuth deleted the jgilles/v8validation branch August 29, 2024 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants