Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

This was referenced Apr 29, 2025
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 96bbf61 to 4bce024 Compare April 29, 2025 13:55
@dorimedini-starkware dorimedini-starkware self-assigned this Apr 29, 2025
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review April 29, 2025 13:57
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 4bce024 to b9809d5 Compare April 29, 2025 14:31
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from cf6c1c6 to 79149b6 Compare April 30, 2025 11:00
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 4baa338 to 8d04277 Compare May 5, 2025 12:50
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from fadd262 to 0a2e8aa Compare May 5, 2025 13:08
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 8d04277 to c31f349 Compare May 5, 2025 13:08
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from 0a2e8aa to 1128228 Compare May 6, 2025 12:26
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from c31f349 to 78fd611 Compare May 6, 2025 12:26
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from 1128228 to 230d440 Compare May 6, 2025 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 78fd611 to 5862351 Compare May 6, 2025 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from 230d440 to 534fc5d Compare May 8, 2025 09:22
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 5862351 to c133897 Compare May 8, 2025 09:22
@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from 534fc5d to f1733ec Compare May 8, 2025 09:51
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from c133897 to bfda8d5 Compare May 8, 2025 09:51
Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r2, 7 of 7 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 10 unresolved discussions (waiting on @TzahiTaub)


-- commits line 1 at r3:
note there are two commits here - not sure if Graphite will merge well like this

Code quote:

ew commits in r2 on 08/05/2025 at 12:50, probably rebased from r1:

crates/apollo_starknet_os_program/Cargo.toml line 17 at r3 (raw file):

[dependencies]
serde_json.workspace = true

you sure we need this?

Code quote:

[dependencies]
serde_json.workspace = true

crates/apollo_starknet_os_program/build/main.rs line 3 at r3 (raw file):

#[cfg(feature = "dump_source_files")]
use std::path::PathBuf;

why not call this file build.rs?


crates/apollo_starknet_os_program/build/main.rs line 8 at r3 (raw file):

/// Build script for the `apollo_starknet_os_program` crate.
/// Recompiles the OS program if the source files change.

This will happen in a future PR?
consider adding a TODO (if it's annoying then NVM)

Code quote:

/// Build script for the `apollo_starknet_os_program` crate.
/// Recompiles the OS program if the source files change.

.gitignore line 15 at r3 (raw file):

*.egg-info
/build

shouldn't you add crates/apollo_starknet_os_program/build?
I'm not familiar with gitignore patterns, so I may be wrong

Code quote:

/build

.gitignore line 34 at r3 (raw file):

# Native blockifier artifacts.
/crates/native_blockifier/build

I don't see this dir?

Code quote:

# Native blockifier artifacts.
/crates/native_blockifier/build

crates/apollo_starknet_os_program/build/dump_source.rs line 8 at r3 (raw file):

/// Utility function to recursively find all cairo files.
fn get_cairo_file_map_recursive(entry: DirEntry) -> HashMap<String, String> {

do we expect non-cairo files in the OS's directory (crates/apollo_starknet_os_program/src/cairo/starkware/starknet)?
if not you can pass - there's probably a std library function to copy all files in a directory.

Code quote:

fn get_cairo_file_map_recursive(entry: DirEntry) -> HashMap<String, String> {

crates/apollo_starknet_os_program/build/dump_source.rs line 22 at r3 (raw file):

                path.to_str().unwrap().to_string(),
                std::fs::read_to_string(path).unwrap(),
            )))

why use an iterator here?

Code quote:

            HashMap::from_iter(std::iter::once((
                path.to_str().unwrap().to_string(),
                std::fs::read_to_string(path).unwrap(),
            )))

crates/apollo_starknet_os_program/build/dump_source.rs line 32 at r3 (raw file):

/// the map as JSON to the specified location.
pub fn dump_source_files(dump_to: PathBuf) {
    println!("cargo::warning=Dumping OS source files...");

why warning?

Code quote:

println!("cargo::warning=Dumping OS source files...");

crates/apollo_starknet_os_program/build/dump_source.rs line 45 at r3 (raw file):

                .strip_prefix(base_path_string)
                .unwrap()
                .strip_prefix("/cairo/")

why not add this to base_path?

Code quote:

 .strip_prefix("/cairo/")

@dorimedini-starkware dorimedini-starkware force-pushed the 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code branch from f1733ec to 4efc54d Compare May 8, 2025 16:12
@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from bfda8d5 to fa52b15 Compare May 8, 2025 16:12
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 8 files reviewed, 10 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)


-- commits line 1 at r3:

Previously, amosStarkware wrote…

note there are two commits here - not sure if Graphite will merge well like this

WDYM? why not? the merge queue merge method squashes; graphite should handle this just fine, no?


.gitignore line 15 at r3 (raw file):

Previously, amosStarkware wrote…

shouldn't you add crates/apollo_starknet_os_program/build?
I'm not familiar with gitignore patterns, so I may be wrong

specifically, the crates/apollo_starknet_os_program/build directory is source code, not build artifacts.
the changes in this file are specifically here to not ignore that directory; without the changes, the build/main.rs and build/dump_source.rs files are ignored by git


.gitignore line 34 at r3 (raw file):

