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

feat: add compile sierra to casm util #24

Closed
wants to merge 1 commit into from

Conversation

ArniStarkware
Copy link
Contributor

@ArniStarkware ArniStarkware commented Apr 4, 2024

This change is Reviewable

@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/use_executable/try_5 branch 4 times, most recently from c3ebd7c to 41bf4a4 Compare April 7, 2024 07:54
@ArniStarkware ArniStarkware force-pushed the arni/sierra_to_casm/use_executable/try_5 branch from 41bf4a4 to 682b35f Compare April 7, 2024 08:00
@ArniStarkware
Copy link
Contributor Author

crates/gateway/build.rs line 13 at r2 (raw file):

    command.arg("--root");
    command.arg("tmp/cargo"); // TODO: DOn't dup the path.
    command.arg("starknet-sierra-compile");

This causes the following files to be created:

The compiler exe: crates/gateway/tmp/cargo/bin/starknet-sierra-compile
Files related to cargo: crates/gateway/tmp/cargo/.crates.toml, crates/gateway/tmp/cargo/.crates2.json

Code quote:

    let mut command = Command::new("cargo");
    command.arg("install");
    command.arg("--root");
    command.arg("tmp/cargo"); // TODO: DOn't dup the path.
    command.arg("starknet-sierra-compile");

@ArniStarkware
Copy link
Contributor Author

This replaces: #16

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 48.93%. Comparing base (2e9a1d0) to head (682b35f).

Files Patch % Lines
crates/gateway/src/compiler/compile.rs 80.95% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main      #24       +/-   ##
===========================================
+ Coverage   23.07%   48.93%   +25.85%     
===========================================
  Files           2        3        +1     
  Lines          26       47       +21     
  Branches       26       47       +21     
===========================================
+ Hits            6       23       +17     
- Misses         20       22        +2     
- Partials        0        2        +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@giladchase giladchase left a comment

Choose a reason for hiding this comment

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

Great! Some thoughts for future reference:

  1. Git Submodule Alternative: Using git submodule, which is basically "cloning cairo repo in a specific commit hash/tag and putting it in some directory inside the mempool repo". This is less hackish, and will make versioning and caching simpler (see comments below).
    This will change the build.rs file into "cargo build in the submodule dir", which will generate the executable inside a dedicated target/ directory, inside the submodule dir. Changing the version will be just changing the tag that the submodule is checked-out in.
    However, this solution might be overkill.

  2. Binary dependencies is an unstable feature in nightly rust ATM, and from looking at the tracking issue, it has a bunch of stabilization issues and won't be stabilized anytime soon. So this PR's workaround approach will stay relevant for the foreseeable future.
    If we want to find better workarounds, we can always scrape the mentions of the tracking issue for other repos that mention it while merging their own workarounds.

  3. Separate Crate for Logic: Ideally, moving this build logic to a separate crate would be preferable for organization. However, this complicates access to the generated executable, making the current module-based approach more practical for now.

Reviewable status: 0 of 6 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)


crates/gateway/build.rs line 12 at r2 (raw file):

    command.arg("install");
    command.arg("--root");
    command.arg("tmp/cargo"); // TODO: DOn't dup the path.

Suggestion uses the location that build.rs output is expected to be, see first sentence here.

The main benefit of this (other than being the canonical location) is that it caches the result since it places it inside the target directory.

However, now we need to deal with cache invalidation.
I naive way of doing this that won't bug us too much is to add:

println!("cargo:rerun-if-changed=path/to/Cargo.lock");
// Add more of these as desired, like maybe one for changes in build.rs itself.

as the first line in the main function.
This is cargo magic that will re-trigger build.rs whenever Cargo.lock changes, with the logic being that this is kind of a dependency, so let's compile it along with the rest of the dependency changes when they appear.

Suggestion:

    let out_dir = env::var("OUT_DIR").unwrap();
    let mut command = Command::new("cargo");
    command.arg("install");
    command.arg("--root");
    command.arg(&out_dir); // TODO: DOn't dup the path.

crates/gateway/build.rs line 13 at r2 (raw file):

    command.arg("--root");
    command.arg("tmp/cargo"); // TODO: DOn't dup the path.
    command.arg("starknet-sierra-compile");

There is still the issue of versioning.

This currently installs the latest version, and it isn't clear what version that is, and when should it be updated.

The obvious naive solution is to track a hardcoded version in this file, but that introduces coupling with the version in the Cargo.toml which will break at some point.

However, an even uglier hack that eliminates this coupling is to parse the cargo.toml and look for the version there 🥲.
rustc-perf used a variation of this hack (but not for version tracking I think) so you can use that for reference.

If we go along with this hack, we might be able to tweak our cache invalidation of this file (see previous comments) to be version sensitive.

The place in cargo.toml that will list the dependedcy can be [build-dependencies], which are deps available solely for bjuild.rs.

Code quote:

command.arg("starknet-sierra-compile")

crates/gateway/src/compiler/account_faulty.sierra.json line 1 at r2 (raw file):

{

Move to crates/gateway/tests/fixtures/.
This is where cargo expects test things to be, specifically integration tests, but the main point is that it detaches it from the source code of the crate, and is compiled in as a separate anonymous crate.X

@ArniStarkware
Copy link
Contributor Author

This is replaced by: #35

@ArniStarkware ArniStarkware deleted the arni/sierra_to_casm/use_executable/try_5 branch May 30, 2024 12:59
@ArniStarkware ArniStarkware restored the arni/sierra_to_casm/use_executable/try_5 branch July 16, 2024 08:40
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.

3 participants