-
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): verify local cairo repo state before compiling #194
chore(blockifier): verify local cairo repo state before compiling #194
Conversation
0516f8b
to
249b887
Compare
e3b2330
to
e62e313
Compare
249b887
to
d870356
Compare
e62e313
to
4fe6734
Compare
d870356
to
13acfad
Compare
4fe6734
to
9facef4
Compare
13acfad
to
aa0605e
Compare
9facef4
to
0d099c5
Compare
aa0605e
to
9ae0de0
Compare
0d099c5
to
a12e845
Compare
9ae0de0
to
66e9a05
Compare
a12e845
to
d2dd1b8
Compare
66e9a05
to
8cbaa12
Compare
d2dd1b8
to
10d4789
Compare
8cbaa12
to
1fe0755
Compare
55fda8f
to
ca9ccb1
Compare
55907ae
to
031e4da
Compare
ca9ccb1
to
3ec4eb7
Compare
031e4da
to
6e5f836
Compare
3ec4eb7
to
ef4ccf9
Compare
6e5f836
to
9942ae2
Compare
ef4ccf9
to
7ce475e
Compare
9942ae2
to
0895bc5
Compare
7ce475e
to
cbb6cfe
Compare
0895bc5
to
388820c
Compare
cbb6cfe
to
28ecdcd
Compare
388820c
to
7a76151
Compare
28ecdcd
to
9b19d4e
Compare
7a76151
to
83dc987
Compare
9b19d4e
to
aa4a115
Compare
83dc987
to
b29a56b
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: all files reviewed, 2 unresolved discussions (waiting on @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @nimrod-starkware)
crates/blockifier/src/test_utils/cairo_compile.rs
line 183 at r4 (raw file):
"Cannot verify Cairo1 contracts, Cairo repo not found at {0}.\n\ Please run:\n\ git clone https://github.com/starkware-libs/cairo {0} && git -C {0} checkout {1}\n\
The script checkout in the next stage, I think this can be removed
crates/blockifier/src/test_utils/cairo_compile.rs
line 205 at r4 (raw file):
"--verify", &tag, ]));
What does this add to the previous action (I mean, if the tag doesn't exist, we will fail in the checkout stage)?
Code quote:
// Verify that the checked out tag is as expected.
run_and_verify_output(Command::new("git").args([
"-C",
cairo_repo_path.to_str().unwrap(),
"rev-parse",
"--verify",
&tag,
]));
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 205 at r4 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
What does this add to the previous action (I mean, if the tag doesn't exist, we will fail in the checkout stage)?
in case we somehow "silently" fail the checkout and not catch the error.
may be redundant, but when we parallelize this test (takes forever to sequencially compile all contracts...) it will be useful to split checkout / verify checkout phases anyway
aa4a115
to
cf43b14
Compare
b29a56b
to
9d6f7ff
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 r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @AvivYossef-starkware, @meship-starkware, and @nimrod-starkware)
crates/blockifier/src/test_utils/cairo_compile.rs
line 205 at r4 (raw file):
Previously, dorimedini-starkware wrote…
in case we somehow "silently" fail the checkout and not catch the error.
may be redundant, but when we parallelize this test (takes forever to sequencially compile all contracts...) it will be useful to split checkout / verify checkout phases anyway
Can't say I understand (to parallelize I assume you need to run different tag compilation sequentially and parallelize only contracts using the same tag. In this case you can make one checkout, parallel compilations, and when finished - new checkout, (parallel) compilation etc. Don't see where the verify helps here. But unblocking
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 @nimrod-starkware)
crates/blockifier/src/test_utils/cairo_compile.rs
line 205 at r4 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Can't say I understand (to parallelize I assume you need to run different tag compilation sequentially and parallelize only contracts using the same tag. In this case you can make one checkout, parallel compilations, and when finished - new checkout, (parallel) compilation etc. Don't see where the verify helps here. But unblocking
exactly.
I can checkout once, and verify in every thread
Merge activity
|
9d6f7ff
to
e31d80c
Compare
This change is