Add CLI option to specify entrypoint #220

Bernardstanislas · 2022-06-30T21:33:12Z

Configurable entrypoint

Description

As described in #180, it might be useful to be able to run another function than the main function as an entrypoint.

Checklist

Linked to Github Issue
Unit tests added
Integration tests added.

Juan-M-V · 2022-07-01T13:05:09Z

Thank you for taking the time implementing a new feature. The team and I will review the RP

src/main.rs

Oppen · 2022-07-01T14:36:58Z

src/serde/deserialize_program.rs

    let program_json: ProgramJson = deserialize_program_json(path)?;
+
+    let main_pc = match program_json


For consistency this could be named entrypoint_pc.

Oppen · 2022-07-01T14:39:58Z

Hi @Bernardstanislas. Thanks for sending this feature. I'd suggest you add a test that proves the right entrypoint is called. It could simply be copy-pasting some of the examples with a renamed main function instead, or replacing in the JSON before use in memory.

Bernardstanislas · 2022-07-07T14:38:57Z

Hi @Bernardstanislas. Thanks for sending this feature. I'd suggest you add a test that proves the right entrypoint is called. It could simply be copy-pasting some of the examples with a renamed main function instead, or replacing in the JSON before use in memory.

For sure, I'm a bit busy these days but I'm not forgetting this PR :)

Oppen · 2022-07-12T14:08:08Z

For sure, I'm a bit busy these days but I'm not forgetting this PR :)

I've got a bit of time for this, would you like it if I made a PR against your branch with a test?

Oppen · 2022-07-13T18:49:25Z

Hi @Bernardstanislas, I just sent you a PR against your branch with proposed changes.

Bernardstanislas · 2022-07-13T19:05:09Z

Hi @Bernardstanislas, I just sent you a PR against your branch with proposed changes.

Thanks a lot, just merged your PR

Oppen · 2022-07-13T20:20:19Z

Hmmm, it looks like I didn't include the test in the PR for some reason 🤦
It's nearly end of day for me, is it OK for you if I send a new one tomorrow?

Oppen · 2022-07-25T14:25:50Z

Hi @Bernardstanislas. Did you find the time to look into the new PR I sent you? Sorry for not mentioning it here, I assumed you'd get notified by GitHub. Then I remembered those tend to be rather easy to miss (it happens to me a lot).

Bernardstanislas · 2022-07-25T16:06:53Z

Hi @Bernardstanislas. Did you find the time to look into the new PR I sent you? Sorry for not mentioning it here, I assumed you'd get notified by GitHub. Then I remembered those tend to be rather easy to miss (it happens to me a lot).

Wow I'm really sorry @Oppen I did actually miss it. So if I understand correctly, you need to include the tests and we're good to go?

Oppen · 2022-07-25T22:03:56Z

Wow I'm really sorry @Oppen I did actually miss it. So if I understand correctly, you need to include the tests and we're good to go?

Yes, it's included in the PR. I'm not sure if any new merge conflicts were introduced in the meantime tho, but I expect not.

Bernardstanislas · 2022-08-11T15:32:23Z

@Oppen I am mot sure I understood correctly, do we need your rebase to be able to merge this PR? Do you need something on my side? Sorry for the seemingly misunderstanding.

Juan-M-V · 2022-08-12T13:56:20Z

Hi @Bernardstanislas! @Oppen is on vacation for a few weeks. What we need is for you to accept his PR with the tests into your PR.
After that, there are conflicts to resolve with the main branch and we should be good to go. If you could look into solving the conflicts that would be awesome, since I don't know if we can do it for you on a timely manner.

Bernardstanislas · 2022-08-14T21:58:33Z

Hi @Juan-M-V , I merged the PR on my branch and it is now up to date and rebased!

Juan-M-V · 2022-08-16T12:31:42Z

Perfect! I'll run the tests and talk with the rest of the team to merge your changes

codecov-commenter · 2022-08-16T12:53:02Z

Codecov Report

Merging #220 (0d0c62e) into main (b805afa) will decrease coverage by 0.01%.
The diff coverage is 92.30%.

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   97.88%   97.87%   -0.02%     
==========================================
  Files          44       44              
  Lines       11600    11619      +19     
