Skip to content

Conversation

@noam-starkware
Copy link
Contributor

@noam-starkware noam-starkware commented Dec 9, 2025

Type

  • feature
  • bugfix
  • dev (no functional changes, no API changes)
  • fmt (formatting, renaming)
  • build
  • docs
  • testing

Description

Breaking changes?

  • yes
  • no

This change is Reviewable

@noam-starkware noam-starkware force-pushed the noamp/cargo_compile_no_extra_flags branch from 3cce748 to cee4aa7 Compare December 10, 2025 07:54
@noam-starkware noam-starkware force-pushed the noamp/cargo_compile_no_extra_flags branch from cee4aa7 to 497dbc5 Compare December 10, 2025 08:45
Copy link
Contributor

@OmriEshhar1 OmriEshhar1 left a comment

Choose a reason for hiding this comment

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

@OmriEshhar1 reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @noam-starkware)


.github/workflows/upload_artifacts_workflow.yml line 94 at r1 (raw file):

          destination: "stwo_run_and_prove_sapphirerapids_artifacts/${{ env.SHORT_HASH }}/release"

      - name: Build stwo_run_and_prove for graniterapids architecture

this granite-rapids architecture, don't you need to add it in main repo as well?

Copy link
Contributor Author

@noam-starkware noam-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 @gilbens-starkware and @OmriEshhar1)


.github/workflows/upload_artifacts_workflow.yml line 94 at r1 (raw file):

Previously, OmriEshhar1 wrote…

this granite-rapids architecture, don't you need to add it in main repo as well?

Yes, but when I add it to the main repo the compiled binary (for the relevant proving-utils ref that we use) must exist in storage. That's why I first need to merge this PR to main, which will trigger the auto build, and once that completes I can add the new binary in the main repo.

A lot more words in case you're bored:

  1. In the main repo, the tool that loads binaries has two options. It looks up the binary name and the repo+hash we reference. If the binary of that hash exists in storage, it pulls it and continues. Otherwise, it builds it on the fly and continues. The build-on-the-fly method doesn't work for all CI tests so it only helps for local tests (which I think is a good thing, you definitely don't want to reference a non-existing binary in a merged commit in the main repo). This is why the order is always to merge the PR that builds the binary in proving-utils and then add it in the main repo.
  2. For this case specifically, I already have a PR in the main repo that and I tested to make sure the binary is actually called and works (it does, with good times as well). That PR references a WIP branch in proving-utils that I created to "force" an auto build of the binaries before merging to main here.

Copy link
Contributor

@OmriEshhar1 OmriEshhar1 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @gilbens-starkware)

@noam-starkware noam-starkware merged commit 83e2822 into main Dec 14, 2025
7 checks passed
@noam-starkware noam-starkware deleted the noamp/cargo_compile_no_extra_flags branch December 14, 2025 10:01
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.

3 participants