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

Explicitly check mir-json schema version #2228

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

RyanGlScott
Copy link
Contributor

This ensures that SAW is only ever loading MIR JSON files that are known to be supported. (See also GaloisInc/crucible#1309 for similar changes on the Crux-MIR side.)

Fixes #2111.

This bumps the following submodules:

Copy link
Contributor

@sauclovian-g sauclovian-g left a comment

Choose a reason for hiding this comment

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

LGTM (so far, not sure what else might be needed)

It might have been worth splitting the commit up, but I don't think it's worth retconning it now.

@RyanGlScott
Copy link
Contributor Author

How would you propose splitting up the commit, out of curiosity?

@sauclovian-g
Copy link
Contributor

I'd probably separate out all the mir-json updates, probably do them first since they don't actually depend on the rest. One might also split the submodule bumps from the rest for visibility; there was some talk about that last summer.

This regenerates all version-controlled MIR JSON files to use `mir-json` commit
GaloisInc/mir-json@c52b16b,
which adds a schema version to the JSON. For now, SAW simply ignores these
version numbers, but a subsequent commit will update SAW to explicitly check
for them.
This ensures that SAW is only ever loading MIR JSON files that are known to be
supported. (See also GaloisInc/crucible#1309 for
similar changes on the Crux-MIR side.)

Fixes #2111.

This bumps the following submodules:

* `crucible` (bringing in the changes from GaloisInc/crucible#1309)
* `mir-json` (bringing in the changes from GaloisInc/mir-json#75)
This documents the expectations surrounding SAW's requirements for `mir-json`
schema versions in the `README`. It also advertises the fact that we check for
schema versions in the changelog.
@RyanGlScott RyanGlScott force-pushed the crucible-T1253-mir-json-schema-version branch from 18c51de to 05d7c89 Compare February 20, 2025 16:06
@RyanGlScott RyanGlScott marked this pull request as ready for review February 20, 2025 16:06
@RyanGlScott
Copy link
Contributor Author

I've force-pushed a new version of the PR that splits the changes into three separate commits.

@RyanGlScott RyanGlScott merged commit d8e2b6d into master Feb 20, 2025
34 checks passed
@RyanGlScott RyanGlScott deleted the crucible-T1253-mir-json-schema-version branch February 20, 2025 18:55
@sauclovian-g
Copy link
Contributor

I did say I didn't think that was worth the trouble :-)

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.

Better synchronize SAW with the mir-json version it depends on
2 participants