Conversation
72e0027 to
155b327
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4184 +/- ##
==========================================
- Coverage 33.12% 32.84% -0.28%
==========================================
Files 462 462
Lines 55958 55958
==========================================
- Hits 18534 18378 -156
- Misses 34168 34356 +188
+ Partials 3256 3224 -32 |
155b327 to
a79704b
Compare
❌ 8 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
477c24d to
8cd0330
Compare
KolbyML
left a comment
There was a problem hiding this comment.
Everything looks really good, just one minor concern. I don't know when lib/lib-wasm would be built, but it looks like target/lib doesn't use the standard target? But it isn't built with make test-go so I am not sure.
Anyways I would double check that, other then that I think this PR is good.
It looks like our CI doesn't verify have coverage if this is an issue or not, maybe we don't even need to lines, but Tsahi added one so I am assuming it was added intentional
There was a problem hiding this comment.
it was stated on call this would be reverted, just flagging it so we don't forget
| println!("cargo:rustc-link-search=../../target/lib-wasm/"); | ||
| println!("cargo:rustc-link-search=../target/lib/"); | ||
| println!("cargo:rustc-link-search=../../../target/lib-wasm/"); | ||
| println!("cargo:rustc-link-search=../../../target/lib/"); |
There was a problem hiding this comment.
The original pathing looks odd
Maybe it was intentional? it looks like lib-wasm/lib isn't built when running make test-go, but I am assuming
Maybe we should do
println!("cargo:rustc-link-search=../../../target/lib-wasm/");
println!("cargo:rustc-link-search=../target/lib/");
As there is a target folder in crates/wasm-libraries, and target/lib-wasm/ looks like it is targeting the workspaces target.
Ideally everything was in 1 workspace, maybe having so many seperate workspaces was intentional? but that problem is unrelated to this PR, maybe something we can look into in the future, as having 1 target folder in the project would be very nice
|
@KolbyML regarding the paths in the build scripts - it's pretty tricky and it took me some time until I got them right; First thing to note is that
Both workspaces have at least one crate that requires some static libraries during linking phase. These libraries used to live in two different Hopefully this should explain all the changes to the target paths. |
Oh I see 👍 |
46e2e4d
194c0fd
arbitrator/directory tocrates/.Cargo.{toml, lock}to the repo root.target/directory for the main Rust workspace and Go artifacts (previously we had maintarget/andarbitrator/target.pulls in: OffchainLabs/nitro-contracts#406
pulls in: OffchainLabs/nitro-contracts#407