lib.packagesFromDirectoryRecursive: refactor (v2) #361424

nbraud · 2024-12-03T11:18:24Z

Second go at #359941

An assertion was too restrictive and broke the installer's tests, due to directory being a symlink.

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
- x86_64-darwin
- aarch64-darwin
Tested, as applicable:
- NixOS test installer
- nix-instantiate --eval --strict lib/tests/misc.nix
- nix-build lib/tests/release.nix
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
Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

nbraud · 2024-12-03T11:19:10Z

@ofborg test installer

nbraud · 2024-12-03T11:25:07Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 361424

`aarch64-darwin`

✅ 1 package built:

nixpkgs-manual

nbraud · 2024-12-03T11:26:56Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 361424

`x86_64-linux`

⏩ 1 package blacklisted:

nixos-install-tools

✅ 1 package built:

nixpkgs-manual

nbraud · 2024-12-03T12:32:00Z

Successfully built nixosTests.installer locally (on NixOS x86-64)

PS: ofBorg seems to have bailed out without completing the test (no error in the log, it just stops partway through), possibly due to a timeout or such? I had noticed the test was running very slowly (based on the live log output)

nbraud · 2024-12-03T12:37:07Z

@K900 I requested your review so you'd have a chance to confirm whether this cause issues with the installer, as I am unsure whether nixosTests.installer covers everything necessary.

K900 · 2024-12-03T13:07:31Z

Ofborg's lib tests can help here.

nbraud · 2024-12-03T14:42:53Z

Ofborg's lib tests can help here.

I already ran the lib tests locally (see PR checklist) but they didn't catch the issue in the previous PR either.

K900 · 2024-12-03T14:45:06Z

You want to run lib/tests/release.nix.

nbraud · 2024-12-03T14:50:35Z

You want to run lib/tests/release.nix.

Thanks for the pointer, and that passes too.

nbraud · 2024-12-04T16:49:05Z

@infinisil @K900, could one of you OK this (and potentially merge) ?

This is blocking other work on lib.packageFromDirectoryRecursive :3

nbraud · 2024-12-04T20:33:17Z

@K900 Is “severity: blocker” not the appropriate label here? Given that #359984 is stuck in draft mode until this is completed, it fits the description:

This is preventing another PR or issue from being completed.

K900 · 2024-12-05T10:59:04Z

"Blocker" generally refers to things that are blocking major changes, not feature additions.

nbraud · 2024-12-05T20:14:49Z

"Blocker" generally refers to things that are blocking major changes, not feature additions.

OK, then the label description should be updated to reflect that.

nbraud · 2024-12-05T20:16:37Z

@Gabriella439, as you reviewed the o.g. PR, would you be willing to review this?
The only difference is the removal of assert pathIsDirectory directory;

infinisil

Didn't have the chance to review this the first time, so here's a late one. Just minor things though, looking good overall :)

lib/filesystem.nix

infinisil · 2024-12-11T11:38:23Z

lib/filesystem.nix

