-
Notifications
You must be signed in to change notification settings - Fork 160
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
add ensure-no-std crate to workspace + add ensure_no_std crate #1215
Conversation
0860d29
to
7b2eeb2
Compare
@juanbono @Oppen Would you like to give it a review? The |
And I don't know what is going on with this one: https://github.com/lambdaclass/cairo-rs/actions/runs/5220573543/jobs/9423831041?pr=1215 |
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.
On the whole it looks good. I have just two questions.
There's one more issue. It seems the IAI benchmark no longer finds the data from the previous run. That may happen if the |
@Oppen nope, target is still being build at the root level |
76e8090
to
5aedbcd
Compare
vm/src/vm/errors/vm_exception.rs
Outdated
#[cfg(test)] | ||
let input_file_path = { | ||
use std::path::PathBuf; | ||
let current_dir = std::env::current_dir().unwrap(); | ||
let mut parent_dir: PathBuf = current_dir.parent().unwrap().into(); | ||
parent_dir.push(input_file_path); | ||
parent_dir | ||
}; |
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 fixes the path problem.
As tests are run from the crate directory itself, we can just get the current_dir, take its parent, and then append the input_file_path
.
It gives us an absolute path that works.
That doesn't solve the case where programs are not compiled from the same dir as the one the vm is executed. But it will be a problem for 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.
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.
That's cool. Could you add that information as a little comment in place?
b54aa01
to
5f0fb4d
Compare
Codecov Report
@@ Coverage Diff @@
## main #1215 +/- ##
=======================================
Coverage 97.60% 97.60%
=======================================
Files 89 89
Lines 36195 36205 +10
=======================================
+ Hits 35327 35337 +10
Misses 868 868
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
c58518a
to
e2a495b
Compare
The build itself is doing that, but
We need to update the
Note the path changes so it looks inside crate specific target dirs. |
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.
Amazing work @tdelabro !!!!
🎉
@tdelabro Can you please add a line in the CHANGELOG.md explaining the added feature and we will be OK to go! |
e2a495b
to
2615034
Compare
no_std check
Description
add a way to check that the crates are not breaking their no-std compatiblity
Some times ago, new features were added to cairo-vm that broke it's no-std compatibility.
This PR add the crate
ensure_no_std
to the repo. It is empty a binary, to be compiled with dependency on the crates we want to test for no-std (cairo-vm, and cairo-felt). We should build it in the CI, if the build fail it is that one of the dependency has break it's no-std compatibility.It has to be build with nightly and to a the
wasm32-unknow-unknow
target. Which mean it cannot be part of the workspace. But for some reason,members = ["."]
combined to theexclude = ["ensure_no_std"]
caused trouble to the rust compiler.So I moved the VM in it's own directory,
vm
and split the root Cargo.toml between itsworkspace
part, which stay at the same place, and it'scairo-vm
part which has been moved into thevm
directory.This allow for a much more clear config, clearly stating what is at the workspace level and what only concerns the
vm
crate.It is not perfect. It would be great that a subsequent PR make every crates in the repo depends on the workspace rather than each one re-specifying it's own deps. But it is still better than the previous state.
The
ensure_no_std
only check forcairo-felt
for now becausecairo-vm
no-std is broken. It is not used in the CI yet, it will when thevm
will be fixed again