mirror of
https://github.com/NixOS/nixpkgs.git
synced 2025-11-12 10:43:16 +01:00
4a81a5e556
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 |
||
|---|---|---|
| .. | ||
| modules | ||
| packages-from-directory | ||
| .editorconfig | ||
| check-eval.nix | ||
| fetchers.nix | ||
| filesystem.sh | ||
| maintainer-module.nix | ||
| maintainers.nix | ||
| misc.nix | ||
| modules.sh | ||
| network.sh | ||
| nix-for-tests.nix | ||
| release.nix | ||
| sources.sh | ||
| systems.nix | ||
| teams.nix | ||
| test-to-plist-escaped-expected.plist | ||
| test-to-plist-unescaped-expected.plist | ||
| test-with-nix.nix | ||