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

chore(blockifier): verify cairo compiler repo (with correct tag) #183

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Jul 29, 2024

This change is Reviewable

Copy link
Contributor

@nimrod-starkware nimrod-starkware 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 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @TzahiTaub)


crates/blockifier/src/test_utils/cairo_compile.rs line 68 at r2 (raw file):

}

/// Run a command, assert exit code is zero (otherwise panic with stderr output).

Suggestion:

/// Runs a command. If it has succeeded, it returns the command's output; otherwise, it panics with stderr output.

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from 852ec7b to cfeb968 Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from d7806fc to b7301ad Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from cfeb968 to af3aebb Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from b7301ad to 8e54eec Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from af3aebb to 9c74f42 Compare July 31, 2024 16:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 8e54eec to 1927450 Compare July 31, 2024 16:51
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, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, @nimrod-starkware, and @TzahiTaub)


crates/blockifier/src/test_utils/cairo_compile.rs line 68 at r2 (raw file):

}

/// Run a command, assert exit code is zero (otherwise panic with stderr output).

Done.

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 1927450 to 5d0ac45 Compare July 31, 2024 16:54
Copy link
Contributor

@nimrod-starkware nimrod-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 0 of 1 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @TzahiTaub)

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from 9c74f42 to f5b23c7 Compare August 1, 2024 08:50
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 0e1981a to e91cecc Compare August 1, 2024 08:53
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.

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @nimrod-starkware)


crates/blockifier/src/test_utils/cairo_compile.rs line 9 at r3 (raw file):

const CAIRO0_PIP_REQUIREMENTS_FILE: &str = "tests/requirements.txt";
const LOCAL_CAIRO1_REPO_RELATIVE_PATH: &str = "../../../cairo";

This is too strict, please replace with env vars to allow a a hierarchy where the sequencer and cairo dirs aren't siblings, to work.

Code quote:

const LOCAL_CAIRO1_REPO_RELATIVE_PATH: &str = "../../../cairo";

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: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @meship-starkware, @nimrod-starkware, and @TzahiTaub)


crates/blockifier/src/test_utils/cairo_compile.rs line 9 at r3 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

This is too strict, please replace with env vars to allow a a hierarchy where the sequencer and cairo dirs aren't siblings, to work.

Done. Now can be overridden by env var

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from caacae7 to fa5d93a Compare August 1, 2024 15:57
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from e91cecc to 1a2bc9e Compare August 1, 2024 15:57
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from fa5d93a to eac83c2 Compare August 6, 2024 12:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 1a2bc9e to c250527 Compare August 6, 2024 12:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from eac83c2 to 177370c Compare August 6, 2024 12:28
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from c250527 to a65c413 Compare August 6, 2024 12:29
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from 177370c to 8937375 Compare August 7, 2024 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from a65c413 to 378e67b Compare August 7, 2024 14:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from 8937375 to 1b47e77 Compare August 7, 2024 16:16
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 378e67b to e57ad1c Compare August 7, 2024 16:16
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review August 8, 2024 09:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from 1b47e77 to b45df76 Compare August 11, 2024 07:45
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from e57ad1c to 1fd4711 Compare August 11, 2024 07:45
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from b45df76 to c095163 Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 1fd4711 to 678e67e Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation branch from c095163 to 1f7061e Compare August 11, 2024 09:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 678e67e to 469c6a8 Compare August 11, 2024 09:35
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 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @meship-starkware)


crates/blockifier/src/test_utils/cairo_compile.rs line 62 at r6 (raw file):

/// Returns the path to the local Cairo1 compiler repository.
/// Returns <sequencer_crate_root>/<RELATIVE_PATH_TO_CAIRO_REPO>, where the relative path can be

The Sequencer is not a crate, right?

Suggestion:

<sequencer_repo_root>

@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from 469c6a8 to b4e9649 Compare August 11, 2024 12:58
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 r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)

Copy link
Collaborator Author

dorimedini-starkware commented Aug 11, 2024

Merge activity

@dorimedini-starkware dorimedini-starkware changed the base branch from 07-29-feat_blockifier_verify_cairo-lang_version_before_cairo0_compilation to graphite-base/183 August 11, 2024 15:52
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/183 to main August 11, 2024 16:00
@dorimedini-starkware dorimedini-starkware force-pushed the 07-29-chore_blockifier_verify_cairo_compiler_repo_with_correct_tag_ branch from b4e9649 to 840ecbf Compare August 11, 2024 16:01
@dorimedini-starkware dorimedini-starkware merged commit 0657397 into main Aug 11, 2024
14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
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.

3 participants