lib.packagesFromDirectoryRecursive: mark directories with `recurseIntoAttrs` #369421

nbraud · 2024-12-30T15:17:05Z

Split off #359984, at this requires fixes in the BSD package sets.

Things done

Built on platform(s)
- x86_64-linux
- aarch64-linux
- x86_64-darwin
- aarch64-darwin
Tested, as applicable:
- tests in lib/tests
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
25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
- (Function updates) Added a release notes entry if the change is significant
Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Found by `deadnix` 🖤

in preparation for adding more tests for `lib.packagesFromDirectoryRecursive`

…ested scopes

…en `newScope` is provided Co-authored-by: Rebecca Turner <[email protected]>

Fixes a bug preventing `recurseIntoDirectory` from changing the `directory` argument. Moreover, `processDir` now cannot capture arguments from the `packagesFromDirectoryRecursive` call, entirely preventing this class of bug from reoccurring should new parameters be added etc.

see [discussion](NixOS#270537 (comment))

…rectory`

…oAttrs` This makes the `recurseIntoAttrs` behaviour consistent regardless of whether `newScope` is provided, and is more coherent with nixpkgs convention.

Issue exposed when `packagesFromDirectoryRecursive` set `recurseForDerivations`, as QA automation could then find the `libspl` derivation.

nbraud · 2024-12-30T15:27:20Z

@artemist @emilazy @rhelmot Pinging you as you seem to be the frequent committers to pkgs/os-specific/bsd :3

(also “hi, long time no see” to some of you ^^)

pkgs/os-specific/bsd/freebsd/pkgs/mkDerivation.nix

nbraud · 2024-12-30T15:43:07Z

freebsd.ports' fetchzip fails because CGit returns 400 “Bad Request.” I guess cgit.freebsd.org only keeps prebuilt snapshots for a limited period of time.

Unfortunately, fetchgit also has known issues fetching commits by hash (rather than tag or other ref) but it still seems more-stable than soon-to-expire snapshots, so I'm switching it over. (PS: Oops, I first forgot to set the commit hash 😅 )

rhelmot · 2024-12-30T15:46:18Z

I've been trying my hardest to nuke freebsd.ports. I think it's a bit of a code smell to have in nixpkgs, since it's hard to make it play nice with the rest of the tree... If we can't get rid of it now then I suppose it should be fixed, but I'm making a 😬 face.

nbraud · 2024-12-30T15:56:34Z

I think it's a bit of a code smell to have [freebsd.ports] in nixpkgs, since it's hard to make it play nice with the rest of the tree...

Entirely fair, and I won't stand in the way of removing it. I'm just going for the lowest-touch change that won't break things when setting recurseIntoAttrs by default... mostly because I have no idea about the BSD-specific parts of nixpkgs, even though I'd been meaning to get into it. 😅
Speaking of, thanks a bunch for your work there, and in nixpkgs in general. I think it's great for our software distribution which is useable on a wider variety of opensource OSes.

Going back to the issue at hand, in principle we could tar the {free, net, open}bsd attrsets with the dontRecurseIntoAttrs brush, but I'd prefer not: that would effectively hide the os-specific/bsd tree from nixpkgs' QA tooling, when this change exposed issues like libspl's broken metadata.

rhelmot · 2024-12-30T16:00:56Z

Yeah, I think we should totally have recursion enabled! The review comment I left was asking what issues you had with the default platforms = lib.platforms.unix in freebsd.mkDerivation.

nbraud · 2024-12-30T16:07:21Z

The review comment I left

Oops, sorry I completely missed that one 😅

SigmaSquadron · 2024-12-30T16:54:00Z

Doesn't this break everyone's usage of packagesFromDirectoryRecursive? I love how simple and useful that function is, but adding a bunch of scope-related inputs seems counter-intuitive for most use cases. Couldn't this be a separate lib function instead of breaking packagesFromDirectoryRecursive?

nbraud · 2024-12-30T19:02:22Z

Doesn't this break everyone's usage of packagesFromDirectoryRecursive?

By “this,” do you mean applying recurseIntoAttrs when recursing into a directory?
Note that only the last 3 commits are specific to this PR; everything else is from #359984, which quite-deliberately does not change the default behaviour at all.

I love how simple and useful that function is, but adding a bunch of scope-related inputs seems counter-intuitive for most use cases. Couldn't this be a separate lib function instead of breaking packagesFromDirectoryRecursive?

The scope-related machinery is optional, and not used by default; there might be a misunderstanding rather anything “breaking.”

SigmaSquadron · 2024-12-30T19:04:44Z

The scope-related machinery is optional, and not used by default; there might be a misunderstanding rather anything “breaking.”

It's very likely that I misunderstood the diff. I thought the optional newScope attribute defaulted to a throw, making it effectively mandatory and forcing every usage of packagesFromDirectoryRecursive to define a scope.

nbraud · 2024-12-30T19:10:49Z

It's very likely that I misunderstood the diff. I thought the optional newScope attribute defaulted to a throw, making it effectively mandatory and forcing every usage of packagesFromDirectoryRecursive to define a scope.

Ah, no, not at all. Some arguments default to throw to make them optional while still rejecting unknown arguments (bf5cd41)
I could have used null I guess, but it seemed clearer to use a throw when using the default value at all is a bug (in packagesFromDirectoryRecursive's implementation)

SigmaSquadron · 2024-12-30T19:12:41Z

I see! Thank you for explaining; since this doesn't alter packagesFromDirectoryRecursive's current behaviour, feel free to disregard my concerns.

nbraud · 2024-12-30T19:14:35Z

I see! Thank you for explaining; since this doesn't alter packagesFromDirectoryRecursive's current behaviour, feel free to disregard my concerns.

Thanks for clarifying your concerns. ❤️

This draft PR does alter the behaviour, but only in that attrsets corresponding to directories are tagged with lib.recurseIntoAttrs so various tools can find the derivations they contain.

Many FreeBSD-specific drvs with undefined platform fail to build on Linux.

Many NetBSD-specific drvs with undefined platform fail to build on Linux.

The snapshot URL returned error 400 “Bad request,” likely due to CGit expiring snapshots.

nbraud · 2024-12-30T22:31:22Z

`nixpkgs-review` result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 369421

`x86_64-linux`

⏩ 1 package marked as broken and skipped:

emacsPackages.manualPackages.structured-haskell-mode

⏩ 1 package blacklisted:

nixos-install-tools

✅ 68 packages built:

emacsPackages.manualPackages.mu4e
emacsPackages.manualPackages.notdeft
emacsPackages.manualPackages.ott-mode
emacsPackages.manualPackages.pod-mode
emacsPackages.manualPackages.pod-mode.doc
emacsPackages.manualPackages.prisma-mode
emacsPackages.manualPackages.prolog-mode
emacsPackages.manualPackages.rect-mark
emacsPackages.manualPackages.sunrise-commander
emacsPackages.manualPackages.sv-kalender
emacsPackages.manualPackages.texpresso
emacsPackages.manualPackages.tree-sitter-langs
emacsPackages.manualPackages.tsc
emacsPackages.manualPackages.urweb-mode
emacsPackages.manualPackages.voicemacs
emacsPackages.manualPackages.wat-mode
emacsPackages.manualPackages.xapian-lite
emacsPackages.manualPackages.yes-no
emacsPackages.manualPackages.youtube-dl
emacsPackages.manualPackages.zstd
freebsd.boot-install
freebsd.compat
freebsd.freebsdSetupHook
freebsd.libnetbsd
freebsd.lorder
freebsd.lorder.man
freebsd.makeMinimal
freebsd.ports
freebsd.source
freebsd.xargs-j
netbsd.column
netbsd.compat
netbsd.compat.dev
netbsd.config
netbsd.dict
netbsd.fts
netbsd.genassym
netbsd.gencat
netbsd.getconf
netbsd.getent
netbsd.install
netbsd.libterminfo
netbsd.lorder
netbsd.make-rules
netbsd.makeMinimal
netbsd.misc
netbsd.mknod
netbsd.mtree
netbsd.nbperf
netbsd.netbsdSetupHook
netbsd.rpcgen
netbsd.source
netbsd.stat
netbsd.statHook
netbsd.tic
netbsd.tsort
netbsd.uudecode
nixpkgs-manual
openbsd.boot-config
openbsd.boot-ctags
openbsd.compat
openbsd.compatHook
openbsd.lorder
openbsd.make-rules
openbsd.makeMinimal
openbsd.makefs
openbsd.openbsdSetupHook
openbsd.source

nbraud · 2024-12-31T10:19:01Z

@rhelmot I'd like a new review, but you can wait until the blocker is merged if that's easier. (At least GitHub won't show the parent PR in the diff)

nbraud added 10 commits December 28, 2024 21:38

lib.filesystem: remove dead let-bindings

b377da6

Found by `deadnix` 🖤

lib/tests: move packages-from-directory/* into a subdir

3eb6c8c

in preparation for adding more tests for `lib.packagesFromDirectoryRecursive`

lib.packagesFromDirectoryRecursive: use explicit recursion, support n…

03968c4

…ested scopes

lib.packagesFromDirectoryRecursive: add tests for nested scopes

b0df85b

lib.packagesFromDirectoryRecursive: default to creating new scopes wh…

3ae1e97

…en `newScope` is provided Co-authored-by: Rebecca Turner <[email protected]>

lib.packagesFromDirectoryRecursive: reject unknown arguments

bf5cd41

see [discussion](NixOS#270537 (comment))

lib.packagesFromDirectoryRecursive: Add example use of `recurseIntoDi…

0fe9ad2

…rectory`

lib.packagesFromDirectoryRecursive: mark directories with `recurseInt…

baff116

…oAttrs` This makes the `recurseIntoAttrs` behaviour consistent regardless of whether `newScope` is provided, and is more coherent with nixpkgs convention.

libspl: Fix metadata

ab70a80

Issue exposed when `packagesFromDirectoryRecursive` set `recurseForDerivations`, as QA automation could then find the `libspl` derivation.

nbraud added 0.kind: enhancement Add something new 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed labels Dec 30, 2024

This comment was marked as outdated.

Sign in to view

rhelmot reviewed Dec 30, 2024

View reviewed changes

pkgs/os-specific/bsd/freebsd/pkgs/mkDerivation.nix Show resolved Hide resolved

nbraud force-pushed the lib/fs/packagesFromDir/recurseIntoAttrs branch from d59d323 to f23618e Compare December 30, 2024 15:50

This comment was marked as outdated.

Sign in to view

nbraud mentioned this pull request Dec 30, 2024

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

Open

12 tasks

nbraud added 4 commits December 30, 2024 22:08

pkgs/os-specific/freebsd: Default meta.platforms to freebsd

ce4fe32

Many FreeBSD-specific drvs with undefined platform fail to build on Linux.

pkgs/os-specific/netbsd: Default meta.platforms to netbsd

d7f374c

Many NetBSD-specific drvs with undefined platform fail to build on Linux.

freebsd.ports: Switch to fetchgit, fix build

feb2c4f

The snapshot URL returned error 400 “Bad request,” likely due to CGit expiring snapshots.

fixup! pkgs/os-specific/freebsd: Default meta.platforms to freebsd

a77d3bf

nbraud force-pushed the lib/fs/packagesFromDir/recurseIntoAttrs branch from f23618e to a77d3bf Compare December 30, 2024 22:14

nbraud added 2 commits December 30, 2024 22:24

fixup! pkgs/os-specific/netbsd: Default meta.platforms to netbsd

c86512d

fixup! pkgs/os-specific/netbsd: Default meta.platforms to netbsd

ca37ca6

github-actions bot added 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500 and removed 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100 labels Dec 30, 2024

ofborg bot added the 8.has: package (new) This PR adds a new package label Dec 31, 2024

nbraud requested a review from rhelmot December 31, 2024 10:17

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 4, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib.packagesFromDirectoryRecursive: mark directories with `recurseIntoAttrs` #369421

lib.packagesFromDirectoryRecursive: mark directories with `recurseIntoAttrs` #369421

nbraud commented Dec 30, 2024

This comment was marked as outdated.

nbraud commented Dec 30, 2024

nbraud commented Dec 30, 2024 •

edited

Loading

rhelmot commented Dec 30, 2024

nbraud commented Dec 30, 2024 •

edited

Loading

rhelmot commented Dec 30, 2024

This comment was marked as outdated.

nbraud commented Dec 30, 2024

SigmaSquadron commented Dec 30, 2024 •

edited

Loading

nbraud commented Dec 30, 2024

SigmaSquadron commented Dec 30, 2024

nbraud commented Dec 30, 2024 •

edited

Loading

SigmaSquadron commented Dec 30, 2024

nbraud commented Dec 30, 2024

nbraud commented Dec 30, 2024

nbraud commented Dec 31, 2024

lib.packagesFromDirectoryRecursive: mark directories with recurseIntoAttrs #369421

Are you sure you want to change the base?

lib.packagesFromDirectoryRecursive: mark directories with recurseIntoAttrs #369421

Conversation

nbraud commented Dec 30, 2024

Things done

This comment was marked as outdated.

nbraud commented Dec 30, 2024

nbraud commented Dec 30, 2024 • edited Loading

rhelmot commented Dec 30, 2024

nbraud commented Dec 30, 2024 • edited Loading

rhelmot commented Dec 30, 2024

This comment was marked as outdated.

nbraud commented Dec 30, 2024

SigmaSquadron commented Dec 30, 2024 • edited Loading

nbraud commented Dec 30, 2024

SigmaSquadron commented Dec 30, 2024

nbraud commented Dec 30, 2024 • edited Loading

SigmaSquadron commented Dec 30, 2024

nbraud commented Dec 30, 2024

nbraud commented Dec 30, 2024

nixpkgs-review result

x86_64-linux

nbraud commented Dec 31, 2024

lib.packagesFromDirectoryRecursive: mark directories with `recurseIntoAttrs` #369421

lib.packagesFromDirectoryRecursive: mark directories with `recurseIntoAttrs` #369421

nbraud commented Dec 30, 2024 •

edited

Loading

nbraud commented Dec 30, 2024 •

edited

Loading

SigmaSquadron commented Dec 30, 2024 •

edited

Loading

nbraud commented Dec 30, 2024 •

edited

Loading

`nixpkgs-review` result

`x86_64-linux`