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

Introduce decodeme crate and start supporting old file format versions. #181

Merged
merged 8 commits into from
Sep 15, 2021

Conversation

michaelwoerister
Copy link
Member

@michaelwoerister michaelwoerister commented Sep 13, 2021

This PR splits the version-specific parts of file decoding into a separate crate called decodeme. This will make it easy in the future to keep supporting old file formats by making analyzeme depend on multiple versions of decodeme and always have it convert data to the current format.

As an example, this is what the crate graph would look like for analyzeme 17.0.0 if we want it to support a couple of older file formats.

measureme_15_0_0 <--- decodeme_15_0_0 <----+
                                           |
measureme_16_0_0 <--- decodeme_16_0_0 <----+
                                           |
measureme_17_0_0 <--- decodeme_17_0_0 <----+---- analyzeme_17_0_0

See analyzeme/src/file_formats/v7.rs for an example of what it looks like to implement support for an old file format.

Since decodeme does not yet exist for v9.x, this commit will make analyzeme 10.0.0 depend on analyzeme 9.2.0.

TODO:

  • Merge Prepare 9.2.0 release #180 and make this PR depend on the 9.2.0 git tag.
  • Update installation instructions and CHANGELOG.md
  • Add test cases that make sure old fileformats still work? @rylev will take care of this in a separate PR.
  • Make error messages suggest to upgrade to a newer version of measureme.

r? @wesleywiser & @rylev

Fixes #178

@michaelwoerister michaelwoerister force-pushed the decodeme-v10 branch 2 times, most recently from 824eb7f to 14db1f7 Compare September 13, 2021 14:39
Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks great!

analyzeme/Cargo.toml Outdated Show resolved Hide resolved
analyzeme/src/file_formats/v7.rs Show resolved Hide resolved
analyzeme/src/file_formats/v7.rs Outdated Show resolved Hide resolved
analyzeme/src/file_formats/v7.rs Outdated Show resolved Hide resolved
analyzeme/src/file_formats/v8.rs Outdated Show resolved Hide resolved
decodeme/src/lib.rs Outdated Show resolved Hide resolved
@michaelwoerister
Copy link
Member Author

I did not update the installation instructions. Instead I opened #182 for discussing a more permanent solution.

Otherwise I think this is ready for a final review.

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

This looks great!

analyzeme/src/profiling_data.rs Show resolved Hide resolved
@wesleywiser wesleywiser merged commit 47eb7c8 into rust-lang:master Sep 15, 2021
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.

Make it easier to transition between versions
3 participants