Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Nov 28, 2024

Follow-up on #359941, blocked on it as this PR contains commits from it.
Blocked on #359898 and #361424.

  • Extend lib.filesystem.packagesFromDirectoryRecursive with support for explicit recursion.
  • Update packagesFromDirectoryRecursive to create nested scopes for each (sub)directory (not just the top-level one) when newScope is given.
  • Apply recurseIntoAttrs by default when recursing into a directory.
  • Add a test using scopes for packagesFromDirectoryRecursive, ensuring:
    • packages can depend on other packages in the same scope and in ancestor scopes (the scope's parent or grandparent) ;
    • packages can have mutual dependencies.
  • Reject unknown parameters (breaking change!)
  • Add an example showing how to use recurseIntoDirectory.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested nix-instantiate --eval --strict lib/tests/misc.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
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Note
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@nbraud nbraud added 0.kind: enhancement Add something new 9.needs: maintainer feedback 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed needs_reviewer (old Marvin label, do not use) 8.has: tests This PR has tests labels Nov 28, 2024
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 28, 2024
@nbraud

This comment was marked as resolved.

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 1753d6d to 4c6c2ec Compare November 28, 2024 22:22
@nbraud
Copy link
Contributor Author

nbraud commented Nov 28, 2024

Ran nixfmt over the new test fixtures

lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

Rebased following the parent PR being merged

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 4c6c2ec to 8a5830c Compare December 1, 2024 10:09
@nbraud nbraud marked this pull request as ready for review December 1, 2024 10:09
@nix-owners nix-owners bot requested a review from infinisil December 1, 2024 10:11
lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

I'd prefer to land #359898 first, as there will be a merge conflict over the function's documentation, and having the improved documentation will make documenting this PR's changes simpler.

PS: Marked as draft again, since I'll wait for that to land before writing documentation in this PR.

@nbraud nbraud marked this pull request as draft December 1, 2024 10:45
lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 85b9516 to ecca66e Compare December 3, 2024 12:45
@nbraud
Copy link
Contributor Author

nbraud commented Dec 3, 2024

Rebased atop both blocking PRs, added documentation.

Still needs an example how to use recurseIntoDirectory

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 29, 2024
@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

Fixed up the release notes, putting those entries under the lib section

@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

@NixOS/ofborg-maintainers, where does one get details on that ofborg-internal-error ?

I'm guessing @ofborg choked on something which it wasn't previously recursing into, possibly the FreeBSD-specific packages? (for which it has no builder)

…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.
@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

Squashed fixup! commits, no diff

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from e2bff52 to 3627f0a Compare December 30, 2024 14:25
@nbraud

This comment was marked as outdated.

@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

Using recurseIntoAttrs by default will involve fixing some callers of packagesFromDirectoryRecursive in nixpkgs, so I am pushing it back to a later PR.

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 3627f0a to 0fe9ad2 Compare December 30, 2024 15:03
@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 359984


x86_64-linux

⏩ 1 package blacklisted:
  • nixos-install-tools
✅ 1 package built:
  • nixpkgs-manual

@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

This is only waiting for reviews, no functional change since the beginning of the month: only rebased to address the merge conflict and squash fixup commits, and moved the release notes entries. (One further change was tried and reverted, to be done in #369421)

@9999years if you re-review, I think we could get that merged soonish

Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes. Thank you for the efforts that you already put into it.

# recurseIntoDirectory can modify the function used when processing directory entries
# and recurseArgs can (optionally) hold data for its use ; see nixdoc above
recurseArgs ? throw "lib.packagesFromDirectoryRecursive: recurseArgs wasn't passed in args",
recurseIntoDirectory ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function could be its own unit. For example defaultRecurseIntoDirectory. This makes it possible to unit-test and reuse this function. I think inline here is a little bit unhappy.

Also this inline documentation as plain comment will not render into any documentation.

If you pull this out you can write a nice doc-comment describing the behavior.
Place mutual references in the parent function-library-lib.filesets.packagesFromDirectoryRecursive
This will make it much easier for people to understand and implement their own function if needed.

/** 
  Create a new scope and mark it `recurseForDerivations`.
  This lets the packages refer to each other.
  
  See also:
  - [lib.makeScope](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.customisation.makeScope) and
  - [lib.recurseIntoAttrs](https://nixos.org/manual/nixpkgs/unstable/#function-library-lib.customisation.makeScope)
*/
defaultRecurseIntoDirectory = args: if args ? newScope then ...

...
}:
# recurseIntoDirectory can modify the function used when processing directory entries
# and recurseArgs can (optionally) hold data for its use ; see nixdoc above
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# and recurseArgs can (optionally) hold data for its use ; see nixdoc above
# and recurseArgs can (optionally) hold data for its use ; see comment above

[`lib.packagesFromDirectoryRecursive`]: https://nixos.org/manual/nixpkgs/stable/#function-library-lib.filesystem.packagesFromDirectoryRecursive
[`lib.recurseIntoAttrs`]: https://nixos.org/manual/nixpkgs/stable/#function-library-lib.attrsets.recurseIntoAttrs

### Other notable changes {#sec-release-25.05-lib-notable-changes}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we used a different heading in all the previous release notes:

Suggested change
### Other notable changes {#sec-release-25.05-lib-notable-changes}
### Additions and Improvements {#sec-release-25.05-lib-additions-improvements}

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lib The Nixpkgs function library 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person ofborg-internal-error Ofborg encountered an error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants