Skip to content

Conversation

@dorimedini-starkware
Copy link
Collaborator

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@dorimedini-starkware dorimedini-starkware marked this pull request as draft April 15, 2025 14:31
@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch 2 times, most recently from 1bbf23b to 9b9ee5f Compare April 15, 2025 16:14
@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch 2 times, most recently from e908e85 to 2c846ea Compare April 17, 2025 13:57
@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch from 2c846ea to 389d706 Compare April 18, 2025 10:52
@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch from 389d706 to d157ddd Compare April 22, 2025 13:10
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review April 22, 2025 13:13
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)


taplo.toml line 5 at r2 (raw file):

  "crates/blockifier/cairo_native/**/*.toml",
  "crates/native_blockifier/.cargo/config.toml",
  "sequencer_venv/**/*.toml",

Why should there be a toml in here?

Code quote:

"sequencer_venv/**/*.toml",

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 14 files at r1, all commit messages.
Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @nimrod-starkware)


taplo.toml line 5 at r2 (raw file):

  "crates/blockifier/cairo_native/**/*.toml",
  "crates/native_blockifier/.cargo/config.toml",
  "sequencer_venv/**/*.toml",

Why should there be a toml in here?

Code quote:

"sequencer_venv/**/*.toml",

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

Reviewed 13 of 14 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nimrod-starkware)


crates/blockifier_test_utils/src/cairo_compile.rs line 181 at r2 (raw file):

        Command::new("sh").arg("-c").arg("pip freeze | grep cairo-lang").output().unwrap().stdout;
    let cairo_lang_version_untrimmed = String::from_utf8(cairo_lang_version_output).unwrap();
    let cairo_lang_version = cairo_lang_version_untrimmed.trim();

If we're here

Suggestion:

    let cairo_lang_version = String::from_utf8(cairo_lang_version_output).unwrap().trim();

crates/blockifier_test_utils/src/cairo_compile.rs line 189 at r2 (raw file):

            panic!("Could not find cairo-lang in {:?}.", *CAIRO0_PIP_REQUIREMENTS_FILE)
        })
        .trim();

I think it's better for the "version" to only hold the version instead of the entire string, something like the suggestion.
And even better to push it into a Semver struct with semver::Version::parse(
The ends_with check below is a bit unclear - with this you can check equality.

Suggestion:

      .trim().split("==").nth(1).expect("Missing == in cairo lang version string")

TzahiTaub
TzahiTaub approved these changes Apr 23, 2025
@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch from d157ddd to fc26ebe Compare April 23, 2025 14:43
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: 13 of 14 files reviewed, all discussions resolved (waiting on @nimrod-starkware and @TzahiTaub)


taplo.toml line 5 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Why should there be a toml in here?

when I remove this line, taplo fails; so, there is, somewhere in the venv. doesn't matter, we can ignore everything in this venv...


crates/blockifier_test_utils/src/cairo_compile.rs line 181 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

If we're here

nope:

temporary value dropped while borrowed

crates/blockifier_test_utils/src/cairo_compile.rs line 189 at r2 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

I think it's better for the "version" to only hold the version instead of the entire string, something like the suggestion.
And even better to push it into a Semver struct with semver::Version::parse(
The ends_with check below is a bit unclear - with this you can check equality.

the split("==") forces the version to always be equality... which is not actually a bad thing, so ok.
done

@dorimedini-starkware dorimedini-starkware force-pushed the 04-12-chore_blockifier_test_utils_ci_consolidate_cairo-lang_version_and_recompile_deprecated_contracts branch from fc26ebe to cefb40c Compare April 23, 2025 15:18
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


crates/blockifier_test_utils/src/cairo_compile.rs line 181 at r2 (raw file):

Previously, dorimedini-starkware wrote…

nope:

temporary value dropped while borrowed

:(

@dorimedini-starkware dorimedini-starkware added this pull request to the merge queue Apr 24, 2025
Merged via the queue into main with commit 33bcdd4 Apr 24, 2025
16 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 25, 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