-
Notifications
You must be signed in to change notification settings - Fork 6
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: Add release workflow #48
Conversation
I believe that we should handle the proper versioning of the crates that are meant to be used as dependencies, so I wouldn't add them in my opinion. |
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.
LGTM!
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.
This looks great! Just one comment on maintenability of a list of crates distinct from the workspace.
.github/workflows/release-pr.yml
Outdated
runs-on: ubuntu-latest | ||
env: | ||
# Crates from which the version will be bumped | ||
CRATES: './cli, ./core, ./derive, ./eval, ./helper, ./primitives, ./prover, ./recursion/circuit, ./recursion/compiler, ./recursion/core, ./recursion/gnark-ffi, ./recursion/program, ./sdk, ./tutorials, ./zkvm/entrypoint, ./zkvm/precompiles' |
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.
This is going to be a pain to maintain.
How about :
- removing the glob in the
members
field of the[workspace]
section in the top-levelCargo.toml
, - installing
tomlq
in the CI machines using cargo-install, - do something like:
huitseeker@MacBook-Pro.localdomain➜~/tmp/sphinx(dev)» for i in $(tomlq workspace.members -f Cargo.toml|jq -r '.[] | "\"" + . + "\","'); do echo $i; done
"cli",
"core",
"derive",
"eval",
"helper",
"primitives",
"prover",
"recursion/circuit",
"recursion/compiler",
"recursion/core",
"recursion/gnark-ffi",
"recursion/program",
"sdk",
"tutorials",
"zkvm/*",
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.
Great idea, I used this up to date fork instead: https://github.com/cryptaliagy/tq-rs
f564136
to
fb020b6
Compare
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.
LGTM, NIT about referring to environment variable in the same way ($MY_VAR
vs ${{ env.MY_VAR }}
Adds a release workflow based on argumentcomputer/zk-light-clients#19 and argumentcomputer/aptos-core#5
TODO:
Do we also want to bump theCargo.toml
s fortests
,examples
, andcli/src/assets
?