Skip to content

Comments

chore: update nix flake#10485

Merged
zerosnacks merged 9 commits intofoundry-rs:masterfrom
shaunkh:update-nix-flake
Jun 11, 2025
Merged

chore: update nix flake#10485
zerosnacks merged 9 commits intofoundry-rs:masterfrom
shaunkh:update-nix-flake

Conversation

@shaunkh
Copy link
Contributor

@shaunkh shaunkh commented May 9, 2025

The nix flake dev shell needed some updating.

  1. Removed flake-utils, it was unnecessary
  2. Updated the flake
  3. rust-overlay --> fenix (lighter)
  4. Environment variables that allow build to succeed (jemalloc was causing issues)

mattsse
mattsse reviewed May 10, 2025
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unfamiliar with this or how to review

but this changes seem appropriate from what a can decipher

anyone familiar with nix @grandizzy @zerosnacks @yash-atreya ?

@mattsse
Copy link
Member

mattsse commented May 10, 2025

@shaunkh looks like this exists as well #10316

can these be combined, should be merge #10316 first?

@shaunkh
Copy link
Contributor Author

shaunkh commented May 10, 2025

@mattsse ah didn't notice that PR

yup, can merge it first and i'll update this PR after

@shaunkh
Copy link
Contributor Author

shaunkh commented May 10, 2025 •
edited
Loading

I can also update it based on this #9260 and add CI for the flake

zerosnacks
zerosnacks reviewed May 13, 2025
View reviewed changes
flake.nix Outdated
Comment on lines 30 to 32
solc_0_8_27
# default for foundry's tests
(solc.mkDefault pkgs solc_0_8_27)
Copy link
Member

@zerosnacks zerosnacks May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest solc is now 0.8.30

Copy link
Contributor Author

@shaunkh shaunkh May 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unstable unfortunately doesn't have 0.8.30 yet but i've just updated it to use solc directly from nixpkgs instead of needing on overlay per @beeb suggestion

@zerosnacks
Copy link
Member

zerosnacks commented May 13, 2025

cc @beeb would you mind reviewing / testing this change?

I'm not very familiar with NixOS

Per #10316, it is a good idea we add this to the CI

@beeb
Copy link
Contributor

beeb commented May 13, 2025

Hey! I think there are some interesting points in this PR as well as #10316. Ideally we would merge them. Which one should we use to merge those ideas @zerosnacks ?

@zerosnacks @sveitser
add nix.yml ci job
6508d3f 
Co-authored-by: sveitser <sveitser@gmail.com>
@zerosnacks
Copy link
Member

zerosnacks commented May 13, 2025 •
edited
Loading

@beeb Thanks!

Which one should we use to merge those ideas

Let's use this one to aggregate the proposals as this one has momentum right now

I've copied the proposed CI flow with some minor modifications and pushed it to this branch with the original author included as co-author

@shaunkh would you mind integrating changes you see fit from https://github.com/foundry-rs/foundry/blob/27279a03744124b42e166b2d4d8f4fdd3ffbd7bb/flake.nix?

This was referenced May 13, 2025
Closed
Closed
@beeb
Copy link
Contributor

beeb commented May 13, 2025

Will review shortly

beeb
beeb reviewed May 14, 2025
View reviewed changes
Copy link
Contributor

@beeb beeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looking good! Small question

beeb
beeb approved these changes May 14, 2025
View reviewed changes
Copy link
Contributor

@beeb beeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@shaunkh
Copy link
Contributor Author

shaunkh commented May 14, 2025

@zerosnacks pushed changes, lmk what you think
thanks for the reviews @beeb

zerosnacks
zerosnacks reviewed May 15, 2025
View reviewed changes
@zerosnacks zerosnacks self-requested a review May 15, 2025 15:58
zerosnacks
zerosnacks approved these changes May 15, 2025
View reviewed changes
Copy link
Member

@zerosnacks zerosnacks left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 👍

I haven't been able to test the CI workflow locally w/ act but will manually test after merging, looks good to me

zerosnacks
zerosnacks reviewed May 15, 2025
View reviewed changes
flake.nix
# Environment variables
RUST_SRC_PATH = "${toolchain}/lib/rustlib/src/rust/library";
LD_LIBRARY_PATH = lib.makeLibraryPath [ pkgs.libusb1 ];
CFLAGS = "-DJEMALLOC_STRERROR_R_RETURNS_CHAR_WITH_GNU_SOURCE";
Copy link
Member

@zerosnacks zerosnacks May 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref for reviewers: https://github.com/foundry-rs/foundry/pull/9848/files, it is a fix for building tikv-jemalloc-sys

@beeb
Copy link
Contributor

beeb commented Jun 11, 2025

Any progress on this? @zerosnacks @mattsse

@zerosnacks
Copy link
Member

zerosnacks commented Jun 11, 2025

Hi @beeb thanks for the ping - will follow up shortly, no blockers on my end

@zerosnacks
Copy link
Member

zerosnacks commented Jun 11, 2025

Merging, manually reviewed again together with @grandizzy

@zerosnacks zerosnacks merged commit 2dbd8c9 into foundry-rs:master Jun 11, 2025
22 checks passed
@github-project-automation github-project-automation bot moved this to Done in Foundry Jun 11, 2025
@zerosnacks
Copy link
Member

zerosnacks commented Jun 11, 2025 •
edited
Loading

Hi, it unfortunately looks like the CI step to compile is failing: https://github.com/foundry-rs/foundry/actions/runs/15585038634/job/43889031628

It looks like we it is using Rust 1.86 but Foundry requires 1.87:

error: rustc 1.86.0 is not supported by the following packages:
  anvil@1.2.3 requires rustc 1.87
  anvil@1.2.3 requires rustc 1.87

Additionally it would be an improvement if the CI would fetch and build prior to creating a PR to bump the flake.lock conditionally

I'll leave the PR merged for now, I'll disable the weekly action however as it is currently failing until a fix has been merged

@zerosnacks zerosnacks mentioned this pull request Jun 11, 2025
Closed
@shaunkh
Copy link
Contributor Author

shaunkh commented Jun 11, 2025

@zerosnacks
added PR here #10760

@0xgregthedev 0xgregthedev mentioned this pull request Jun 27, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattsse mattsse mattsse left review comments

@zerosnacks zerosnacks zerosnacks approved these changes

@DaniPopes DaniPopes Awaiting requested review from DaniPopes DaniPopes is a code owner

@klkvr klkvr Awaiting requested review from klkvr

@grandizzy grandizzy Awaiting requested review from grandizzy grandizzy is a code owner

@yash-atreya yash-atreya Awaiting requested review from yash-atreya

+1 more reviewer

@beeb beeb beeb approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

Foundry
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shaunkh @mattsse @zerosnacks @beeb