-
Notifications
You must be signed in to change notification settings - Fork 65
feat(apollo_starknet_os_program): compile programs asynchronously #5945
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(apollo_starknet_os_program): compile programs asynchronously #5945
Conversation
c040f8d to
d08a026
Compare
b5385b4 to
622d12c
Compare
d08a026 to
3433e05
Compare
622d12c to
a136d27
Compare
3433e05 to
9f38ec0
Compare
a136d27 to
32cb04d
Compare
9f38ec0 to
0c77b10
Compare
32cb04d to
aff8f88
Compare
c2b6c01 to
662b2c5
Compare
6b303d2 to
71f496a
Compare
662b2c5 to
8661ab4
Compare
71f496a to
b767ef3
Compare
8661ab4 to
ec5bbc7
Compare
b767ef3 to
090dda3
Compare
ec5bbc7 to
42908ca
Compare
090dda3 to
d6cae49
Compare
42908ca to
e05feb2
Compare
d6cae49 to
1d5a7c9
Compare
49c095f to
34f9837
Compare
34f9837 to
cb73d6f
Compare
TzahiTaub
left a comment
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 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
crates/apollo_starknet_os_program/build/main.rs line 20 at r2 (raw file):
task_set.spawn(compile_program::compile_and_output_program( out_dir.clone(), "starkware/starknet/core/os/os.cairo",
Please const the OS and Agg paths.
Lat on - I think we should have an enum with methods for the OS and Aggregator with all const data (that will also replace the ProgramToDump enum in the cli).
Code quote:
"starkware/starknet/core/os/os.cairo",
dorimedini-starkware
left a comment
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 @TzahiTaub)
crates/apollo_starknet_os_program/build/main.rs line 20 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Please const the OS and Agg paths.
Lat on - I think we should have an enum with methods for the OS and Aggregator with all const data (that will also replace theProgramToDumpenum in the cli).
this is the build script - so we can't use objects from here in external crates, and we can't use constants defined in the crate's lib.rs and it's imports.
a workaround is to define a paths.rs file with just string const definitions, declare it in lib.rs, and "paste" it here in the build script with include_str!; e.g. what we do here. Is that what you meant? or do you have another idea?
cb73d6f to
db02f56
Compare
TzahiTaub
left a comment
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 @dorimedini-starkware)
crates/apollo_starknet_os_program/build/main.rs line 20 at r2 (raw file):
Previously, dorimedini-starkware wrote…
this is the build script - so we can't
useobjects from here in external crates, and we can'tuseconstants defined in the crate'slib.rsand it's imports.
a workaround is to define apaths.rsfile with just string const definitions, declare it inlib.rs, and "paste" it here in the build script withinclude_str!; e.g. what we do here. Is that what you meant? or do you have another idea?
Nope, wasn't familiar with it, no need to make a const file. I do think you should const the paths in this file to appear in the head of the file.
BTW, the script has no rerun-if trigger which means it will be rerun for every change. Better aim it to the internal starknet dir.
dorimedini-starkware
left a comment
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 @TzahiTaub)
crates/apollo_starknet_os_program/build/main.rs line 20 at r2 (raw file):
Previously, TzahiTaub (Tzahi) wrote…
Nope, wasn't familiar with it, no need to make a const file. I do think you should const the paths in this file to appear in the head of the file.
BTW, the script has norerun-iftrigger which means it will be rerun for every change. Better aim it to the internalstarknetdir.
I think it should rerun on every change in the crate; if you change the recompilation logic you would also want to recompile, right?

No description provided.