Skip to content
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): recompile Cairo1 in the CI #197

Merged

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented Jul 30, 2024

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this Jul 30, 2024
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from e98af69 to 801ff61 Compare July 30, 2024 11:31
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 9a950bf to 25de71b Compare July 30, 2024 13:22
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 801ff61 to 8788493 Compare July 30, 2024 13:22
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 25de71b to ca8d23b Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 8788493 to 6e3d149 Compare July 31, 2024 16:34
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from ca8d23b to 6799aa3 Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 6e3d149 to d5ef49f Compare July 31, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 6799aa3 to dd8634f Compare July 31, 2024 16:52
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from d5ef49f to aafebad Compare July 31, 2024 16:52
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from dd8634f to 9782b30 Compare July 31, 2024 16:54
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from aafebad to 16edf9a Compare July 31, 2024 16:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 9782b30 to e6f7a3f Compare August 1, 2024 08:50
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 16edf9a to 1f3a521 Compare August 1, 2024 08:50
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from e6f7a3f to af5515a Compare August 1, 2024 08:53
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 1f3a521 to 97e4c2f Compare August 1, 2024 08:53
@dorimedini-starkware dorimedini-starkware marked this pull request as ready for review August 8, 2024 09:51
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from f282206 to db0b683 Compare August 8, 2024 12:35
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 5d3dbb0 to afc8510 Compare August 11, 2024 07:45
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from db0b683 to fa2914a Compare August 11, 2024 07:46
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from afc8510 to e87bd1c Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from fa2914a to 9e7cac1 Compare August 11, 2024 08:55
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from e87bd1c to 9b0ad2b Compare August 11, 2024 09:35
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 9e7cac1 to 4993d6c Compare August 11, 2024 09:35
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 2 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aner-starkware, @AvivYossef-starkware, @dorimedini-starkware, @meship-starkware, and @noaov1)


.github/workflows/blockifier_compiled_cairo.yml line 14 at r11 (raw file):

      - 'crates/blockifier/feature_contracts/**'
      - 'crates/blockifier/src/test_utils/cairo_compile.rs'
      - 'crates/blockifier/tests/**'

Shouldn't this specify the two files currently in the dir instead? (So future files won't trigger this action)

Code quote:

 'crates/blockifier/tests/**'

.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

        with:
          repository: 'starkware-libs/cairo'
          fetch-depth: 0

By the docs

    # Whether to fetch tags, even if fetch-depth > 0.
    # Default: false
    fetch-tags: ''

we only need the tags don't we?

Suggestion:

 fetch-depth: 1

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 23 at r11 (raw file):

// Then, run the FIX_COMMAND above.

// To test Cairo1 feature contracts, first clone the Cairo repo and checkout the required tag.

Suggestion:

To fix Cairo1

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a 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, 4 unresolved discussions (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, @noaov1, and @TzahiTaub)


.github/workflows/blockifier_compiled_cairo.yml line 14 at r11 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

Shouldn't this specify the two files currently in the dir instead? (So future files won't trigger this action)

Done.


.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

By the docs

    # Whether to fetch tags, even if fetch-depth > 0.
    # Default: false
    fetch-tags: ''

we only need the tags don't we?

Done. thanks!


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 23 at r11 (raw file):

// Then, run the FIX_COMMAND above.

// To test Cairo1 feature contracts, first clone the Cairo repo and checkout the required tag.

Done.

@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-chore_blockifier_add_is_legacy_method branch from 9b0ad2b to 1c94532 Compare August 11, 2024 12:58
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 4993d6c to feb2d27 Compare August 11, 2024 12:58
Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a 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 @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)


.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

Previously, dorimedini-starkware wrote…

Done. thanks!

didn't work, reverting

@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from feb2d27 to f8f23d8 Compare August 11, 2024 13:47
Copy link
Contributor

@TzahiTaub TzahiTaub left a 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 r13.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)


.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

Previously, dorimedini-starkware wrote…

didn't work, reverting

OK, pretty annoying. Do you wanna try something else or give up?
We can try:

- uses: actions/checkout@v4
  with:
    fetch-depth: 0
    filter: object:type=tag

(reached to here from the above link)

Copy link
Contributor

@TzahiTaub TzahiTaub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from f8f23d8 to 2bae019 Compare August 11, 2024 15:06
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, @noaov1, and @TzahiTaub)


.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

Previously, TzahiTaub (Tzahi) wrote…

OK, pretty annoying. Do you wanna try something else or give up?
We can try:

- uses: actions/checkout@v4
  with:
    fetch-depth: 0
    filter: object:type=tag

(reached to here from the above link)

trying now...

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, @noaov1, and @TzahiTaub)


.github/workflows/blockifier_compiled_cairo.yml line 58 at r11 (raw file):

Previously, dorimedini-starkware wrote…

trying now...

nope

filter 'object:type' not supported

@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from 2bae019 to d7ec00a Compare August 11, 2024 15:09
@dorimedini-starkware dorimedini-starkware changed the base branch from 07-30-chore_blockifier_add_is_legacy_method to graphite-base/197 August 11, 2024 16:42
@dorimedini-starkware dorimedini-starkware force-pushed the 07-30-feat_blockifier_recompile_cairo1_in_the_ci branch from d7ec00a to 19b3812 Compare August 11, 2024 16:50
@dorimedini-starkware dorimedini-starkware changed the base branch from graphite-base/197 to main August 11, 2024 16:51
Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

Copy link
Collaborator Author

@dorimedini-starkware dorimedini-starkware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @aner-starkware from a discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @aner-starkware, @AvivYossef-starkware, @meship-starkware, and @noaov1)

@dorimedini-starkware dorimedini-starkware merged commit 6996784 into main Aug 11, 2024
39 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants