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

ci: support Cairo1 recompile in CI #1877

Open
wants to merge 1 commit into
base: dori/add-cairo1-version-file-for-ci
Choose a base branch
from

Conversation

dorimedini-starkware
Copy link
Collaborator

@dorimedini-starkware dorimedini-starkware commented May 7, 2024

This change is Reviewable

@dorimedini-starkware dorimedini-starkware self-assigned this May 7, 2024
@codecov-commenter
Copy link

codecov-commenter commented May 7, 2024

Codecov Report

Attention: Patch coverage is 0% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 76.47%. Comparing base (103ae1b) to head (cf07a5d).

Files Patch % Lines
crates/blockifier/src/test_utils/contracts.rs 0.00% 5 Missing ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           dori/add-cairo1-version-file-for-ci    #1877      +/-   ##
=======================================================================
- Coverage                                76.48%   76.47%   -0.01%     
=======================================================================
  Files                                       61       61              
  Lines                                     8770     8771       +1     
  Branches                                  8770     8771       +1     
=======================================================================
  Hits                                      6708     6708              
- Misses                                    1622     1623       +1     
  Partials                                   440      440              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dorimedini-starkware dorimedini-starkware changed the base branch from main to dori/add-cairo1-version-file-for-ci May 7, 2024 10:23
@dorimedini-starkware dorimedini-starkware changed the title Dori/cairo1 recompile ci support ci: support Cairo1 recompile in CI May 7, 2024
@dorimedini-starkware dorimedini-starkware force-pushed the dori/add-cairo1-version-file-for-ci branch from 0d4274f to de67951 Compare May 7, 2024 10:26
@dorimedini-starkware dorimedini-starkware force-pushed the dori/cairo1-recompile-ci-support branch 3 times, most recently from 0999c4d to f54a069 Compare May 7, 2024 10:49
@dorimedini-starkware dorimedini-starkware force-pushed the dori/add-cairo1-version-file-for-ci branch from de67951 to 103ae1b Compare May 7, 2024 11:05
@dorimedini-starkware dorimedini-starkware force-pushed the dori/cairo1-recompile-ci-support branch 3 times, most recently from b3c1d93 to 7c929a4 Compare May 7, 2024 11:56
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: 0 of 19 files reviewed, all discussions resolved (waiting on @dorimedini-starkware)


.github/workflows/compiled_cairo.yml line 1 at r2 (raw file):

name: Verify Cairo File Dependencies

this was a mistake before, IIUC. now the workflows area doesn't show CI twice

Code quote:

Verify Cairo File Dependencies

@dorimedini-starkware dorimedini-starkware force-pushed the dori/cairo1-recompile-ci-support branch 2 times, most recently from 7952832 to 22d2764 Compare May 7, 2024 14:50
Copy link
Collaborator

@giladchase giladchase 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 17 of 19 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @OriStarkware and @Yoni-Starkware)


.github/workflows/compiled_cairo.yml line 1 at r2 (raw file):

Previously, dorimedini-starkware wrote…

this was a mistake before, IIUC. now the workflows area doesn't show CI twice

OMG THANK YOU!
This was so annoying.


.github/workflows/compiled_cairo.yml line 48 at r3 (raw file):

          fetch-tags: tags
          ref: ${{ env.TAG }}
          path: 'cairo'

Do we want to add - uses: Swatinem/rust-cache@v2 to these entries?
Might save the cloning time, though it's probably negligible since this isn't run too often.

Code quote:

      # Checkout blockifier into a dedicated directory - technical requirement in order to be able to checkout `cairo` in a sibling directory.
      - name: checkout blockifier into `blockifier` directory.
        uses: actions/checkout@v4
        with:
          repository: 'starkware-libs/blockifier'
          path: 'blockifier'

      - name: Read Cairo1 Tag to compile contracts with from Blockifier.
        id: read-tag
        # GITHUB_ENV is a variable github allocates for dynamic stuff inside workflow like our usage, name not customizable.
        run: echo "TAG=$(cat blockifier/crates/blockifier/tests/cairo1_compiler_tag.txt)" >> $GITHUB_ENV

      - name: Read legacy Cairo1 Tag to compile the legacy contract with.
        id: read-legacy-tag
        # GITHUB_ENV is a variable github allocates for dynamic stuff inside workflow like our usage, name not customizable.
        run: echo "LEGACY_TAG=$(cat blockifier/crates/blockifier/tests/legacy_cairo1_compiler_tag.txt)" >> $GITHUB_ENV

      - name: checkout cairo1 repo in order to compile cairo1 contracts.
        uses: actions/checkout@v4
        with:
          repository: 'starkware-libs/cairo'
          fetch-tags: tags
          ref: ${{ env.TAG }}
          path: 'cairo'

crates/blockifier/tests/feature_contracts_compatibility_test.rs line 147 at r3 (raw file):

            } else {
                std::env::var("CI").is_err() || !contract.is_legacy()
            })

Totally optional, only if you feel like it,

Suggestion:

    let fix_features = env::var("FIX_FEATURE_TEST").is_ok();
    let test_legacy_only = env::var("LEGACY").is_ok();
    let not_in_ci = env::var("CI").is_err();

    let contracts_to_test = FeatureContract::all_feature_contracts().filter(|contract| {
        // If called with LEGACY environment variable set, only test legacy contracts (from CI
        // or not).
        // If tested from the CI *without* the LEGACY environment variable set, test only
        // non-legacy contracts of the respective cairo version.
        // If not tested from CI, test all contracts of the requested cairo version.
        contract.cairo_version() == cairo_version
            && (if test_legacy_only {
                contract.is_legacy()
            } else {
                not_in_ci || !contract.is_legacy()
            })

@dorimedini-starkware dorimedini-starkware force-pushed the dori/cairo1-recompile-ci-support branch 4 times, most recently from 979d669 to 68f63b9 Compare May 8, 2024 15:34
Signed-off-by: Dori Medini <dori@starkware.co>
Copy link
Collaborator

@giladchase giladchase 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 r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @OriStarkware and @Yoni-Starkware)


crates/blockifier/tests/feature_contracts_compatibility_test.rs line 147 at r3 (raw file):

Previously, giladchase wrote…

Totally optional, only if you feel like it,

🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants