feat(blockifier): compile cairo native runtime

[rodrigo-pino]

Add the Cairo Native runtime binary to the root after executing ./scripts/dependencies.sh

---

This change is 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.305 ms 34.420 ms 34.582 ms]
change: [-2.7316% -2.2314% -1.6446%] (p = 0.00 < 0.05)
Performance has improved.
Found 12 outliers among 100 measurements (12.00%)
4 (4.00%) high mild
8 (8.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.574 ms 34.914 ms 35.295 ms]
change: [+1.2328% +2.4180% +3.4831%] (p = 0.00 < 0.05)
Performance has regressed.
Found 22 outliers among 100 measurements (22.00%)
5 (5.00%) high mild
17 (17.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.397 ms 34.464 ms 34.538 ms]
change: [-6.9043% -5.2597% -3.7010%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
2 (2.00%) high severe

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.133 ms 34.188 ms 34.255 ms]
change: [-7.1077% -4.8592% -2.9185%] (p = 0.00 < 0.05)
Performance has improved.
Found 9 outliers among 100 measurements (9.00%)
3 (3.00%) high mild
6 (6.00%) high severe

[avi-starkware]

Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

---

scripts/dependencies.sh line 73 at r5 (raw file):

    popd || exit 1

    mv ./cairo_native/target/release/libcairo_native_runtime.a ./libcairo_native_runtime.so

Why did you rename the file extension .a -> .so?

Code quote:

    mv ./cairo_native/target/release/libcairo_native_runtime.a ./libcairo_native_runtime.so

[avi-starkware]

Reviewed 1 of 2 files at r1, 2 of 2 files at r5, all commit messages.
Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @rodrigo-pino)

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [35.223 ms 35.283 ms 35.348 ms]
change: [-9.4652% -6.7850% -4.3002%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
5 (5.00%) high mild
1 (1.00%) high severe

full_committer_flow performance improved 😺
full_committer_flow time: [29.919 ms 29.967 ms 30.014 ms]
change: [-3.6412% -2.4774% -1.6118%] (p = 0.00 < 0.05)
Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
1 (1.00%) low mild
1 (1.00%) high mild

[rodrigo-pino]

Reviewable status: 1 of 4 files reviewed, all discussions resolved (waiting on @avi-starkware)

---

scripts/dependencies.sh line 73 at r5 (raw file):

Previously, avi-starkware wrote…

Why did you rename the file extension .a -> .so?

Done

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.107 ms 34.135 ms 34.165 ms]
change: [-5.5006% -4.0332% -2.6914%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
3 (3.00%) high mild
2 (2.00%) high severe

[avi-starkware]

crates/blockifier/build.rs line 47 at r22 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

think the println! directives for the rerun-if flags have to be at the beginning of main() to work.

Nop, they location is not important, I put them there to re-use variables. Can put them in front if you like that more.

it doesn't make sense to have the flags that check if the script should be rerun depend on variables that only exist if the entire script has indeed been rerun.

I see no relationship between both mechanisms. The directives are there in case there is a change in the env-var. The script fails to run if the env vars are unset.
If the script never got built then directives are not important since nothing go built.

As I mentioned before, the directives can go anywhere. In our specific context they are only relevant when cairo_native feature is enabled, so the only requirement is that they get printed if that condition is met. Again, I put them there so that any change in the build.rs will keep it consistent with the directives, I can put it in front if you think it's better.

I tried to test if the flags work properly on my machine locally, but I can't get the build script to run without error.
This is the error I am getting:

The error is caused because you don't have the submodule. You can fix it by running:

git submodule update --init --recursive

I believe this can be detected easily in the build.rs, I'll add to it

Now it works thanks!

Can you please add a comment documenting the need for this additional command?

I played around with building the crate and rerunning it, and it seems that the cargo build always reruns the build.rs script as long as the rerun-if-changed directives are present, regardless of whether the files have actually changed. This holds true also when I put the directives at the beginning of main (with explicit paths - no variables).

When I experimented with rerun-if-changed directive on a simpler build.rs script, it behaved as expected: rerunning when there were changes and skipping build.rs if there were no changes, so I am not sure what is happening here.

Did you find a case where the script is not rerun in its current state? (I can see the script is rerun since crates/blockifier/libcairo_native_runtime.a is modified each time I run the script.)

[avi-starkware]

crates/blockifier/build.rs line 47 at r22 (raw file):

Previously, avi-starkware wrote…

Now it works thanks!

Can you please add a comment documenting the need for this additional command?

I played around with building the crate and rerunning it, and it seems that the cargo build always reruns the build.rs script as long as the rerun-if-changed directives are present, regardless of whether the files have actually changed. This holds true also when I put the directives at the beginning of main (with explicit paths - no variables).

When I experimented with rerun-if-changed directive on a simpler build.rs script, it behaved as expected: rerunning when there were changes and skipping build.rs if there were no changes, so I am not sure what is happening here.

Did you find a case where the script is not rerun in its current state? (I can see the script is rerun since crates/blockifier/libcairo_native_runtime.a is modified each time I run the script.)

Regardless, this is non-blocking.

[rodrigo-pino]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @alon-dotan-starkware, @noaov1, and @PearsonWhite)

---

crates/blockifier/build.rs line 47 at r22 (raw file):

Can you please add a comment documenting the need for this additional command?

