Add Nix flake for deterministic Nix builds (attempt 2)#909
Add Nix flake for deterministic Nix builds (attempt 2)#909ThatOtherAndrew wants to merge 2 commits intodigraphs:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Nix flake support to enable deterministic nix build / nix develop workflows for building the GAP Digraphs package from this repository.
Changes:
- Introduce
flake.nixdefining adigraphsderivation and a development shell. - Pin Nixpkgs via
flake.lockfor reproducible builds. - Ignore the default Nix build symlink (
result) in.gitignore.
Reviewed changes
Copilot reviewed 1 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| flake.nix | Defines Nix flake outputs (package + devShell) and the build/install steps for Digraphs. |
| flake.lock | Pins nixpkgs revision/hash to make builds deterministic. |
| .gitignore | Ignores the result symlink created by nix build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configurePhase = '' | ||
| runHook preConfigure | ||
|
|
||
| # Generate the configure script | ||
| mkdir -p gen | ||
| aclocal -Wall --force | ||
| autoconf -Wall -f | ||
| autoheader -Wall -f | ||
|
|
||
| # Run configure, pointing at the GAP root | ||
| ./configure --with-gaproot=${gaproot} | ||
|
|
||
| runHook postConfigure | ||
| ''; |
There was a problem hiding this comment.
The custom configurePhase calls ./configure without passing the standard Nix configure flags (e.g., --prefix=$out, build/host flags, and any configureFlags), which can lead to incorrect paths and reduced portability (especially for cross builds). Prefer using the default configurePhase with configureFlags = [ "--with-gaproot=..." ]; or, if keeping a custom phase, invoke ./configure with the standard flags included.
| nativeBuildInputs = with pkgs; [ | ||
| autoconf | ||
| automake | ||
| gcc |
There was a problem hiding this comment.
nativeBuildInputs includes gcc, but this flake advertises Darwin support as well; forcing gcc can cause toolchain mismatches or failures on macOS where the default stdenv compiler is clang. Consider relying on stdenv.cc (already provided by mkDerivation) and drop gcc, or select an appropriate stdenv/toolchain per-system.
| gcc |
|
|
||
| buildPhase = '' | ||
| runHook preBuild | ||
| make |
There was a problem hiding this comment.
The overridden buildPhase runs plain make, which bypasses the standard stdenv make flags (including parallelism and any makeFlags). To keep builds consistent and faster under Nix, either use the default buildPhase or call make with the stdenv-provided flags.
| make | |
| make $makeFlags |
| configurePhase = '' | ||
| runHook preConfigure | ||
|
|
||
| # Generate the configure script | ||
| mkdir -p gen | ||
| aclocal -Wall --force | ||
| autoconf -Wall -f | ||
| autoheader -Wall -f | ||
|
|
||
| # Run configure, pointing at the GAP root | ||
| ./configure --with-gaproot=${gaproot} | ||
|
|
||
| runHook postConfigure | ||
| ''; | ||
|
|
There was a problem hiding this comment.
This flake reimplements the repo's existing ./autogen.sh logic inline. Using the script directly (or autoreconfHook) would reduce drift if the autogen steps change in the future and keeps the build definition simpler.
| configurePhase = '' | |
| runHook preConfigure | |
| # Generate the configure script | |
| mkdir -p gen | |
| aclocal -Wall --force | |
| autoconf -Wall -f | |
| autoheader -Wall -f | |
| # Run configure, pointing at the GAP root | |
| ./configure --with-gaproot=${gaproot} | |
| runHook postConfigure | |
| ''; | |
| preConfigure = '' | |
| ./autogen.sh | |
| ''; | |
| configureFlags = [ | |
| "--with-gaproot=${gaproot}" | |
| ]; |
| # Copy GAP source files | ||
| cp -r gap "$out/digraphs/" | ||
| cp -r lib "$out/digraphs/" 2>/dev/null || true | ||
| cp -r doc "$out/digraphs/" 2>/dev/null || true | ||
| cp -r data "$out/digraphs/" 2>/dev/null || true | ||
| cp -r tst "$out/digraphs/" 2>/dev/null || true | ||
|
|
||
| # Copy the compiled shared object | ||
| cp -r bin "$out/digraphs/" | ||
|
|
||
| # Copy package metadata files | ||
| cp PackageInfo.g "$out/digraphs/" | ||
| cp init.g "$out/digraphs/" | ||
| cp read.g "$out/digraphs/" | ||
| cp -r makedoc.g "$out/digraphs/" 2>/dev/null || true | ||
| cp LICENSE "$out/digraphs/" 2>/dev/null || true | ||
| cp CHANGELOG.md "$out/digraphs/" 2>/dev/null || true | ||
| cp README.md "$out/digraphs/" 2>/dev/null || true | ||
|
|
There was a problem hiding this comment.
The installPhase uses cp ... 2>/dev/null || true to ignore missing paths, which can silently produce incomplete outputs if a directory unexpectedly disappears or is renamed. Prefer explicit if [ -d ... ]; then ...; fi (or a single copy of an explicit file list) so packaging failures are surfaced when the layout changes.
| # Copy GAP source files | |
| cp -r gap "$out/digraphs/" | |
| cp -r lib "$out/digraphs/" 2>/dev/null || true | |
| cp -r doc "$out/digraphs/" 2>/dev/null || true | |
| cp -r data "$out/digraphs/" 2>/dev/null || true | |
| cp -r tst "$out/digraphs/" 2>/dev/null || true | |
| # Copy the compiled shared object | |
| cp -r bin "$out/digraphs/" | |
| # Copy package metadata files | |
| cp PackageInfo.g "$out/digraphs/" | |
| cp init.g "$out/digraphs/" | |
| cp read.g "$out/digraphs/" | |
| cp -r makedoc.g "$out/digraphs/" 2>/dev/null || true | |
| cp LICENSE "$out/digraphs/" 2>/dev/null || true | |
| cp CHANGELOG.md "$out/digraphs/" 2>/dev/null || true | |
| cp README.md "$out/digraphs/" 2>/dev/null || true | |
| # Copy GAP source files (required) | |
| cp -r gap "$out/digraphs/" | |
| # Copy optional GAP directories if they exist | |
| if [ -d lib ]; then | |
| cp -r lib "$out/digraphs/" | |
| fi | |
| if [ -d doc ]; then | |
| cp -r doc "$out/digraphs/" | |
| fi | |
| if [ -d data ]; then | |
| cp -r data "$out/digraphs/" | |
| fi | |
| if [ -d tst ]; then | |
| cp -r tst "$out/digraphs/" | |
| fi | |
| # Copy the compiled shared object (required) | |
| cp -r bin "$out/digraphs/" | |
| # Copy required package metadata files | |
| cp PackageInfo.g "$out/digraphs/" | |
| cp init.g "$out/digraphs/" | |
| cp read.g "$out/digraphs/" | |
| # Copy optional metadata / documentation files if they exist | |
| if [ -f makedoc.g ]; then | |
| cp makedoc.g "$out/digraphs/" | |
| fi | |
| if [ -f LICENSE ]; then | |
| cp LICENSE "$out/digraphs/" | |
| fi | |
| if [ -f CHANGELOG.md ]; then | |
| cp CHANGELOG.md "$out/digraphs/" | |
| fi | |
| if [ -f README.md ]; then | |
| cp README.md "$out/digraphs/" | |
| fi |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #909 +/- ##
=======================================
Coverage 97.35% 97.35%
=======================================
Files 50 50
Lines 21045 21045
Branches 639 639
=======================================
Hits 20489 20489
Misses 491 491
Partials 65 65 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
reiniscirpons
left a comment
There was a problem hiding this comment.
Hi Andromeda, thanks for the PR! I'm not a nix user myself, but think its a good effort to try and package Digraphs for simpler installation.
Some comments:
- It seems the flake only lists gap itself as a dependency, but Digraphs depends on other gap packages such as
datastructuresandio(see theDependenciesrecord inPackageInfo.g(https://github.com/digraphs/Digraphs/blob/main/PackageInfo.g#L560) for more info). Thegapdependency may or may not package them, and may or may not package the right versions of these packages, so its probably a good idea to explicitly set them as dependencies.
This of course introduces some headache, since as far as I know neither the datastructures nor io are available as nix packages. Of course, in theory we could make flakes available for these packages too, but now these would also need to be maintained etc, which can quickly get out of hand. I think this should be a community rather than single author effort to make it maintainable, but at the moment nix is not used much in the community as far as I can tell.
-
Gap has its own package manager: the
PackageManagerpackage (documentation: https://gap-packages.github.io/PackageManager/doc/chap0_mj.html , github: https://github.com/gap-packages/PackageManager) by @mtorpey . It solves quite a lot of headaches related to building and installing gap packages and their dependencies, and if I understand correctly it should be able to build Digraphs too (including a development version). So maybe it would be possible to simplify the script by having a separate flake forPackageManager, and then all this flake would do is call the PackageManagerInstallPackagecommand from gap? -
Before merging this PR we would probably want to add some CI job for testing the nix builds. Otherwise it is a very high probability that this file will just go out-of-sync or break as we update the package. At the same time, I'm not sure if any of the current maintainers of digraphs would be able to fix any nix-related issues, since we are not nix users ourselves.
Overall I think its a good effort and probably quite useful for other nix users wanting to install digraphs, but I'm not sure we will merge this PR in the near future mainly due to a lack of nix support in the gap ecosystem and lack of nix know-how among the digraphs maintainers. So don't feel like you need to spend much more time fixing any of the issues above.
If you are interested in gap packaging in general, it might be a good idea to talk to Michael at one of the meetings about the PackageManager package. Thanks again!
| mkdir -p gen | ||
| aclocal -Wall --force | ||
| autoconf -Wall -f | ||
| autoheader -Wall -f |
There was a problem hiding this comment.
| mkdir -p gen | |
| aclocal -Wall --force | |
| autoconf -Wall -f | |
| autoheader -Wall -f | |
| ./autogen.sh |
I think the preferred method for generating the configure script is via the autogen.sh script. But I'm not sure if there is some technical nix reason to prefer the more explicit instructions.
Nix is a functional programming language used for deterministic builds, by pinning environment "inputs" (e.g.
gcc) to a specific version. This allows for a universal and deterministicnix buildcommand which replaces the need for autogen, configure, and make.This is a recreation of a previous PR (#906) which was messed up due to accidentally committing multiple features to the main branch. I have learned from my mistake and one sleepless night later my changes are properly branched now. oops!