-
Notifications
You must be signed in to change notification settings - Fork 277
Rust crate preparation - final touches #7625
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
Rust crate preparation - final touches #7625
Conversation
… internal headers
`other/` contains a C smoke test file, that we need to include so that a user of the crate can actually test the crate without getting errors related to lack of the file.
…ory. The build requirements have failed, and any other changes are very *very* brittle, as they require downloading a CBMC package (so that an `include/cprover/` folder is present), knowing the version and the location of the folder, and that will also fail on the release PRs incrementing the version (as the packaged versions and the reported version are going to be divergent).
d9e315f
to
8ca2615
Compare
Codecov ReportPatch and project coverage have no change.
Additional details and impacted files@@ Coverage Diff @@
## develop #7625 +/- ##
========================================
Coverage 78.50% 78.50%
========================================
Files 1671 1671
Lines 191847 191847
========================================
Hits 150618 150618
Misses 41229 41229 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@@ -27,7 +29,7 @@ directory of the CBMC project.) | |||
```sh | |||
$ cd src/libcprover-rust | |||
$ cargo clean | |||
$ CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo build | |||
$ CBMC_INCLUDE_DIR=../../build/include CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo build |
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.
Maybe don't fix a version of CBMC here, use something like CBMC_VERSION=X.XX.X
?
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 in the documentation, and I meant it as a concrete example.
Should I still change this? Perhaps a good middle ground might be to add a reference to the version schema in the description of the environment variable, starting in line 19.
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'm agnostic on it. On the one hand I can see someone simply copying and pasting, on the other hand a concrete example is nicer than one with further things to modify.
@@ -36,7 +38,118 @@ look like this: | |||
```sh | |||
$ cd src/libcprover-rust | |||
$ cargo clean | |||
$ CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo test -- --test-threads=1 --nocapture | |||
$ CBMC_INCLUDE_DIR=../../build/include CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo test -- --test-threads=1 --nocapture |
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.
And here.
@@ -27,7 +29,7 @@ directory of the CBMC project.) | |||
```sh | |||
$ cd src/libcprover-rust | |||
$ cargo clean | |||
$ CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo build | |||
$ CBMC_INCLUDE_DIR=../../build/include CBMC_LIB_DIR=../../build/lib CBMC_VERSION=5.78.0 cargo build |
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'm agnostic on it. On the one hand I can see someone simply copying and pasting, on the other hand a concrete example is nicer than one with further things to modify.
// those), and so because of the mechanism that cxx.rs uses, we need to have the | ||
// types present at compilation time (an incomplete type won't do - I've tried). | ||
// At the same time, we want to allow the Rust API to be able to catch at the | ||
// shimlevel the errors generated within CBMC, which are C++ types (and |
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.
shimlevel -> shim level
|
||
// The vector of string is supposed to contain a string denoting | ||
// a filepath that is erroneous. | ||
let vec: Vec<String> = vec!["/fkjsdlkjfisudifoj2309".to_owned()]; |
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 have not tried to break this, but what happens if I have a valid input file at /fkjsdlkjfisudifoj2309
? I'm also curious if the /
is OS specific to any degree?
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.
Well, that was my best attempt at making a random string + file path combo, without resorting to a more elaborate mechanism based on random strings or whatnot.
I'm personally happy with it failing if a user does have a randomly (?) named file locally present. If that happens, the user/developer can just have a look at the test, and see that we are looking for this file, inspect his filesystem and locate the file, and potentially alter the test/delete the file.
This is the final PR before we publish the crate on
crates.io
.Have slightly improved docs, build and added a test to verify that our translation
of C++ exceptions to Rust Result types work.
cargo test
,cargo build
,cargo doc
andcargo publish --dry-run
all work at this point.