Previously, amosStarkware wrote…

I don't see this dir?

after I changed build -> /build, my git status showed me this; I may not be the only one, and it is definitely not something we want to commit. I see no harm in adding it to gitignore


crates/apollo_starknet_os_program/Cargo.toml line 17 at r3 (raw file):

Previously, amosStarkware wrote…

you sure we need this?

technically, in this PR, we only need it if the dump_source_files feature is active (see lib.rs); but I'd rather keep it as is, in subsequent PRs we will always need it (for the OS program deserialization itself)


crates/apollo_starknet_os_program/build/dump_source.rs line 8 at r3 (raw file):

Previously, amosStarkware wrote…

do we expect non-cairo files in the OS's directory (crates/apollo_starknet_os_program/src/cairo/starkware/starknet)?
if not you can pass - there's probably a std library function to copy all files in a directory.

we may decide to move the constants_template.txt file inside here, so I'd rather keep this search cairo-only. will also help with forward-compatibility, + I like being explicit in what I want to copy


crates/apollo_starknet_os_program/build/dump_source.rs line 22 at r3 (raw file):

Previously, amosStarkware wrote…

why use an iterator here?

Done.


crates/apollo_starknet_os_program/build/dump_source.rs line 32 at r3 (raw file):

Previously, amosStarkware wrote…

why warning?

this is how you print output in build scripts. see here. regular println! doesn't appear in the console


crates/apollo_starknet_os_program/build/dump_source.rs line 45 at r3 (raw file):

Previously, amosStarkware wrote…

why not add this to base_path?

Done. I still want to remove the leading / though, to get relative paths


crates/apollo_starknet_os_program/build/main.rs line 3 at r3 (raw file):

Previously, amosStarkware wrote…

why not call this file build.rs?

you either use build.rs, if you have a single build file, or build/main.rs if you have several build files (by convention). note this line in the Cargo.toml:

build = "build/main.rs"

crates/apollo_starknet_os_program/build/main.rs line 8 at r3 (raw file):

Previously, amosStarkware wrote…

This will happen in a future PR?
consider adding a TODO (if it's annoying then NVM)

a bit annoying to rebase; the reason this is here is because the entire "check file consistency" feature was added after I already had most of the stack in place (splitting build.rs into modules in build/* was very annoying, this slipped through)
will be implemented in later PRs (specifically, here)

@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from fa52b15 to 0cb58d5 Compare May 8, 2025 17:34
@dorimedini-starkware dorimedini-starkware changed the base branch from 04-13-feat_apollo_starknet_os_program_copy_os_cairo_code to main May 8, 2025 17:34
@github-actions
Copy link

github-actions bot commented May 8, 2025

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.497 ms 34.516 ms 34.536 ms]
change: [-4.8007% -3.0672% -1.5233%] (p = 0.00 < 0.05)
Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
3 (3.00%) high mild

Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @TzahiTaub)


-- commits line 1 at r3:

Previously, dorimedini-starkware wrote…

WDYM? why not? the merge queue merge method squashes; graphite should handle this just fine, no?

maybe, you know Graphite better. I thought Graphit wants one commit per branch


.gitignore line 15 at r3 (raw file):

Previously, dorimedini-starkware wrote…

specifically, the crates/apollo_starknet_os_program/build directory is source code, not build artifacts.
the changes in this file are specifically here to not ignore that directory; without the changes, the build/main.rs and build/dump_source.rs files are ignored by git

so any crate that has a build directory, which was ignored before, won't be ignored now?
are you sure there aren't many such directories?
it seems better to just rename apollo_starknet_os_program/build


crates/apollo_starknet_os_program/build/dump_source.rs line 45 at r3 (raw file):

Previously, dorimedini-starkware wrote…

Done. I still want to remove the leading / though, to get relative paths

you can just do join("src/cairo/");, no? non blocking

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @amosStarkware and @TzahiTaub)


-- commits line 1 at r3:

Previously, amosStarkware wrote…

maybe, you know Graphite better. I thought Graphit wants one commit per branch

apparently not :)


.gitignore line 15 at r3 (raw file):

Previously, amosStarkware wrote…

so any crate that has a build directory, which was ignored before, won't be ignored now?
are you sure there aren't many such directories?
it seems better to just rename apollo_starknet_os_program/build

my git status is clean, and I think it's standard to name this dir build, since in the case where it's a single module it's called build.rs (in which case no special setting in the Cargo.toml is required).
rust uses the target terminology for output artifacts


crates/apollo_starknet_os_program/build/dump_source.rs line 45 at r3 (raw file):

Previously, amosStarkware wrote…

you can just do join("src/cairo/");, no? non blocking

sure sure if PathBuf::from("a/").to_str() is "a/" or "a"

Copy link
Collaborator

@amosStarkware amosStarkware left a comment

Choose a reason for hiding this comment

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

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

@dorimedini-starkware dorimedini-starkware force-pushed the 04-29-feat_apollo_starknet_os_program_cairo_file_list_test_fixer branch from 0cb58d5 to 6e38ba8 Compare May 11, 2025 09:49
@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue May 11, 2025
Merged via the queue into main with commit 28bdb78 May 11, 2025
24 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants