Skip to content
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

1142 add nix flake support #1157

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

lessuselesss
Copy link
Contributor

Relates to:

Issue #1142

Risks

Low - This change:

  • Only affects development environment setup
  • Doesn't modify runtime code
  • Is optional (developers can still use traditional npm/pnpm setup)
  • Can be easily reverted if issues arise

Background

What does this PR do?

Adds Nix Flake support to provide a reproducible development environment with:

  • Correct Node.js and pnpm versions
  • Helpful welcome message showing common commands
  • Integration with existing monorepo structure

What kind of change is this?

Improvements (adds optional development tooling without changing existing functionality)

Documentation changes needed?

My changes require a change to the project documentation.
I will update the local development guide to include:

  1. Installation of Nix using Determinate Nix Installer
  2. Instructions for using the development environment

Testing

Where should a reviewer start?

  1. Install Nix using Determinate Nix Installer:
curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix | sh -s -- install
  1. Clone the PR and enter the development environment:
git clone https://github.com/ai16z/eliza.git
cd eliza
nix develop
  1. Verify the welcome message appears with instructions for:
    • pnpm i
    • pnpm build
    • pnpm clean

Detailed testing steps

  1. Prerequisites:

    • Install Nix following the steps above
    • Verify flakes are enabled by default
  2. Test environment setup:

    git clone https://github.com/ai16z/eliza.git
    cd eliza
    nix develop
    • Verify welcome message appears
    • Verify Node.js version matches project requirements
    • Verify pnpm is available
  3. Test build process:

    pnpm i
    pnpm build
    • Verify all dependencies install correctly
    • Verify build completes successfully
  4. Test clean process:

    pnpm clean
    pnpm i
    pnpm build
    • Verify clean removes build artifacts
    • Verify rebuild works after clean

Discord username

Adam Turner | lessuseless
ar4s_45979

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

@HashWarlock
Copy link
Collaborator

@lessuselesss love this PR, but there are some weird problems that will cause a NixOS user to fail when building the codebase with nix flakes enabled.

For example, I built this on my NixOS machine and we see this error:

WARN  Unsupported engine: wanted: {"node":"23.3.0"} (current: {"node":"v20.18.1","pnpm":"9.15.0"})
docs                                     |  WARN  Unsupported engine: wanted: {"node":"23.3.0"} (current: {"node":"v20.18.1","pnpm":"9.15.0"})

We may think...what?! No Way...But how?? The pkgs specifically lists nodejs_23 and when I run node version I will see the v23.2.0, but that still does not equal v20.18.1.

So I did some digging bc Nix can be a pain in the ass at times with weird dependencies errors. So I checked the pnpm pkgs source code and found this line https://github.com/NixOS/nixpkgs/blob/394571358ce82dff7411395829aa6a3aad45b907/pkgs/development/tools/pnpm/generic.nix#L28

And nodejs pkg points to:
image

So this here is the culprit for why a NixOS user will hit this weird error even though we declaratively chose the right node version.

@monilpat monilpat changed the base branch from main to develop December 17, 2024 08:35
@lessuselesss
Copy link
Contributor Author

Hello,

Thank you so much for the valuable feedback. I'm excited to contribute and am happy (and was hoping!!) to have someone from the nix community overseeing contributions here!

Nice catch on finding the culprit, I'll investigate some workarounds 🙇

@odilitime
Copy link
Collaborator

I don't like the hardcoded versions, maybe another dev can offer a better suggestions on how to get the latest version

like git describe --tags --abbrev=0

@lessuselesss
Copy link
Contributor Author

lessuselesss commented Dec 18, 2024

@odilitime

Thanks for the feedback! I've updated the implementation to automatically detect versions from package.json instead of hardcoding them. The flake now:

  • Reads Node.js version from package.json's engines field
  • Extracts pnpm version from packageManager field
  • Automatically handles version updates when package.json changes
  • No manual version or hash updates needed

This should make it more maintainable and reduce the risk of version mismatches.

Alternatively,

@HashWarlock would an overlay for nodejs/pnpm be more appropriate nix'ing here?

- Add Nix flake for reproducible development environment
- Auto-detect Node.js and pnpm versions from package.json
- Configure development tools (gcc, python3, vips, etc.)
- Add shell hook with helpful development commands
@lessuselesss lessuselesss force-pushed the 1142--add-nix-flake-support branch from 72d22cd to b7185b6 Compare December 18, 2024 05:23
@lessuselesss
Copy link
Contributor Author

I've reset the branch to resolve the merge conflicts and provide a clean implementation. The core functionality remains the same:

  • Auto-detecting versions from package.json
  • Setting up the development environment
  • Configuring all necessary tools

The previous discussion and feedback has been incorporated into this cleaner version.

@HashWarlock
Copy link
Collaborator

@odilitime

Thanks for the feedback! I've updated the implementation to automatically detect versions from package.json instead of hardcoding them. The flake now:

  • Reads Node.js version from package.json's engines field
  • Extracts pnpm version from packageManager field
  • Automatically handles version updates when package.json changes
  • No manual version or hash updates needed

This should make it more maintainable and reduce the risk of version mismatches.

Alternatively,

@HashWarlock would an overlay for nodejs/pnpm be more appropriate nix'ing here?

Probably best to do an overlay bc whenever I run the current nix develop it will try to install package that is not present in nixpkgs.
telegram-cloud-photo-size-1-5122966996043017587-y

How do the tests work in your nix environment?

@lessuselesss
Copy link
Contributor Author

@HashWarlock

I ended up installing eliza on a fresh VM as per the readme, and found similar results to the below (esp. #4)

Let me share what I've found from running the tests in the nix environment:

  1. Core Tests Status:
  • All 121 tests passed (with 1 skipped)
  • Clean execution with no major issues
  • Test duration: 1.52s
  1. Plugin Tests Status:
  • Several plugins executed tests successfully (e.g., solana plugin: 5 tests passed)
  • Some failures were observed:
    • plugin-evm: 3 failed tests due to network connectivity issues with IoTeX endpoints
    • plugin-0g: Test process hangs and requires manual interruption
    • Several plugins have no test scripts configured yet
  1. Regarding the nix environment:
  • Tests are running through the nix shell environment successfully
  • The test infrastructure itself works as expected
  • We're seeing expected behavior with node modules and dependencies
  1. Areas for Improvement:
  • Some plugins need test script configurations
  • Network-dependent tests (like EVM) might need mocking or better error handling
  • The plugin-0g hanging issue needs investigation

Regarding your question about an overlay - yes, that sounds like a good approach so I've gone ahead and made an attempt.

Let me know if you'd like me to focus on any specific area or if you need more details about the test results.

@lessuselesss lessuselesss force-pushed the 1142--add-nix-flake-support branch from 4cafe97 to 2c78149 Compare December 18, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants