-
Notifications
You must be signed in to change notification settings - Fork 211
add ensure-no-std crate to workspace + add ensure_no_std crate #1215
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
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 |
Oppen
left a comment
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
| #[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. |
pefontana
left a comment
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_stdto 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-unknowtarget. 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,
vmand split the root Cargo.toml between itsworkspacepart, which stay at the same place, and it'scairo-vmpart which has been moved into thevmdirectory.This allow for a much more clear config, clearly stating what is at the workspace level and what only concerns the
vmcrate.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_stdonly check forcairo-feltfor now becausecairo-vmno-std is broken. It is not used in the CI yet, it will when thevmwill be fixed again