+    if pathIsRegularFile defaultPath then
+      # if `${directory}/package.nix` exists, call it directly
+      callPackage defaultPath {}
+    else lib.concatMapAttrs (name: type:


Suggested change

else lib.concatMapAttrs (name: type:

else concatMapAttrs (name: type:

And add concatMapAttrs to the import list at the top. A bit annoying, but this is the standard to keep lib more performant.

That implies pretty silly things about the (C++) implementation, and seems like a very-premature optimization, especially considering that the filesystem access should be much slower

... but I'm not opposed to doing it either (as well as for lib.removeSuffix) 😅

I was told it is better for eval performance: NixOS#361424 (comment)

nbraud · 2024-12-12T18:10:18Z

@infinisil Thanks for catching those, and done!

This is a second take on NixOS#359941, which was reverted by 940db57. Co-authored-by: Silvan Mosberger <[email protected]>

I was told it is better for eval performance: NixOS#361424 (comment)

infinisil

LGTM! Did you want to squash yourself? Feel free to merge yourself as you wish :)

infinisil · 2024-12-12T23:02:39Z

lib/filesystem.nix

+      inherit (lib) concatMapAttrs removeSuffix;
+      inherit (lib.path) append;


Usually this would be added to the very top of this file, but after thinking about it, the performance benefit is also there if you do it here. In any case, that's a tiny detail and not worth nitpicking over further :)

nbraud · 2024-12-28T21:10:33Z

Squashed fixup! commit, no diff (so previous review and CI results remain valid)

nbraud · 2024-12-28T21:11:38Z

Thanks again for the reviews, @infinisil <3

I was told it is better for eval performance: NixOS/nixpkgs#361424 (comment)

phanirithvij · 2025-01-04T13:39:26Z

lib/filesystem.nix

-          (packageDirectoryEntries path);
+      inherit (lib) concatMapAttrs removeSuffix;
+      inherit (lib.path) append;
+      defaultPath = append directory "package.nix";


Previously directory could be a string but this forces it to be a path.
But I guess this change is heading in the right direction and users should adapt?

for e.g. https://github.com/9999years/git-prole/blob/68b4ba943a4bbb3d8452c67bbf50b0ba3f58956a/nix/makePackages.nix#L12

thought I could do append /. "string-dir" and pass it as the directory, but it is not allowed either.

Oof, good point, that wasn't an intended regression; if it was, at the very least it should be documented as such.

On the one hand, pushing for more-structured path handling makes sense. On the other, breaking an API present in stable releases (esp. in a relatively-subtle way) is not how I'd prefer to push that forward.

I'll shortly prepare a PR to fix this, thanks a lot for the report.
PS: “shortly” might take longer than expected, everything hurts today 😬

github-actions bot added the 6.topic: lib The Nixpkgs function library label Dec 3, 2024

nix-owners bot requested a review from infinisil December 3, 2024 11:19

github-actions bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Dec 3, 2024

nbraud mentioned this pull request Dec 3, 2024

lib.packagesFromDirectoryRecursive: use explicit recursion, support nested scopes #359984

Open

15 tasks

nbraud requested a review from K900 December 3, 2024 12:32

nbraud pushed a commit to nbraud/nixpkgs that referenced this pull request Dec 3, 2024

Merge NixOS#359898 and NixOS#361424

9c65740

ofborg bot added the 10.rebuild-darwin: 1 label Dec 4, 2024

nbraud added 1.severity: blocker This is preventing another PR or issue from being completed needs_reviewer (old Marvin label, do not use) labels Dec 4, 2024

K900 removed the 1.severity: blocker This is preventing another PR or issue from being completed label Dec 4, 2024

infinisil requested changes Dec 11, 2024

View reviewed changes

github-actions bot removed the 10.rebuild-darwin: 1 label Dec 12, 2024

nbraud requested a review from infinisil December 12, 2024 18:08

nbraud added a commit to nbraud/nixpkgs that referenced this pull request Dec 12, 2024

lib.packagesFromDirectoryRecursive: let-bind functions called from lib

6b8521e

I was told it is better for eval performance: NixOS#361424 (comment)

nbraud and others added 2 commits December 12, 2024 18:48

lib.packagesFromDirectoryRecursive: refactor, again

1db8650

This is a second take on NixOS#359941, which was reverted by 940db57. Co-authored-by: Silvan Mosberger <[email protected]>

lib.packagesFromDirectoryRecursive: let-bind functions called from lib

065b480

I was told it is better for eval performance: NixOS#361424 (comment)

infinisil approved these changes Dec 12, 2024

View reviewed changes

ofborg bot added the 10.rebuild-darwin: 1 label Dec 13, 2024

wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 15, 2024

nbraud force-pushed the lib/fs/packagesFromDir-refactor branch from 6b8521e to 065b480 Compare December 28, 2024 21:10

nbraud merged commit f91fbd1 into NixOS:master Dec 28, 2024
18 of 19 checks passed

nbraud deleted the lib/fs/packagesFromDir-refactor branch December 28, 2024 21:11

github-actions bot removed the 10.rebuild-darwin: 1 label Dec 28, 2024

khaneliman pushed a commit to khaneliman/nixpkgs that referenced this pull request Dec 28, 2024

Merge NixOS#361424: refactor lib.packagesFromDirectoryRecursive (v2)

cd05003

github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Dec 29, 2024

lib.packagesFromDirectoryRecursive: let-bind functions called from lib

a76abd2

I was told it is better for eval performance: NixOS/nixpkgs#361424 (comment)

phanirithvij reviewed Jan 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.packagesFromDirectoryRecursive: refactor (v2) #361424

lib.packagesFromDirectoryRecursive: refactor (v2) #361424

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 3, 2024

nbraud commented Dec 3, 2024

nbraud commented Dec 3, 2024

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 3, 2024 •

edited

Loading

K900 commented Dec 3, 2024

nbraud commented Dec 3, 2024 •

edited

Loading

K900 commented Dec 3, 2024

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 4, 2024

nbraud commented Dec 4, 2024 •

edited

Loading

K900 commented Dec 5, 2024

nbraud commented Dec 5, 2024

nbraud commented Dec 5, 2024

infinisil left a comment

infinisil Dec 11, 2024

nbraud Dec 12, 2024 •

edited

Loading

nbraud commented Dec 12, 2024

infinisil left a comment

infinisil Dec 12, 2024 •

edited

Loading

nbraud commented Dec 28, 2024

nbraud commented Dec 28, 2024

phanirithvij Jan 4, 2025 •

edited

Loading

nbraud Jan 12, 2025 •

edited

Loading

	else lib.concatMapAttrs (name: type:
	else concatMapAttrs (name: type:

		inherit (lib) concatMapAttrs removeSuffix;
		inherit (lib.path) append;

lib.packagesFromDirectoryRecursive: refactor (v2) #361424

lib.packagesFromDirectoryRecursive: refactor (v2) #361424

Conversation

nbraud commented Dec 3, 2024 • edited Loading

Things done

nbraud commented Dec 3, 2024

nbraud commented Dec 3, 2024

nixpkgs-review result

aarch64-darwin

nbraud commented Dec 3, 2024

nixpkgs-review result

x86_64-linux

nbraud commented Dec 3, 2024 • edited Loading

nbraud commented Dec 3, 2024 • edited Loading

K900 commented Dec 3, 2024

nbraud commented Dec 3, 2024 • edited Loading

K900 commented Dec 3, 2024

nbraud commented Dec 3, 2024 • edited Loading

nbraud commented Dec 4, 2024

nbraud commented Dec 4, 2024 • edited Loading

K900 commented Dec 5, 2024

nbraud commented Dec 5, 2024

nbraud commented Dec 5, 2024

infinisil left a comment

Choose a reason for hiding this comment

infinisil Dec 11, 2024

Choose a reason for hiding this comment

nbraud Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

nbraud commented Dec 12, 2024

infinisil left a comment

Choose a reason for hiding this comment

infinisil Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

nbraud commented Dec 28, 2024

nbraud commented Dec 28, 2024

phanirithvij Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

nbraud Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

nbraud commented Dec 3, 2024 •

edited

Loading

`nixpkgs-review` result

`aarch64-darwin`

`nixpkgs-review` result

`x86_64-linux`

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 3, 2024 •

edited

Loading

nbraud commented Dec 4, 2024 •

edited

Loading

nbraud Dec 12, 2024 •

edited

Loading

infinisil Dec 12, 2024 •

edited

Loading

phanirithvij Jan 4, 2025 •

edited

Loading

nbraud Jan 12, 2025 •

edited

Loading