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

add ensure-no-std crate to workspace + add ensure_no_std crate #1215

Merged
merged 1 commit into from
Jun 13, 2023

Conversation

tdelabro
Copy link
Contributor

@tdelabro tdelabro commented Jun 8, 2023

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 the exclude = ["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 its workspace part, which stay at the same place, and it's cairo-vm part which has been moved into the vm 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 for cairo-felt for now because cairo-vm no-std is broken. It is not used in the CI yet, it will when the vm will be fixed again

@tdelabro tdelabro force-pushed the split-workspace-and-vm branch 13 times, most recently from 0860d29 to 7b2eeb2 Compare June 9, 2023 09:50
@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 9, 2023

@juanbono @Oppen Would you like to give it a review?
The only thing blocking now is this problem with path.

The .json file keeps a relative path to itself, starting from where it has been compiled. If you execute the program in the same directory, everything works nicely, but if you do it from another directory, it won't be able to find the .json file.
I don't know how to fix this exactly, because run_program takes the program as a &[u8] without ever knowing where the file was generated initially.

@tdelabro
Copy link
Contributor Author

tdelabro commented Jun 9, 2023

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

Copy link
Contributor

@Oppen Oppen left a 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.

Cargo.toml Show resolved Hide resolved
Cargo.lock Outdated Show resolved Hide resolved
@Oppen
Copy link
Contributor

Oppen commented Jun 12, 2023

There's one more issue. It seems the IAI benchmark no longer finds the data from the previous run. That may happen if the target dir moves to the subcrates rather than the project's root, which I think is the case when there's no crate in the root as it is the case now.

@tdelabro
Copy link
Contributor Author

@Oppen nope, target is still being build at the root level

@tdelabro tdelabro force-pushed the split-workspace-and-vm branch 3 times, most recently from 76e8090 to 5aedbcd Compare June 12, 2023 15:47
Comment on lines 259 to 269
#[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
};
Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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?

@tdelabro tdelabro force-pushed the split-workspace-and-vm branch 2 times, most recently from b54aa01 to 5f0fb4d Compare June 12, 2023 16:07
@codecov
Copy link

codecov bot commented Jun 12, 2023

Codecov Report

Merging #1215 (2615034) into main (f8927d9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1215   +/-   ##
=======================================
  Coverage   97.60%   97.60%           
=======================================
  Files          89       89           
  Lines       36195    36205   +10     
=======================================
+ Hits        35327    35337   +10     
  Misses        868      868           
Impacted Files Coverage Δ
...rc/hint_processor/builtin_hint_processor/bigint.rs 97.65% <ø> (ø)
...t_processor/builtin_hint_processor/blake2s_hash.rs 100.00% <ø> (ø)
..._processor/builtin_hint_processor/blake2s_utils.rs 99.78% <ø> (ø)
...int_processor/builtin_hint_processor_definition.rs 99.06% <ø> (ø)
...uiltin_hint_processor/cairo_keccak/keccak_hints.rs 94.09% <ø> (ø)
...rocessor/builtin_hint_processor/dict_hint_utils.rs 99.20% <ø> (ø)
...t_processor/builtin_hint_processor/dict_manager.rs 95.74% <ø> (ø)
...int_processor/builtin_hint_processor/ec_recover.rs 100.00% <ø> (ø)
.../hint_processor/builtin_hint_processor/ec_utils.rs 98.53% <ø> (ø)
...ocessor/builtin_hint_processor/field_arithmetic.rs 98.95% <ø> (ø)
... and 75 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tdelabro tdelabro force-pushed the split-workspace-and-vm branch 3 times, most recently from c58518a to e2a495b Compare June 13, 2023 11:04
@Oppen
Copy link
Contributor

Oppen commented Jun 13, 2023

@Oppen nope, target is still being build at the root level

The build itself is doing that, but iai-callgrind has peculiar behavior with regards to that. I just confirmed in the server the benchmark results are stored within vm:

[pelito@archlinux-latest-64-minimal cairo-rs]$ ls vm/target/iai/iai_benchmark/
callgrind.build_runner.out  callgrind.load_program_data.out  callgrind.parse_program.out

We need to update the iai workflows so the caches look like this:

    - name: Save cache
      if: ${{ steps.cache-iai-results.outputs.cache-hit != 'true' }}
      uses: actions/cache/save@v3
      with:
        path: |
          */target
        key: ${{ runner.os }}-iai-benchmark-cache-${{ github.event.pull_request.base.sha }}

Note the path changes so it looks inside crate specific target dirs.

Copy link
Collaborator

@pefontana pefontana left a comment

Choose a reason for hiding this comment

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

Amazing work @tdelabro !!!!
🎉

ensure_no_std/rust-toolchain.toml Outdated Show resolved Hide resolved
@pefontana
Copy link
Collaborator

@tdelabro Can you please add a line in the CHANGELOG.md explaining the added feature and we will be OK to go!

@tdelabro tdelabro force-pushed the split-workspace-and-vm branch from e2a495b to 2615034 Compare June 13, 2023 14:59
@pefontana pefontana added this pull request to the merge queue Jun 13, 2023
Merged via the queue into lambdaclass:main with commit f053646 Jun 13, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
kariy pushed a commit to dojoengine/cairo-rs that referenced this pull request Jun 23, 2023
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.

4 participants