-
Notifications
You must be signed in to change notification settings - Fork 63
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 pin the mir-json
version that SAW requires
#2115
Merged
+74
−3
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Binary file not shown.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this section ought to say something about translate-libs and $SAW_RUST_LIBRARY_PATH, but maybe that belongs in a separate branch.
Otherwise, LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good point, although I wonder if the
README
is the best place to list instructions at this level of depth. Let me know if you feel differently, but I am more inclined to describetranslate-libs
,SAW_RUST_LIBRARY_PATH
, and other topics related tomir-json
in the SAW manual, and then include a forward reference to the SAW manual in this part of theREADME
.That being said, I just checked the relevant part of the SAW manual, and it is unfortunately somewhat scarce on details of how one actually installs
mir-json
. I definitely think we should rectify that, and since this PR is mostly a documentation-related change, I would be fine with adding such details to this PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess there's two aspects to this: (1) the translate-libs process should be documented somewhere, but also (2) all the critical pieces of the complete setup process for doing MIR verification should be mentioned in the same places, especially as long as things fail in a non-obvious way if you forget parts of it. So I think if we're going to mention setting up mir-json in the README, it should also mention the translate-libs process, even if just as a reference into the manual.
(I actually think it's probably good to have the complete setup process in the README, but I'm not totally committed to this given that the whole thing is experimental and hopefully by the time it isn't it'll all be less complicated and less fragile)
So... carry on I guess :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a new version of the PR that expands the discussion in the
README
somewhat, mentioning both the need for a specificmir-json
version and the need for custom Rust standard libraries. I've also fleshed out the relevant section in the SAW manual to mention themir-json
submodule,translate_libs.sh
, and theSAW_RUST_LIBRARY_PATH
environment variable. I think this should be much better than it was before—PTAL.The manual currently assumes that you are starting from a
saw-script
repo checkout. This is not ideal, since many SAW users download a binary distribution instead. In order to do better here, however, I think we need to first explicitly versionmir-json
's schema. I've started work on that in GaloisInc/mir-json#75.