lib.packagesFromDirectoryRecursive: init

[9999years]

Description of changes

Adds a packagesFromDirectoryRecursive helper, which combines builtins.readDir and callPackage to transform a directory of .nix package files into a matching nested attribute set of derivations.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.11 Release Notes (or backporting 23.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

Priorities

Add a 👍 reaction to pull requests you find important.

[infinisil]

Can you split out the documentation updates into a separate PR?

Regarding packagesFromDirectory, I'd rather make this be unified with pkgs/by-name than a separate convention. The 2-letter prefix of pkgs/by-name should definitely be optional, and the ability to use files or directories only could also be optional.

[9999years]

Split the documentation updates into #270696.

[9999years]

I'd rather make this be unified with pkgs/by-name than a separate convention.

That makes sense, but I'd rather not lose the recursive behavior here (subdirectories are also mapped to nested package attributes). I'll see if I can prototype a version with optional two-letter shards and optional directory recursion.

Where should I put tests for this?

[9999years]

OK, I've unified lib.packagesFromDirectory with the pkgs/by-name overlay. The pkgs/by-name tree is already set up in the same manner packagesFromDirectory expects, so I didn't need to make as many changes as anticipated (there's no extra directories without a package.nix for packagesFromDirectory to recurse into).

I've also reimplemented packagesFromDirectory in terms of a new helper, lib.packageFilesFromDirectory, which just returns a set of package paths and skips invoking callPackage. This also lets us hoist the directory traversal portion of the pkgs/by-name out of the overlay function itself, to save evaluation time if the overlay is applied multiple times.

[infinisil]

Thinking about it again, this does sound distinct enough that maybe it shouldn't be shared with pkgs/by-name. Especially since the latter comes with its own CI checking tool (pkgs/tests/nixpkgs-check-by-name) and an RFC.

I'm afraid I don't have a lot of capacity to review this right now though, I hope others can help out with this.

[9999years]

But this is totally compatible with pkgs/by-name and the pkgs/tests/nixpkgs-check-by-name tool?

[infinisil]

The function seems to support a superset of pkgs/by-name functionality. Using files instead of directories is only supported by function, not pkgs/by-name. Same for the recursive sets. Notably the additional checking of pkgs/by-name is only implemented in CI. So people could e.g. create pkgs/by-name/foo/foo.nix locally, and it works just fine locally, but then CI complains. Comparatively before this PR it just didn't work.

Furthermore, as soon as you have a package set there's the requirement of being able to override it. If you use lib.makeScope this gets handled somewhat decently at least for one level, but it's going to get messy for multiple levels. This really needs a lot of thought, and I don't have the capacity for this right now.

[Gabriella439]

I'm filling in to review since @infinisil indicated that he doesn't have time to give this a full review.

I think this is still okay to merge with just one small change. In particular, I'm not too concerned that the pkgs/by-name function might (hypothetically) in the future have different requirements than lib.packagesFromDirectory. If we ever get to that point it won't be hard to split the two implementations, but as far as I'm concerned the fact that this can be reused to powerpkgs/by-name functionality in its present state is a feature.

[Gabriella439]

The main thing I'll suggest is to make this take an attribute set instead of position arguments:

{ callPackage, directory }:

… that way if we need to add new optional arguments (with sensible defaults) we don't have to break backwards-compatibility. This would also be consistent with how pkgs.haskell.lib.packagesFromDirectory works (it also takes an attribute set)

[Gabriella439]

Oh, and also, have the attribute set ignore extra arguments (for forward compatibility), also the same as pkgs.haskell.lib.packagesFromDirectory:

{ callPackage, directory, ... }:

[9999years]

Done.

[nbraud]

@Gabriella439, could you clarify the forward-compatibility aspect? As-is, packagesFromDirectoryRecursive will silently ignore any unknown parameter, which seems to me more likely to cause subtle bugs than make things work on older nixpkgs version: presumably, new (optional) parameters would only be introduced (and specified) if they change the function's behaviour.

Having (older versions of) packagesFromDirectoryRecursive silently revert to the default behaviour, is then non-compatible (the behaviour is different) but the incompatibility is silent rather than an explicit error.

Sorry for coming rather late to the party: I only learnt of packagesFromDirectory yesterday, and it's great! Thank you and @9999years ❤️

[Gabriella439]

I'd be fine removing the .... I'm not too attached to it.

[nbraud]

I was more trying to ask if I was missing some part of the reasoning behind accepting ... 😅

If I accidentally convinced you that dropping ... would be preferable, should I do it as part of #359984 ?
It compounds the issue, by giving a usecase for putting extra parameters (used by recurseIntoDirectory rather than packagesFromDirectoryRecursive itself)

[infinisil]

+1 for dropping ...!

[tomberek]

I'm not sure this callPackage will do the right thing with packages that depend on each other. I can imagine using this via an identity function or an import and then using a standard callPackage.

That being said, this is a good addition into our standard library, often needed.

[9999years]

@tomberek Yeah, the documentation I was asked to remove made this clearer, but lib.packagesFromDirectory is most helpful in combination with lib.makeScope.

[RaitoBezarius]

I'm not a maintainer of lib per se but this seems to be a very useful addition and there's many out of tree code that reinvents the same thing all the time, it would be tidy if we could just reuse nixpkgs for that directly.

[infinisil]

Just minor comments, looking pretty good now!

[infinisil]

Not gonna bother waiting for CI, it's too slow.. I tested it locally, all good 👍

[philiptaron]

I really love this function. Thanks for adding it @9999years. 🙇🏻