==========================================
+ Hits        11355    11372      +17     
- Misses        245      247       +2

Impacted Files	Coverage Δ
src/main.rs	`5.00% <0.00%> (-0.56%)`	⬇️
src/cairo_run.rs	`86.06% <100.00%> (+1.24%)`	⬆️
src/serde/deserialize_program.rs	`91.80% <100.00%> (+0.32%)`	⬆️
src/types/program.rs	`100.00% <100.00%> (ø)`
tests/bitwise_test.rs	`100.00% <100.00%> (ø)`
tests/cairo_run_test.rs	`98.40% <100.00%> (ø)`
tests/pedersen_test.rs	`100.00% <100.00%> (ø)`
tests/struct_test.rs	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Bernardstanislas force-pushed the customize-entrypoint branch 2 times, most recently from a7f8471 to 54a1e80 Compare July 1, 2022 12:13

Juan-M-V reviewed Jul 1, 2022

View reviewed changes

src/main.rs Show resolved Hide resolved

Juan-M-V approved these changes Jul 1, 2022

View reviewed changes

Oppen reviewed Jul 1, 2022

View reviewed changes

Bernardstanislas mentioned this pull request Aug 11, 2022

Execute a cairo file with Cleopatra open-dust/cairo-foundry#6

Closed

Bernardstanislas force-pushed the customize-entrypoint branch from e9e6183 to e3977d8 Compare August 14, 2022 21:56

Bernardstanislas and others added 4 commits August 16, 2022 15:48

Add CLI option to specify entrypoint

db84068

Add test for custom entry point

484ec5d

Rename variable

a3b3f06

Fix broken tests adding main entrypoint

0d0c62e

entropidelic force-pushed the customize-entrypoint branch from 7b65817 to 0d0c62e Compare August 16, 2022 18:58

Dummy commit

c9a01b8

igaray approved these changes Aug 17, 2022

View reviewed changes

igaray merged commit 8328185 into lambdaclass:main Aug 17, 2022

Bernardstanislas deleted the customize-entrypoint branch August 17, 2022 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CLI option to specify entrypoint #220

Add CLI option to specify entrypoint #220

Bernardstanislas commented Jun 30, 2022

Juan-M-V commented Jul 1, 2022

Oppen Jul 1, 2022

unbalancedparentheses Jul 1, 2022 •

edited

Loading

Oppen commented Jul 1, 2022

Bernardstanislas commented Jul 7, 2022

Oppen commented Jul 12, 2022

Oppen commented Jul 13, 2022

Bernardstanislas commented Jul 13, 2022

Oppen commented Jul 13, 2022

Oppen commented Jul 25, 2022

Bernardstanislas commented Jul 25, 2022

Oppen commented Jul 25, 2022

Bernardstanislas commented Aug 11, 2022

Juan-M-V commented Aug 12, 2022

Bernardstanislas commented Aug 14, 2022

Juan-M-V commented Aug 16, 2022

codecov-commenter commented Aug 16, 2022 •

edited

Loading

		let program_json: ProgramJson = deserialize_program_json(path)?;

		let main_pc = match program_json

Add CLI option to specify entrypoint #220

Add CLI option to specify entrypoint #220

Conversation

Bernardstanislas commented Jun 30, 2022

Configurable entrypoint

Description

Checklist

Juan-M-V commented Jul 1, 2022

Oppen Jul 1, 2022

Choose a reason for hiding this comment

unbalancedparentheses Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Oppen commented Jul 1, 2022

Bernardstanislas commented Jul 7, 2022

Oppen commented Jul 12, 2022

Oppen commented Jul 13, 2022

Bernardstanislas commented Jul 13, 2022

Oppen commented Jul 13, 2022

Oppen commented Jul 25, 2022

Bernardstanislas commented Jul 25, 2022

Oppen commented Jul 25, 2022

Bernardstanislas commented Aug 11, 2022

Juan-M-V commented Aug 12, 2022

Bernardstanislas commented Aug 14, 2022

Juan-M-V commented Aug 16, 2022

codecov-commenter commented Aug 16, 2022 • edited Loading

Codecov Report

unbalancedparentheses Jul 1, 2022 •

edited

Loading

codecov-commenter commented Aug 16, 2022 •

edited

Loading