-
Notifications
You must be signed in to change notification settings - Fork 25
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
feat(blockifier): verify cairo-lang version before Cairo0 compilation #182
feat(blockifier): verify cairo-lang version before Cairo0 compilation #182
Conversation
f5b36fa
to
01632aa
Compare
ae546f8
to
360c889
Compare
There was a problem hiding this 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 r1, 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 79 at r1 (raw file):
} /// Verifies that the required dependencies are available before compiling.
Suggestion:
/// Verifies that the required dependencies are available before compiling; panics if unavailable.
360c889
to
852ec7b
Compare
There was a problem hiding this 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 79 at r1 (raw file):
} /// Verifies that the required dependencies are available before compiling.
Done.
There was a problem hiding this 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, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @TzahiTaub)
01632aa
to
c3a164d
Compare
852ec7b
to
cfeb968
Compare
c3a164d
to
2f7cd95
Compare
cfeb968
to
af3aebb
Compare
2f7cd95
to
b2d7a30
Compare
af3aebb
to
9c74f42
Compare
b2d7a30
to
7601648
Compare
caacae7
to
fa5d93a
Compare
800878f
to
088fb75
Compare
fa5d93a
to
eac83c2
Compare
088fb75
to
99a29d0
Compare
eac83c2
to
177370c
Compare
99a29d0
to
c6d68f7
Compare
177370c
to
8937375
Compare
c6d68f7
to
4c186a0
Compare
8937375
to
1b47e77
Compare
There was a problem hiding this 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, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @meship-starkware)
a discussion (no related file):
What happens ATM if we have the package in the wrong version? We fail the comparison, and if we fix we fix the wrong local version we use (and hopefully fails the CI)
crates/blockifier/src/test_utils/cairo_compile.rs
line 98 at r2 (raw file):
cairo_lang_version.trim(), expected_cairo_lang_version.trim(), "cairo-lang not found. Please run:\npip3.9 install -r {}/{}\nthen rerun the test.",
We might have the package in the wrong version
Suggestion:
cairo-lang version {} not found...., expected_cairo_lang_version.trim().rsplit("=").next().unwrap()
There was a problem hiding this 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, and @TzahiTaub)
a discussion (no related file):
Previously, TzahiTaub (Tzahi) wrote…
What happens ATM if we have the package in the wrong version? We fail the comparison, and if we fix we fix the wrong local version we use (and hopefully fails the CI)
The CI installs whatever is in the requirements.txt file, if that version is "wrong" there's nothing we can check
crates/blockifier/src/test_utils/cairo_compile.rs
line 98 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
We might have the package in the wrong version
better?
4c186a0
to
d8b8e73
Compare
1b47e77
to
b45df76
Compare
d8b8e73
to
9e12d56
Compare
b45df76
to
c095163
Compare
There was a problem hiding this 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 r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, and @meship-starkware)
a discussion (no related file):
Previously, dorimedini-starkware wrote…
The CI installs whatever is in the requirements.txt file, if that version is "wrong" there's nothing we can check
I meant if our installed local version doesn't match the requirements file.
crates/blockifier/src/test_utils/cairo_compile.rs
line 98 at r2 (raw file):
Previously, dorimedini-starkware wrote…
better?
Yes, though when the package isn't installed, we'll get (installed version: )
then maybe if cairo_lang_version.is_empty() {"not installed"} else { cairo_lang_version}, and raise the
trim()` above as you did for the expected one.
There was a problem hiding this 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, @dorimedini-starkware, and @meship-starkware)
crates/blockifier/src/test_utils/cairo_compile.rs
line 98 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Yes, though when the package isn't installed, we'll get
(installed version: )
then maybeif cairo_lang_version.is_empty() {"not installed"} else { cairo_lang_version}, and raise the
trim()` above as you did for the expected one.
* and raise the
not a part of the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)
a discussion (no related file):
Previously, TzahiTaub (Tzahi) wrote…
I meant if our installed local version doesn't match the requirements file.
the fix command also compares versions (same cairo0_compile
is called), so you will also fail to fix
crates/blockifier/src/test_utils/cairo_compile.rs
line 98 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
*
and raise the
not a part of the code :)
done. however, can't raise the trim
("value dropped"). defined an intermediate var to call trim once
9e12d56
to
bfd77eb
Compare
c095163
to
1f7061e
Compare
There was a problem hiding this 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 r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware and @meship-starkware)
Merge activity
|
1f7061e
to
79f08a0
Compare
This change is