Uthar / nix-cl

Utilities for packaging ASDF systems using Nix
BSD 2-Clause "Simplified" License
15 stars 6 forks source link

Stack overflow when building full package collection #8

Closed lukego closed 2 years ago

lukego commented 2 years ago

I noticed that Nix crashes with a stack overflow when I try to build the full set of packages for sbcl like this:

$ nix-build . -A lispPackages_new.sbclPackages
error: stack overflow (possible infinite recursion)

Does anyone understand what the problem is? (Is it a true infinite recursion problems or just a deep recursion due to coding style that would need to be changed?)

My use case is that I would like to test building all packages via a Hydra CI instance. For this I need to evaluate a Nix expression that returns a complete (or at least ample) set of package derivations.

lukego commented 2 years ago

I have a small patch that makes this work but it feels half-baked:

diff --git a/pkgs/development/lisp-modules-new/lisp-packages.nix b/pkgs/development/lisp-modules-new/lisp-packages.nix
index 3e10f870759..49777ae9701 100644
--- a/pkgs/development/lisp-modules-new/lisp-packages.nix
+++ b/pkgs/development/lisp-modules-new/lisp-packages.nix
@@ -55,11 +55,13 @@ let
   # This is probably causing performance problems...
   flattenedDeps = lispLibs:
     let
+      toSet = list: builtins.listToAttrs (map (d: { name = d.pname; value = d; }) list);
+      toList = attrValues;
       walk = acc: node:
         if length node.lispLibs == 0
         then acc
-        else foldl walk (acc ++ node.lispLibs) node.lispLibs;
-    in unique (walk [] { inherit lispLibs; });
+        else builtins.foldl' walk (acc // toSet node.lispLibs) node.lispLibs;
+    in toList (walk {} { inherit lispLibs; });

   # Stolen from python-packages.nix
   # Actually no idea how this works

The part that's baked is switching the accumulator from a list to a set, so that space usage should be linear instead of quadratic in the number of packages.

The unbaked part is that I needed to randomly change foldl to foldl' in order to make it evaluate.

I have experience optimizing eager functional programs, but not lazy ones, so it is possible I am looking at this all wrong.

Relatedly: if I did have a proper fix then would it make sense to PR that here or to the main nixpkgs repo? (How is the packaging code kept in sync?)

Uthar commented 2 years ago

Yes, the code there was just bad, inefficient.

teu5us suggested to propagate dependencies using propagatedBuildInputs + an env hook instead of walking the dependencies in Nix code. Recently I've been rewriting it this way on the cleanup branch, and it's been much faster.

I think it's better to do it this way, because it's more idiomatic to the way nixpkgs does things in pythonPackages etc.

(Probably all the fetchTarball's should be changed to pkgs.fetchzip, because right now only evaluating the derivations needs to wait to fetch the source code, which makes it slower)

Uthar commented 2 years ago

I just had a problem there, where adding the hook via addEnvHook made it so it ran a bunch of times which was slow. I worked around it by just calling the hook manually in preConfigure, but I don't know if it's the right way?

lukego commented 2 years ago

I don't know either.

The change did work well enough to get Hydra to start building the sbcl packages though. Maybe this will be a convenient way to keep track of which packages work, and what changes between updates? https://hydra.nuddy.co/project/nixpkgs-lisp

Here's the commit that I'm using https://github.com/nuddyco/nixpkgs/commit/67d169949cab476643e98bf51dd622ccb48012d9 in case you want to cherry-pick that or suggest a way that I should contribute it.

Uthar commented 2 years ago

Wow, this is really cool :-D I never played with Hydra before

Uthar commented 2 years ago

Thanks lukego for this effective fix

Currently committing is complicated because the repo is in fossil, and I'm exporting to git. I should switch fully to git so that people can push.

We should also make a PR to nixpkgs, sadly the process now is manual patching

Uthar commented 2 years ago

Hmmm I think this also made CI fail here https://github.com/NixOS/nixpkgs/pull/172234 https://gist.github.com/GrahamcOfBorg/85a4865781194f0d0d4c96cd3e5ec8ab

Uthar commented 2 years ago

(Probably all the fetchTarball's should be changed to pkgs.fetchzip, because right now only evaluating the derivations needs to wait to fetch the source code, which makes it slower)

done in 60dbe8f643eced491762bca2cf2ff5b0f998fdb3

Uthar commented 2 years ago

We should also make a PR to nixpkgs, sadly the process now is manual patching

https://github.com/NixOS/nixpkgs/pull/193754

lukego commented 2 years ago

woohoo :)