Skip to content

Add nix flake #68

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

JosephGoulden
Copy link

@JosephGoulden JosephGoulden commented Jul 24, 2025

Its great to see a new project to build a server utilising LDK. Nix can help a lot with reproducible/declarative builds and system configuration and testing. I've added a flake to show what it can do.

The flake:

  • Builds all projects
  • Runs all cargo tests
  • Formats nix files
  • Runs cargo advisory check
  • Defines a systemd service
  • Runs an end to end test of server and CLI.

I also updated cargo dependencies to remove a vulnerability found in the advisory check.

And I deleted the debug_assert! macros in the sqlite store. This is because nix runs the tests on the production optimised build and these were failing as the test expects a panic but doesn't get it. I think its better to keep prod and debug behavior the same in this case.

CI now just calls nix flake check. This means that a developer can easily run the build and tests in the exact same way (toolchain, packages, etc) that is run in CI with one simple command.
I removed the Mac build as nix works on Mac so it should be fine. I understand this might just be for developer experience though so I can add that back if needs be.

There are some minor rust formats, I think due to upgrading to a more recent toolchain.

@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Jul 24, 2025

👋 I see @tankyleo was un-assigned.
If you'd like another reviewer assignment, please click here.

Builds all projects
Runs all tests
Formats nix files
Runs cargo advisory check
Updated cargo deps to remove vulnerability
Added end to end system test
@ldk-reviews-bot ldk-reviews-bot requested a review from tankyleo July 24, 2025 23:28
@tnull tnull requested review from tnull and removed request for tankyleo July 25, 2025 07:53
@ldk-reviews-bot
Copy link

🔔 1st Reminder

Hey @tnull! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Thanks for showing interest to improve the LDK Server project!

While we're interested in improving our build system going forward--and nix being a contender there--I'm not sure that we want to unilaterally switch LDK Server over to use nix at the current time. I think exploring nix might be a promising direction in the future, and while this PR is a great first PoC, I don't think we are in a position right now to just go for such a fundamental decision on a whim.

As a side note, we def. don't want to test changes or switch our formatter, just to accommodate any particular build system.

@JosephGoulden
Copy link
Author

JosephGoulden commented Jul 28, 2025

Okay, if I revert the changes to CI and the rust formatter will you be able to merge it then? It will be able to run alongside the cargo build and developers will have the option to use it or not. It works the same way in core lightning with the nix flake I added there. Its much easier for anyone that is already using nix to work on the project if it has a flake. And if it doesn't work out then it can easily be removed.

I can offer my assistance with maintenance, if it needs work after merge feel free to tag me. If it means anything I've been writing rust for about seven years and I've built more CI pipelines and test frameworks than I can remember. And having used nix for a few years, I don't think you'll find a better tool for the job.

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