fix(starknet_sierra_compile): resolve out_dir usage

[avi-starkware]

No description provided.

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 0.48%. Comparing base (e3165c4) to head (f12b98a).
Report is 816 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #2616       +/-   ##
==========================================
- Coverage   40.10%   0.48%   -39.62%     
==========================================
  Files          26      98       +72     
  Lines        1895   13365    +11470     
  Branches     1895   13365    +11470     
==========================================
- Hits          760      65      -695     
- Misses       1100   13293    +12193     
+ Partials       35       7       -28     

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

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @ArniStarkware and @avi-starkware)

---

crates/starknet_sierra_compile/build.rs line 51 at r1 (raw file):

    // Create a directory to store the compiled outputs.
    let compiled_output_dir = compilation_output_dir(out_dir());
    std::fs::create_dir_all(&compiled_output_dir).expect("Failed to create the output directory.");

Please see comment below 🙏

Code quote:

std::fs::create_dir_all(&compiled_output_dir).expect("Failed to create the output directory.");

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

#[cfg(feature = "cairo_native")]
pub fn compilation_output_file_path(out_dir: std::path::PathBuf) -> String {
    compilation_output_dir(out_dir).join("output.so").to_str().unwrap().into()

I'm slightly confused:

I thought that out_dir() points to the outputs of the build script, namely the native compiler executable.
out_dir() example: target/debug/build/starknet_sierra_compile-e679b05b01eef89a

I also thought that during the build process, the binaries are moved from out_dir() to target/debug/shared_executables

Why does the code require out_dir() to invoke the executables and to access their results? I would have expected it to rely on target/debug/shared_executables for that.

Am I missing something?

Code quote:

compilation_output_dir(out_dir).join("output.so").to_str().unwrap().into()

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 71 at r1 (raw file):

        let output_directory = compilation_output_dir(out_dir());
        std::fs::create_dir_all(&output_directory)
            .expect("Failed to create native compilation output directory");

WDYT about moving this into compilation_output_dir(...)?

Code quote:

        std::fs::create_dir_all(&output_directory)
            .expect("Failed to create native compilation output directory");

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware and @avi-starkware)

---

crates/starknet_sierra_compile/build.rs line 109 at r1 (raw file):

        .args([post_install_file_path.as_os_str(), binary_path.as_os_str()])
        .status()
        .expect("Failed to perform mv command.");

Using this pr to point at another issue:
I think that this mv command is the culprit that re-triggers the build process -- the executable is removed from the out_dir(), triggering the println!("cargo:rerun-if-changed={:?}", binary_path); invocation condition.

Could we please change this to cp for the meanwhile, and add a todo to further investigate / design a more suited mechanim?

@noaov1 @giladchase @dan-starkware @dorimedini-starkware FYI

Code quote:

    let move_command_status = Command::new("mv")
        .args([post_install_file_path.as_os_str(), binary_path.as_os_str()])
        .status()
        .expect("Failed to perform mv command.");

[avi-starkware]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @ArniStarkware, @dan-starkware, @dorimedini-starkware, @giladchase, @Itay-Tsabary-Starkware, and @noaov1)

---

crates/starknet_sierra_compile/build.rs line 51 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please see comment below 🙏

Done.

---

crates/starknet_sierra_compile/build.rs line 109 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Using this pr to point at another issue:
I think that this mv command is the culprit that re-triggers the build process -- the executable is removed from the out_dir(), triggering the println!("cargo:rerun-if-changed={:?}", binary_path); invocation condition.

Could we please change this to cp for the meanwhile, and add a todo to further investigate / design a more suited mechanim?

@noaov1 @giladchase @dan-starkware @dorimedini-starkware FYI

I don't think that is the issue. I think I resolved the re-triggering of the build process. The culprit was REQUIRED_CAIRO_NATIVE_VERSION. It held the wrong version, so re-installation was always required. Now, rebuilding the crate after a successful build takes less than 1 second.

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 71 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

WDYT about moving this into compilation_output_dir(...)?

Done.

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

I'm slightly confused:

I thought that out_dir() points to the outputs of the build script, namely the native compiler executable.
out_dir() example: target/debug/build/starknet_sierra_compile-e679b05b01eef89a

I also thought that during the build process, the binaries are moved from out_dir() to target/debug/shared_executables

Why does the code require out_dir() to invoke the executables and to access their results? I would have expected it to rely on target/debug/shared_executables for that.

Am I missing something?

This is just a path to put the output of sierra -> native compilation in. Not the path to the binary of the compiler itself.
Should I change this path to /tmp?

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @avi-starkware)

---

crates/starknet_sierra_compile/build.rs line 109 at r1 (raw file):

Previously, avi-starkware wrote…

I don't think that is the issue. I think I resolved the re-triggering of the build process. The culprit was REQUIRED_CAIRO_NATIVE_VERSION. It held the wrong version, so re-installation was always required. Now, rebuilding the crate after a successful build takes less than 1 second.

👑

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

Previously, avi-starkware wrote…

This is just a path to put the output of sierra -> native compilation in. Not the path to the binary of the compiler itself.
Should I change this path to /tmp?

Yes, please do 🫡

[avi-starkware]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @ArniStarkware and @Itay-Tsabary-Starkware)

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Yes, please do 🫡

Done.

[Itay-Tsabary-Starkware]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @avi-starkware)

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

Previously, avi-starkware wrote…

Done.

Please see the suggestion above. If you find it appealing, then you can delete both of these functions.
If not, regardless of other changes, I suggest unifying them into a single function, as compilation_output_dir() is used only once, by compilation_output_file_path().

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 69 at r3 (raw file):

    ) -> Result<AotContractExecutor, CompilationUtilError> {
        let compiler_binary_path = &self.path_to_starknet_native_compile_binary;
        let output_file_path = compilation_output_file_path();

Suggestion:

1. create a rust temp dir here (tempdir())
2. create the output path with .join("output.so")

This has the added benefits of:

1. It maintains the temp dir handle exactly for as needed: the scope of this function.
2. Multiple / concurrent invocations should not interfere with each other
3. Resistent to the OS cleaning /tmp in the background
4. Removes the need for other functions.

WDYT?

Code quote:

let output_file_path = compilation_output_file_path();

[avi-starkware]

Reviewable status: 0 of 5 files reviewed, 2 unresolved discussions (waiting on @ArniStarkware and @Itay-Tsabary-Starkware)

---

crates/starknet_sierra_compile/src/command_line_compiler.rs line 69 at r3 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Suggestion:

1. create a rust temp dir here (tempdir())
2. create the output path with .join("output.so")

This has the added benefits of:

1. It maintains the temp dir handle exactly for as needed: the scope of this function.
2. Multiple / concurrent invocations should not interfere with each other
3. Resistent to the OS cleaning /tmp in the background
4. Removes the need for other functions.

WDYT?

Done.

---

crates/starknet_sierra_compile/src/paths.rs line 28 at r1 (raw file):

Previously, Itay-Tsabary-Starkware wrote…

Please see the suggestion above. If you find it appealing, then you can delete both of these functions.
If not, regardless of other changes, I suggest unifying them into a single function, as compilation_output_dir() is used only once, by compilation_output_file_path().

Done.

[Itay-Tsabary-Starkware]

Reviewed 2 of 5 files at r1, 1 of 3 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

[ArniStarkware]

Reviewed 1 of 5 files at r1, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @avi-starkware)