From 4a81a5e55686ca1360ad67daddb559865af713b5 Mon Sep 17 00:00:00 2001
From: Rebecca Turner <rbt@sent.as>
Date: Tue, 13 May 2025 15:15:22 -0700
Subject: [PATCH] lib.packagesFromDirectoryRecursive: Allow non-"path"
 `directory`
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As initially designed, `lib.packagesFromDirectoryRecursive` allowed
passing a string for the `directory` argument. This is necessary for
several reasons:

- `outPath` on derivations and Flake inputs is not a path.
- Derivations can be coerced to their `outPath` in string interpolation,
  but that produces strings, not paths.
- `builtins.path`, bizarrely, returns a string instead of a path (not
  that the documentation makes this clear).

If a path is used instead of a string here, then Nix will dutifully copy
the entire directory into a new path in the Nix store (ignored as
WONTFIX by Eelco in https://github.com/NixOS/nix/issues/9428). For
industrial use cases, this can result in an extra 10-15 seconds on every
single eval just to copy files from one spot in the Nix store to another
spot in the Nix store.

In #361424, this was changed so that `directory` must be a path,
breaking these use-cases.

I'm not really sure what happened here -- #361424 has very little
justification for why it exists, only a reference to a previous version
of the PR (#359941), which itself had very little justification given.
The description on #359941 explained that it would "Shrink the
function's code by ~2/3rd 🎉", but 60% of the reduction in size was just
deleting comments (!) and bindings like `directoryEntryIsPackage` that
helped clarify the intent of the implementation. As a result, the new
implementation is (to my eyes) more challenging to read and understand.
I think the whole thing was in service of #392800, which adds a
`newScope` argument in order "to create nested scopes for each
(sub)directory (not just the top-level one) when `newScope` is given."

Nobody noticed this regression until after the commit was merged. After
@phanirithvij pointed out the regression, @nbraud said they would
"shortly prepare a PR to fix this" [1] but did not. Later, they would
explain that they were "quite ill the last month(s)" [2], which explains
why this got forgotten about. @nbraud also requested a review from
@Gabriella439 [3], as she had reviewed the original PR adding
`lib.packagesFromDirectoryRecursive`, but not from me, the original
author of that PR. @Gabriella439 did not review the "refactor" PR, and
no attempt to contact her or myself was made after that initial request.
This behavior is admittedly rather subtle, so I'm not sure either
Gabriella or myself would have noticed the change (especially since the
relevant PR restructures the entire implementation).

While I find this a bit frustrating, I should have added a test for this
use-case in my original PR; if there was a test that relied on passing
paths in as a string, perhaps the authors modifying this code would have
noticed that the implementation was not an accident.

[1]: https://github.com/NixOS/nixpkgs/pull/361424#discussion_r1912407693
[2]: https://github.com/NixOS/nixpkgs/pull/359984#issuecomment-2775768808
[3]: https://github.com/NixOS/nixpkgs/pull/361424#issuecomment-2521308983
---
 lib/filesystem.nix |  5 ++---
 lib/tests/misc.nix | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/lib/filesystem.nix b/lib/filesystem.nix
index 1014c274041f..a950720602a7 100644
--- a/lib/filesystem.nix
+++ b/lib/filesystem.nix
@@ -385,7 +385,6 @@ in
         recurseIntoAttrs
         removeSuffix
         ;
-      inherit (lib.path) append;
 
       # Generate an attrset corresponding to a given directory.
       # This function is outside `packagesFromDirectoryRecursive`'s lambda expression,
@@ -396,7 +395,7 @@ in
           name: type:
           # for each directory entry
           let
-            path = append directory name;
+            path = directory + "/${name}";
           in
           if type == "directory" then
             {
@@ -429,7 +428,7 @@ in
       directory,
     }@args:
     let
-      defaultPath = append directory "package.nix";
+      defaultPath = directory + "/package.nix";
     in
     if pathExists defaultPath then
       # if `${directory}/package.nix` exists, call it directly
diff --git a/lib/tests/misc.nix b/lib/tests/misc.nix
index a723d198cb88..d9a72b29f2f4 100644
--- a/lib/tests/misc.nix
+++ b/lib/tests/misc.nix
@@ -4158,6 +4158,34 @@ runTests {
     };
   };
 
+  # Make sure that passing a string for the `directory` works.
+  #
+  # See: https://github.com/NixOS/nixpkgs/pull/361424#discussion_r1934813568
+  # See: https://github.com/NixOS/nix/issues/9428
+  testPackagesFromDirectoryRecursiveStringDirectory = {
+    expr = packagesFromDirectoryRecursive {
+      callPackage = path: overrides: import path overrides;
+      # Do NOT remove the `builtins.toString` call here!!!
+      directory = builtins.toString ./packages-from-directory/plain;
+    };
+    expected = {
+      a = "a";
+      b = "b";
+      # Note: Other files/directories in `./test-data/c/` are ignored and can be
+      # used by `package.nix`.
+      c = "c";
+      my-namespace = {
+        d = "d";
+        e = "e";
+        f = "f";
+        my-sub-namespace = {
+          g = "g";
+          h = "h";
+        };
+      };
+    };
+  };
+
   # Check that `packagesFromDirectoryRecursive` can process a directory with a
   # top-level `package.nix` file into a single package.
   testPackagesFromDirectoryRecursiveTopLevelPackageNix = {