-
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
chore(blockifier): fetch compiler repo version #181
chore(blockifier): fetch compiler repo version #181
Conversation
072267e
to
244da0c
Compare
f5b36fa
to
01632aa
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 07-29-chore_blockifier_move_contract_compilation_logic_to_separate_module_contracts.rs #181 +/- ##
=========================================================================================================================
Coverage ? 76.43%
=========================================================================================================================
Files ? 330
Lines ? 35011
Branches ? 35011
=========================================================================================================================
Hits ? 26762
Misses ? 5956
Partials ? 2293 ☔ View full report in Codecov by Sentry. |
244da0c
to
fe4e14c
Compare
01632aa
to
c3a164d
Compare
fe4e14c
to
271de7a
Compare
c3a164d
to
2f7cd95
Compare
271de7a
to
c3e54f4
Compare
2f7cd95
to
b2d7a30
Compare
c3e54f4
to
0dad397
Compare
b2d7a30
to
7601648
Compare
0dad397
to
e7bfa0d
Compare
7601648
to
67192dd
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.
Waiting on discussion about reading the cairo-lang-casm
version.
Reviewable status: 0 of 4 files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @noaov1)
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 4 files reviewed, 1 unresolved discussion (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)
crates/blockifier/src/test_utils/cairo_compile.rs
line 47 at r3 (raw file):
#[cached] /// Returns the version of the Cairo1 compiler* defined in the root Cargo.toml. pub fn cairo1_compiler_version() -> String {
Add a bash script to read the version from the toml for the CI, and add an assert that the bash output equals the value returned here.
Benchmark movements: |
125e131
to
73757fe
Compare
99a29d0
to
c6d68f7
Compare
Benchmark movements: |
73757fe
to
bcc5aef
Compare
c6d68f7
to
4c186a0
Compare
Benchmark movements: |
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 4 files at r1, 1 of 3 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)
crates/blockifier/src/test_utils/cairo_compile.rs
line 20 at r6 (raw file):
/// """ /// where `VERSION` can be a simple "x.y.z" version string or an object with a "version" field. #[derive(Debug, Serialize, Deserialize)]
I think
Suggestion:
/// where `VERSION` can be a simple "x.y.z" version string or an object with a "version" field.
#[derive(Debug, Serialize, Deserialize)]
crates/blockifier/src/test_utils/cairo_compile.rs
line 46 at r6 (raw file):
#[cached] /// Returns the version of the Cairo1 compiler* defined in the root Cargo.toml.
why the *
? If it's a reference to the head of the file, it isn't clear (replace with Cairo1 compiler "defined" (see doc at the head of the file) in the root...
or something).
Code quote:
compiler* defined
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, @noaov1, and @TzahiTaub)
crates/blockifier/src/test_utils/cairo_compile.rs
line 20 at r6 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
I think
this is not a docstring for the whole file, just for the struct(s) parsing the toml
crates/blockifier/src/test_utils/cairo_compile.rs
line 46 at r6 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
why the
*
? If it's a reference to the head of the file, it isn't clear (replace withCairo1 compiler "defined" (see doc at the head of the file) in the root...
or something).
better?
bcc5aef
to
11f058a
Compare
4c186a0
to
d8b8e73
Compare
Benchmark movements: |
11f058a
to
0c52d60
Compare
d8b8e73
to
9e12d56
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 4 of 4 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @noaov1)
crates/blockifier/src/test_utils/cairo_compile.rs
line 20 at r6 (raw file):
Previously, dorimedini-starkware wrote…
this is not a docstring for the whole file, just for the struct(s) parsing the toml
Just makes it look as docs for the first struct IMO ATM
crates/blockifier/src/test_utils/cairo_compile.rs
line 46 at r6 (raw file):
Previously, dorimedini-starkware wrote…
better?
Yep
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, @meship-starkware, and @noaov1)
crates/blockifier/src/test_utils/cairo_compile.rs
line 20 at r6 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Just makes it look as docs for the first struct IMO ATM
future PRs will add more to this module, this shouldn't be a module docstring.
want me to split the cairo-version-parsing stuff to a separate module and have this as a module-docstirng?
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, @meship-starkware, and @noaov1)
crates/blockifier/src/test_utils/cairo_compile.rs
line 20 at r6 (raw file):
Previously, dorimedini-starkware wrote…
future PRs will add more to this module, this shouldn't be a module docstring.
want me to split the cairo-version-parsing stuff to a separate module and have this as a module-docstirng?
No need.
0c52d60
to
7d376aa
Compare
9e12d56
to
bfd77eb
Compare
Benchmark movements: |
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, @meship-starkware, and @noaov1)
Merge activity
|
bfd77eb
to
9a24c7e
Compare
Benchmark movements: |
This change is