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 version the JSON schema #75

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

RyanGlScott
Copy link
Contributor

@RyanGlScott RyanGlScott commented Sep 9, 2024

We now include a "version": <N> entry in the top-level JSON map that gets generated for each MIR JSON file, where <N> represents the version of the JSON schema that is used to generate the file. Going forward, any changes to the JSON schema will be accompanied by a corresponding schema version bump. I have also included some logic in link_crates to ensure that all of the crates being linked together use the same schema version.

Fixes #45.

Copy link
Collaborator

@spernsteiner spernsteiner left a comment

Choose a reason for hiding this comment

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

link.rs should probably check the versions of its input files and pick a version that's compatible with all of them for the output. Otherwise you can have this issues: the user builds a package and its dependencies with mir-json v1, then rebuilds the package only with mir-json v2, and link.rs ends up mixing v1 and v2 data and wrongly labels it all as v2.

This might require adding the version to the CrateIndex part (named index.cbor IIRC) of the pre-link representation - the crate.json part of that representation is never parsed in full for performance reasons.

README.md Outdated Show resolved Hide resolved
We now include a `"version": <N>` entry in the top-level JSON map that gets
generated for each MIR JSON file, where `<N>` represents the version of the
JSON schema that is used to generate the file. Going forward, any changes to
the JSON schema will be accompanied by a corresponding schema version bump. I
have also included some logic in `link_crates` to ensure that all of the crates
being linked together use the same schema version.

Fixes #45.
@RyanGlScott
Copy link
Contributor Author

I've pushed a reworked version of this PR that uses only a single number for the schema version, and which explicitly checks that all linked crates have the same schema version number. PTAL.

src/link.rs Outdated Show resolved Hide resolved
src/link.rs Outdated Show resolved Hide resolved
@RyanGlScott RyanGlScott merged commit 94be1d6 into master Sep 10, 2024
@RyanGlScott RyanGlScott deleted the T45-schema-versioning branch September 10, 2024 17:02
@sauclovian-g
Copy link
Contributor

It occurred to me just now (in the context of the crucible side) that there's a second factor in here, which is the rustc version. Eventually we'd like not to be tied to the one outdated rustc version, but since we don't control what rustc does we might have trouble encoding different rustc's MIR in the same schema version. So I was thinking we might actually want two major version numbers, one for essentially the MIR revision and one for our own JSON version within that. (Then crucible might support the last three JSON schema versions for each of the currently supported MIR revisions or whatever.)

also there were a couple things I'd add to the docs in here but since it's too late I'll just make another pull request.

@RyanGlScott
Copy link
Contributor Author

Eventually we'd like not to be tied to the one outdated rustc version, but since we don't control what rustc does we might have trouble encoding different rustc's MIR in the same schema version.

I am skeptical that we will ever be able to build a particular version of mir-json with anything more than a single rustc version. Given that mir-json is implemented using the rustc API, it's far too coupled to rustc's internals to be able to support multiple rustc versions without using a significant amount of #ifs (which would be very painful).

That being said, I think it would be fine to bump the schema version whenever we upgrade the version of rustc that we build against.

@sauclovian-g
Copy link
Contributor

In that case the corresponding situation is that we have branches of mir-json for different rustc versions. It's not inconceivable that we might want to do this and maintain more than one at a time. In that case, you don't want to end up in the situation where e.g. the 5.1 branch is using schema version 169 and the 5.2 branch is on schema version 174 after having gone through 170-173, and then some issue comes along where both need to be bumped.

There's no reason we need to add the extra sequence number now, but then we want to remember to add it as soon as we introduce multiple branches or anything of the sort. Which is fine I guess...

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.

Add a schema version to JSON
3 participants