Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4587 +/- ##
==========================================
+ Coverage 34.12% 34.14% +0.02%
==========================================
Files 495 495
Lines 59062 59062
==========================================
+ Hits 20152 20169 +17
+ Misses 35348 35339 -9
+ Partials 3562 3554 -8 |
❌ 7 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
pmikolajczyk41
force-pushed
the
pmikolajczyk/nit-4729-stylus-compiler
branch
from
aeb9210 to
0d7b35e
Compare
pmikolajczyk41
force-pushed
the
pmikolajczyk/nit-4729-stylus-compiler
branch
from
0d7b35e to
9b27233
Compare
bragaigor
added
the
after-next-version
bragaigor
left a comment
bragaigor
left a comment
There was a problem hiding this comment.
LGTM overall, just some minor comments
There was a problem hiding this comment.
Wondering if it would be to our advantage to have someone review the changes in this branch even though it's not a PR?
There was a problem hiding this comment.
a very good idea - before I write a request to Succinct, just wanted to confirm: OffchainLabs/wasmer#34 is the full diff we want to consult, right?
There was a problem hiding this comment.
nitpick: I'm not sure if it's the same for you but locally in my local Mac, in order to make build.sh work I need to run make test-go-deps first before I run ./crates/sp1/build.sh. And at the of such script brotli brotli/CMakeLists.txt file gets modified which is a bit annoying (because of line 21) so:
- Wondering if we should add a check to run
make test-go-depsfirst if the script is being run on MacOS (if this still a problem - Add a line at the end of this script to restore
brotli/CMakeLists.txt?
There was a problem hiding this comment.
restoring CMakeLists.txt was added in 6f6ff2f
as for the make test-go-deps - I don't have this problem, probably because I already have required things available - what was the exact error you got? so that we can pinpoint missing dependency (I wanted to avoid blindly adding make test-go-deps to the script)
There was a problem hiding this comment.
I did a fresh clone and ran ./crates/sp1/build.sh and got:
=> ERROR [brotli-wasm-builder 6/6] RUN cd emsdk && . ./emsdk_env.sh && cd .. && ./scripts/build-brotli.sh -w -t /workspace/install/ 1.7s
------
> [brotli-wasm-builder 6/6] RUN cd emsdk && . ./emsdk_env.sh && cd .. && ./scripts/build-brotli.sh -w -t /workspace/install/:
0.130 Setting up EMSDK environment (suppress these messages with EMSDK_QUIET=1)
0.130 Adding directories to PATH:
0.130 PATH += /workspace/emsdk
0.130 PATH += /workspace/emsdk/upstream/emscripten
0.130 PATH += /workspace/emsdk/node/22.16.0_64bit/bin
0.130
0.130 Setting environment variables:
0.130 PATH = /workspace/emsdk:/workspace/emsdk/upstream/emscripten:/workspace/emsdk/node/22.16.0_64bit/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
0.130 EMSDK = /workspace/emsdk
0.130 EMSDK_NODE = /workspace/emsdk/node/22.16.0_64bit/bin/node
0.148 CMake Deprecation Warning at CMakeLists.txt:5 (cmake_minimum_required):
0.148 Compatibility with CMake < 2.8.12 will be removed from a future version of
...
1.684 CMake Error at CMakeLists.txt:348 (add_test):
1.684 Error evaluating generator expression:
1.684
1.684 $<TARGET_FILE:brotli>
1.684
1.684 No target "brotli"
1.684
1.684
1.685 CMake Warning:
1.685 Manually-specified variables were not used by the project:
1.685
1.685 CMAKE_POLICY_VERSION_MINIMUM
1.685
1.685
1.685 CMake Generate step failed. Build files cannot be regenerated correctly.
------
Dockerfile:11
--------------------
9 | COPY scripts/build-brotli.sh scripts/
10 | COPY brotli brotli
11 | >>> RUN cd emsdk && . ./emsdk_env.sh && cd .. && ./scripts/build-brotli.sh -w -t /workspace/install/
12 |
13 | FROM scratch AS brotli-wasm-export
--------------------
ERROR: failed to build: failed to solve: process "/bin/sh -c cd emsdk && . ./emsdk_env.sh && cd .. && ./scripts/build-brotli.sh -w -t /workspace/install/" did not complete successfully: exit code: 1
make: *** [.make/cbrotli-wasm] Error 1
And I thought that to be related to the CMakeLists.txt addition but I'm not sure.
- I tried such to delete
CMakeLists.txtaddition to then run./crates/sp1/build.shagain and got the same error as above. - But when I run
make test-go-depsbefore./crates/sp1/build.shit works. - notice I was using the other branch
pmikolajczyk/nit-4729-stylus-compilerto run the above.
If I use the current branch and run ./crates/sp1/build.sh I'm getting:
Compiling arbutil v0.1.0 (/Users/braga/repos/nitro/crates/arbutil)
Compiling serde_with v3.9.0
Compiling cargo_metadata v0.18.1
error: could not find native static library `brotlienc-static`, perhaps an -L flag is missing?
error: could not compile `brotli` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
+ git -C /Users/braga/repos/nitro/crates/sp1/../../brotli checkout -- CMakeLists.txt
It was similar to above, when I run make test-go-deps before ./crates/sp1/build.sh it works. But wondering if it could be just me. Maybe it would be nice to get a 3rd data point to make sure it's not just me?
There was a problem hiding this comment.
best apologies from my side - I didn't copy make build-replay-env test-go-deps from the original build.sh - at the time I was sure it is only required for block inputs generation and I happily ignored it; everything worked well for me locally because I had all the artifacts ready from previous runs
I restored it in 6f04660
the only thing that keeps me wondering is that CI passed all green... so I'm not sure why you experienced these problems, but adding make build-replay-env test-go-deps will be needed here anyway, so maybe we shouldn't spend more time here
There was a problem hiding this comment.
that's all good, just wanted to make sure the build scripts works seamlessly and I'm sure we'll keep improving it. So that's all good to me
| set_property(TARGET ${lib} APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${BROTLI_INCLUDE_DIRS}") | ||
| endforeach() | ||
|
|
||
| if(NOT (BROTLI_EMSCRIPTEN AND BROTLI_RISCV)) |
There was a problem hiding this comment.
I think here we meant to be if(NOT (BROTLI_EMSCRIPTEN OR BROTLI_RISCV))
| ) | ||
| endif() | ||
|
|
||
| if(NOT (BROTLI_EMSCRIPTEN AND BROTLI_RISCV)) |
There was a problem hiding this comment.
similar here if(NOT (BROTLI_EMSCRIPTEN OR BROTLI_RISCV)), I don't think we ever reach here which is why it went unoticed?
| @@ -33,6 +33,7 @@ unsafe extern "C" { | |||
| struct ForceSync<T>(T); | |||
|
|
|||
| unsafe impl<T> Sync for ForceSync<T> {} | |||
| unsafe impl<T> Send for ForceSync<T> {} | |||
There was a problem hiding this comment.
Now that ForceSync forces both Sync and Send should we update it's name or just the comment?
There was a problem hiding this comment.
Pulling from #4361:
/// Forces a type to implement [`Sync`] and [`Send`].
/// Only used to wrap static dictionary pointers (`*const EncoderPreparedDictionary`)
/// which point to immutable, process-lifetime data initialized once via `lazy_static`.
struct ForceSync<T>(T);
// SAFETY: ForceSync only wraps raw pointers to immutable, static dictionary data.
// The data is initialized once (via lazy_static) and never mutated or freed,
// so sharing across threads is safe.
|
Assigning it back to you @pmikolajczyk41 until we have 3.10 feature branch |
2 participants
Changed
provercrate:kzgfeature, to hidec_kzgdependency - SP1 crates won't use relevant functionalities, but fail to include it as a dependency (due to some non-digestible C-bindings).structoptan optional dependency (enabled in default features set) - it is used only for the binary, not used in the library part. Caused similar problem for SP1 crates asc_kzg(above).brotlicrate:unsafe impl<T> Send for ForceSync<T> {}: when compiling for SP1, the compiler demands it.target/lib-sp1/libdir for searching for C-compiledbrotlilib for theriscv64architecture.protobuf-compilerin theci-setupaction - required for compiling SP1 programsAdded
sp1/build.sh, that fetches and installssp1upwith C support. Compilesbrotlifor theriscv64target.sp1/brotli_cmake_patch.txtto make brotli work forriscv64.wasmerpin points at the feature branch - essentially it is7.1.0with a few, feature-gated amendments to make it work forriscv64stylus-compiler-programpackage:compile(input: &CompileInput) -> Result<Vec<u8>>that compiles a Stylus WASM program to a rv64 binary using the wasmer singlepass compiler. Just it. Pure Rust, works on every arch. Nothing related to SP1.compilefunction into a full ZK VM program for SP1 execution. Ready to be fed into SP1 ZK VM.stylus-compiler-runnercratestylus-compiler-programpackage.Native: simply runs thecompilemethod in the host environment. Nothing ZK related. Pure native Rust compilation.Execute: runs thestylus-compiler-programbinary crate inside SP1 ZK VM in the 'fast' mode, i.e. without producing proof.Prove: asExecute, but generates proof. Slow.Compare: is an additional mode that runsNative, thenExecuteand compares byte-by-byte, that their results are identical. Used for ensuring consistency in CI.closes NIT-4729
initiates ZK on
master