-
Notifications
You must be signed in to change notification settings - Fork 24
feat: Add nix flake for ffi #1319
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
Conversation
1bf2a49 to
4d41947
Compare
rkuris
left a comment
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.
Definitely don' tknow enough about nix to consider reviewing this, but willing to stamp it after these questions are answered:
- Is there any change to the release process? The RELEASE.md file wasn't changed if so.
- When dependency updates happen, are any additional steps required?
- I noticed all the files in the /nix result directory are dated 1969. Is this expected?
- The object files are about half as big, nearly identical text area bug the "others" section is much smaller. Any idea why?
➜ ffi git:(main) ✗ size firewood_ffi*
__TEXT __DATA __OBJC others dec hex
827635 38472 0 44544 910651 de53b firewood_ffi-ac2de0665df5119e.firewood_ffi.36d0102cbf9e788e-cgu.0.rcgu.o
835697 38464 0 1815990 2690151 290c67 firewood_ffi-df1593dabf0edbed.firewood_ffi.eccd115242f4cb72-cgu.0.rcgu.o
I noticed the relocation table is smaller (25899 entries vs 54092). Maybe some new optimization is being done?
afaik the only change is to the CI job that publishes to firewood-go-ethhash, and I don't see mention of that in RELEASE.md
The only requirement is to keep Cargo.lock up-to-date, and Brandon already added a CI check to ensure PRs won't merge if it's out-of-sync.
That is expected - it's the unix epoch (i.e. time=0). Nix sets all built outputs to this time to enable reproducibility, content-addressibility and build caching.
Assuming this is an apples-to-apples comparison (i.e. maxperf and same build args), I would guess there is a difference in the build tooling that nix uses? |
Yes, it was apples to apples. One concern is that it may affect performance. I'll bring this up at standup. |
Would it make sense to remove the update to the CI job pending qualification of the build artifact with an appropriate performance test? |
|
Yes, that's a good idea.
…On Tue, Sep 30, 2025, 5:48 PM maru ***@***.***> wrote:
*maru-ava* left a comment (ava-labs/firewood#1319)
<#1319 (comment)>
Assuming this is an apples-to-apples comparison (i.e. maxperf and same
build args), I would guess there is a difference in the build tooling that
nix uses?
Yes, it was apples to apples. One concern is that it may affect
performance. I'll bring this up at standup.
Would it make sense to remove the update to the CI job pending
qualification of the build artifact with an appropriate performance test?
—
Reply to this email directly, view it on GitHub
<#1319 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAYLR3H7WEIXGJDINVKM7DL3VMQEBAVCNFSM6AAAAACHT3JLC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTGNJUGI3TMOBWGQ>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
ea5ffaa to
237d67e
Compare
|
Removed changes to attach-static-libs workflow and added new nix-specific ffi-nix job to ensure coverage. |
237d67e to
b1a0a39
Compare
| commonArgs = { | ||
| inherit src; | ||
| strictDeps = true; | ||
| dontStrip = true; |
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.
Nix strips debugging details by default, which was resulting in the .a file being ~6MB instead of ~50MB.
ffi/flake.nix
Outdated
| pname = ffiCargoToml.package.name; | ||
| version = workspaceCargoToml.workspace.package.version; | ||
|
|
||
| CARGO_PROFILE = "maxperf"; |
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 doesn't appear to be working. the size reduction @rkuris noticed is from fewer inlined methods which tells me there's less LTO.
I haven't determined the correct way to set this with rust-overlay, but cargo will use the PROFILE environment variable if --profile is not provided on the command line.
EDIT: that appears to be incorrect. looks like there is no environment variable that cargo will use to infer the profile and it must be provided in the cli invocation. https://doc.rust-lang.org/cargo/reference/profiles.html#profile-selection
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.
I see CARGO_PROFILE is actually a parameter to crane's mkCargoDerivation which is wrapped by buildDepsOnly and buildPackage. So, I'm not entirely sure why this profile doesn't appear to work.
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.
As per the most recent code comment, the reason for the reduction in size was due to nix automatically stripping debugging symbols (which is the default for nix). I've corrected that, and the .a files are now [edit: almost] identical in size.
I also believe that CARGO_PROFILE is being respected. This var is for configuring crane as per its api docs (search for CARGO_PROFILE) rather than intended for direct consumption by cargo. This can be verified by the following invocation:
cd ffi && nix build .#firewood-ffi --print-build-logs --rebuild 2>&1 | grep -i "cargo build"
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.
Ah, I didn't realize the change to not strip came after Ron's comment.
4f04b6d to
76e2985
Compare
ffi/flake.nix
Outdated
| # Install the static library and header | ||
| postInstall = '' | ||
| mkdir -p $out/lib $out/include | ||
| cp target/*/libfirewood_ffi.a $out/lib/ || cp target/release/libfirewood_ffi.a $out/lib/ |
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.
the * should probably be replaced with maxperf as the target directory is deterministic on the profile used. If the resulting artifact isn't in the maxperf directory, then we're using the wrong profile.
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.
Done
|
fwiw I added a script to validate build equivalency. Not sure whether it makes sense to merge it, I just wanted to have a common reference point of comparison. |
|
I tried to run the build equivalency script on macos but there are some issues with the macos support that I think should be corrected before merge. |
Set MAKEFLAGS=-j1 to force sequential build of vendored jemalloc in tikv-jemallocator. The vendored jemalloc has race conditions during parallel builds causing non-deterministic symbol generation on x86_64 (NixOS/nixpkgs#380852). Using MAKEFLAGS instead of NUM_JOBS ensures only the make invocation for jemalloc is affected, while Cargo continues to build Rust crates in parallel. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
7cd05cc to
0cf4a26
Compare
15742d2 to
24b54e7
Compare
| echo "✅ Relocation types match" | ||
| else | ||
| echo "❌ Relocation types differ" | ||
| diff "$TMPDIR/nix-reloc-types.txt" "$TMPDIR/cargo-reloc-types.txt" |
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.
nit: would have been nice for an explicit exit after diff. It's magic to know that diff exits with a non-zero exit status due to there being differences.
## Why this should be merged A nix flake for the firewood ffi library enables deterministic builds both for [firewood-go-ethhash](http://github.com/ava-labs/firewood-go-ethhash) and a future nix build of avalanchego. ## How this works - Adds flake for ffi crate - Updates attach-static-libs workflow to build static libs with the flake ## How was this tested - CI (new ffi-nix job) - Locally - Install nix: - `curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- install` - `cd ffi` - Build: - `nix build .#firewood-ffi` - Test (assuming build step perfomed) outside of a nix dev shell: - `GOLANG="nix run $PWD#go"` - Runs the version of golang referenced by the ffi flake - Need to capture this before changing directories to `result/ffi` because `result` is a nix store symlink so `../../` won't resolve to the `ffi` path containing the flake - Running golang this way instead of with `nix develop` validates usage of the build output won't require `nix develop`'s shell magic - `cd result/ffi` - `GOEXPERIMENT=cgocheck2 TEST_FIREWOOD_HASH_MODE=ethhash ${GOLANG} test ./... -v` --------- Co-authored-by: Claude <noreply@anthropic.com>
As a follow-on to #1319, this PR introduces the [just](https://github.com/casey/just) command runner to ensure the new CI ffi-nix CI job can be trivially reproduced locally. --------- Co-authored-by: Ron Kuris <ron.kuris@avalabs.org>
Why this should be merged
A nix flake for the firewood ffi library enables deterministic builds both for firewood-go-ethhash and a future nix build of avalanchego.
How this works
How was this tested
curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- installcd ffinix build .#firewood-ffiGOLANG="nix run $PWD#go"result/ffibecauseresultis a nix store symlink so../../won't resolve to theffipath containing the flakenix developvalidates usage of the build output won't requirenix develop's shell magiccd result/ffiGOEXPERIMENT=cgocheck2 TEST_FIREWOOD_HASH_MODE=ethhash ${GOLANG} test ./... -v