The error can be detected, would it be good enough suggesting the fix when facing it the first time?
Maybe setting the command as part of the dependencies.sh script? I am unsure how it would affect the CI. Will try this out as well (cc: @alon-dotan-starkware)

Regarding your last three paragraphs, I find it as an odd behaviour. After I implemented the solution I tested it locally and it worked as expected (tried each directive individually) and the build was successfully triggered each time. Then, tried two+ builds in a row and after the first one, the others were no-op (also verified that the libcairo_native_runtime.a remained unmodified)

I'll ask for someone from the team to check if they have the same behaviour as your. We are all using macOS making our results biased. Can you see if your results persist on other Linux machines. (Thinking OS file system differences as a root cause). Finally, to completely rule out human error, are you sure that when running this script (as it is presented in this branch) twice in a row the second re-builds Native Runtime? Could you detect which directive of the three is causing the issue?

[rodrigo-pino]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alon-dotan-starkware, @noaov1, and @PearsonWhite)

---

crates/blockifier/build.rs line 47 at r22 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

Can you please add a comment documenting the need for this additional command?

The error can be detected, would it be good enough suggesting the fix when facing it the first time?
Maybe setting the command as part of the dependencies.sh script? I am unsure how it would affect the CI. Will try this out as well (cc: @alon-dotan-starkware)

Regarding your last three paragraphs, I find it as an odd behaviour. After I implemented the solution I tested it locally and it worked as expected (tried each directive individually) and the build was successfully triggered each time. Then, tried two+ builds in a row and after the first one, the others were no-op (also verified that the libcairo_native_runtime.a remained unmodified)

I'll ask for someone from the team to check if they have the same behaviour as your. We are all using macOS making our results biased. Can you see if your results persist on other Linux machines. (Thinking OS file system differences as a root cause). Finally, to completely rule out human error, are you sure that when running this script (as it is presented in this branch) twice in a row the second re-builds Native Runtime? Could you detect which directive of the three is causing the issue?

...

[avi-starkware]

crates/blockifier/build.rs line 47 at r22 (raw file):

The error can be detected, would it be good enough suggesting the fix when facing it the first time?

This is a good idea! I think adding the explanation to the error message, as you suggested, would be sufficient.

I'll do some more testing on the rerun behavior on my machine and other linux machines.

[rodrigo-pino]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alon-dotan-starkware, @noaov1, and @PearsonWhite)

---

crates/blockifier/build.rs line 47 at r22 (raw file):

I'll do some more testing on the rerun behavior on my machine and other linux machines.

Thank you!

...

This previous message of mine was because I mistakenly set the thread as resolved 🤦‍♂️ , I've just noticed that it's meaning wasn't completely clear

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.772 ms 35.129 ms 35.576 ms]
change: [+1.6433% +2.6817% +3.9920%] (p = 0.00 < 0.05)
Performance has regressed.
Found 17 outliers among 100 measurements (17.00%)
7 (7.00%) high mild
10 (10.00%) high severe

[alon-dotan-starkware]

Reviewable status: 0 of 11 files reviewed, 2 unresolved discussions (waiting on @noaov1 and @PearsonWhite)

[github-actions]

Artifacts upload triggered. View details here

[github-actions]

Benchmark movements:
full_committer_flow performance improved 😺
full_committer_flow time: [29.541 ms 29.601 ms 29.665 ms]
change: [-1.5859% -1.3224% -1.0415%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

[noaov1]

Reviewed 1 of 2 files at r52, 9 of 10 files at r53, 1 of 1 files at r54, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alon-dotan-starkware, @PearsonWhite, and @rodrigo-pino)

---

taplo.toml line 4 at r46 (raw file):

Previously, rodrigo-pino (Rodrigo) wrote…

All the other lints are working over it, taplo is the only one ignoring it. We can make a PR to Cairo Native that fix their taplo issues and then remove this exclusion later on

No need IMO

---

crates/blockifier/build.rs line 59 at r54 (raw file):

    println!("cargo::rerun-if-changed={}", runtime_expected_path.to_str().unwrap());
    println!("cargo::rerun-if-env-changed={RUNTIME_LIBRARY}");
}

Why do we need both?

Code quote:

    println!("cargo::rerun-if-changed={}", runtime_expected_path.to_str().unwrap());
    println!("cargo::rerun-if-env-changed={RUNTIME_LIBRARY}");
}

---

crates/blockifier/build.rs line 62 at r54 (raw file):

fn main() {
    if std::env::var("CARGO_FEATURE_CAIRO_NATIVE").is_ok() {

Can you please explain this check? (and maybe document)

Code quote:

if std::env::var("CARGO_FEATURE_CAIRO_NATIVE").is_ok() {

[noaov1]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alon-dotan-starkware, @PearsonWhite, and @rodrigo-pino)

[github-actions]

Artifacts upload triggered. View details here

[rodrigo-pino]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware, @noaov1, and @PearsonWhite)

---

crates/blockifier/build.rs line 59 at r54 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Why do we need both?

The first one in case the runtime gets deleted, since ti is a binary I think no one will be modifying it.

The second one is in case a developer changes the runtime library path to re-compile it in that location.

---

crates/blockifier/build.rs line 62 at r54 (raw file):

Previously, noaov1 (Noa Oved) wrote…

Can you please explain this check? (and maybe document)

Done , explanation added as a comment

[noaov1]

Reviewed 1 of 1 files at r55, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alon-dotan-starkware and @PearsonWhite)

[avi-starkware]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @PearsonWhite)