chore: Add nix development environment - #6314
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds Nix flakes support to provide reproducible development environments for xrpld. The changes include Nix configuration files, documentation on how to use the Nix development shell, and updates to .gitignore for Nix-related directories.
Changes:
- Adds Nix flake configuration with support for multiple compiler versions (GCC 13-15, Clang 18-21)
- Provides comprehensive documentation for using the Nix development environment
- Updates .gitignore to exclude Nix and direnv artifacts
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| flake.nix | Main Nix flake configuration defining inputs and development shell outputs |
| flake.lock | Lock file pinning nixpkgs version for reproducibility |
| nix/utils.nix | Helper utilities for multi-platform support |
| nix/devshell.nix | Development shell definitions with multiple compiler configurations |
| docs/build/nix.md | Comprehensive guide for using Nix development environments |
| docs/build/environment.md | Added reference to Nix as an alternative setup method |
| .gitignore | Added entries for Nix, direnv, and clangd cache directories |
| .envrc | direnv configuration for automatic Nix shell activation |
| .config/cspell.config.yaml | Added Nix-related terms to spell checker dictionary |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ccache | ||
| pkg-config | ||
| gnumake | ||
| llvmPackages_18.clang-tools |
There was a problem hiding this comment.
Maybe let's use the latest which works well for us?
There was a problem hiding this comment.
As I understand currently ci is using tools from llvm 18. Bringing something newer may cause formatting issues.
There was a problem hiding this comment.
now that you moved develop to clang-formal 21(?), maybe this needs an update as well
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
11c1677 to
7e2c0f9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kuznetsss
added
the
Ready to merge
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6314 +/- ##
=========================================
- Coverage 79.8% 79.8% -0.0%
=========================================
Files 846 846
Lines 67746 67746
Branches 7561 7559 -2
=========================================
- Hits 54061 54050 -11
- Misses 13685 13696 +11 see 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
ximinez
added
the
Needs additional review
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
.envrc:1
.envrcusesuse flake, which requiresnix-direnv; with plaindirenvthis will error once a user runsdirenv allow. Consider either adding a compatibility/bootstrapping snippet (installing or sourcing nix-direnv) or updating the Nix docs to state that auto-activation via the checked-in.envrcrequiresnix-direnvspecifically.
use flake
docs/build/nix.md:84
- This section says "direnv or nix-direnv" can auto-activate, but the repo now ships an
.envrcthat usesuse flake(a nix-direnv feature). To avoid confusion, document that the provided.envrcrequiresnix-direnv, or adjust.envrcto work with plain direnv as well.
## Automatic Activation with direnv
[direnv](https://direnv.net/) or [nix-direnv](https://github.com/nix-community/nix-direnv) can automatically activate the Nix development shell when you enter the repository directory.
docs/build/nix.md:55
nix/devshell.nixexports the shell asno-compiler, but the docs instructnix develop .#no_compiler. That attribute name won't resolve; update the docs to usenix develop .#no-compiler(or rename the devShell attribute to match the docs).
A compiler can be chosen by providing its name with the `.#` prefix, e.g. `nix develop .#gcc15`.
Use `nix flake show` to see all the available development shells.
Use `nix develop .#no_compiler` to use the compiler from your system.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
Adding an .envrc messes things up for anyone who is already using direnv, but doesn't want to use nix. Unless I missed something, there's no way to skip/ignore the .envrc in the current directory to find one in a parent.
|
@ximinez, I have removed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
nix/devshell.nix:43
clangPkg = pkgs."llvmPackages_${toString version}".stdenv or null;will still throw ifllvmPackages_${version}itself is missing, so the intended "Invalid Clang version" error won’t be reached. Guard thellvmPackages_${version}lookup first (get it withor null, then access.stdenv).
clangPkg = pkgs."llvmPackages_${toString version}".stdenv or null;
in
if clangPkg != null && builtins.elem version clangVersions then
clangPkg
else
docs/build/nix.md:55
- The docs reference
nix develop .#no_compiler, but the devShell is exported asno-compiler(with a hyphen) innix/devshell.nix. This command won’t work as written; update the docs to match the actual shell attribute name.
A compiler can be chosen by providing its name with the `.#` prefix, e.g. `nix develop .#gcc15`.
Use `nix flake show` to see all the available development shells.
Use `nix develop .#no_compiler` to use the compiler from your system.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Thanks. I'll remove my review and merge. |
My issue was addressed, though I haven't reviewed the whole PR
ximinez
removed
the
Needs additional review
--------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
High Level Overview of Change
This PR adds a development environment provided by the Nix package manager, along with documentation on how to use it and provides minor updates to the
.gitignorefile.Type of Change
.gitignore, formatting, dropping support for older tooling)