Updated CI to target rv64im#26
Conversation
…rsion - Switch CI no-std job to a custom rv64im target spec with +a enabled - Force transitive radium dependency to commit 723bed5a (conditional atomics) - Simplify check_no_std.sh to use nightly + custom target spec - Remove unused bal-devnet revm/alloy patches from Cargo.toml
The dtolnay action cannot install a custom JSON target via rustup. Instead, install rust-src and let -Zbuild-std compile core/alloc from source.
|
@Andrurachi please take a look |
|
I'll drop my review on this tomorrow. Thanks for tagging me |
Andrurachi
left a comment
Andrurachi
left a comment
There was a problem hiding this comment.
I left a few inline comments regarding the radium patch and the lockfile. Also, one quick question that GitHub wouldn't let me leave inline: it looks like a bump to the testing/ef-specs submodule slipped into the diff. Was that intentional for this PR?
Let me know your thoughts :)
| "cpu": "generic-rv64", | ||
| "data-layout": "e-m:e-p:64:64-i64:64-i128:128-n32:64-S128", | ||
| "eh-frame-header": false, | ||
| "features": "+m,+a", |
There was a problem hiding this comment.
I know the commit message mentions this +a flag is temporary, but I checked out the radium commit (723bed5a) being patched in Cargo.toml. Just like Issue #4 warned, that commit successfully disabled atomics for riscv32im, but it completely missed riscv64im.
Because of this, I think patching this specific radium commit doesn't actually buy us anything for our 64-bit target (which I'm guessing is why this +a flag was still needed). Since it doesn't affect rv64im, should we just drop the radium patch from this PR entirely and include fixing upstream radium in your list of follow-up issues?
There was a problem hiding this comment.
Right, I missed that.
Yea, lets exclude the patch for now, and focus on updating the upstream deps.
| [[package]] | ||
| name = "alloy-eip7928" | ||
| version = "0.3.5" | ||
| version = "0.3.7" |
There was a problem hiding this comment.
Looking at the commit history, it seems like some local revm/alloy devnet patches were removed, which probably triggered a recalculation of dependencies. Do you think we should revert those unrelated lockfile changes for this PR?
There was a problem hiding this comment.
Just checked. cargo update was performed. It is needed after introducing patch to radium.
warning: patch `radium v0.7.1 (https://github.com/ferrilab/radium?rev=723bed5a#723bed5a)` was not used in the crate graph
help: Check that the patched package version and available features are compatible
with the dependency requirements. If the patch has a different version from
what is locked in the Cargo.lock file, run `cargo update` to use the new
version. This may also occur with an optional dependency that is not enabled.
|
@Andrurachi Can you take it from here? I can't commit to this currently, due to other obligations |
| ## `no_std` | ||
|
|
||
| The `stateless` crate is `#![no_std]` compatible and builds for RISC-V targets (`riscv32imac-unknown-none-elf`), making it suitable for use in zkVM environments. | ||
| The `stateless` crate is `#![no_std]` compatible and builds for the 64‑bit RV64IM target (`riscv64im-unknown-none-elf`), making it suitable for use in zkVM environments. |
There was a problem hiding this comment.
should this say ima for now or am I missing something?
| target=support/targets/riscv64im-a-target.json | ||
|
|
||
| cmd=(cargo +stable build --no-default-features --target "$target" -p stateless) | ||
| cmd=(cargo +nightly build --no-default-features -Zjson-target-spec -Zbuild-std=core,alloc --target "$target" -p stateless) |
There was a problem hiding this comment.
maybe add lower-atomics flag? (actually I added this to the issue, so assuming you know)
|
@lazovicff No problem. Thanks for getting the groundwork started. I'll create a fresh PR with the lower-atomic approach Kev also suggested |
|
superseded by #27 |
4 participants
Switch
no_stdCI to a 64-bit RV64IM target with temporary+a(atomics), using a custom target spec and nightly Rust with-Zbuild-std, so the build passes whileserde_coreandtracing-corestill requirealloc::sync.Patch
radiumto a specific commit (723bed5a) that conditionally disables atomics when the target doesn't support them, applied globally via[patch.crates-io].