Skip to content

chore: update nix flake #10485

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

chore: update nix flake #10485

wants to merge 9 commits into from

Conversation

shaunkh
Copy link

@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
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
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

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
Author

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

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 <[email protected]>
@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
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

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

@beeb beeb beeb approved these changes

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

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

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

@yash-atreya yash-atreya Awaiting requested review from yash-atreya yash-atreya is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Foundry
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shaunkh @mattsse @zerosnacks @beeb