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/attrsets: remove copies of recurseIntoAttrs, add recurseIntoAttrsIf #367489

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

Conversation

lucasew
Copy link
Contributor

@lucasew lucasew commented Dec 22, 2024

I actually wanted a way to enable nixpkgs-review to at least find which tests changed the derivation path but it seems it needs a little of preparation. The idea is to pass a type of parameter to nixpkgs and then it would eval tests recursively.

I am not dealing with NixOS tests mapping yet, only a low hanging fruit I found while looking for a way to do it.

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:
  • 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 Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Dec 22, 2024
@nix-owners nix-owners bot requested a review from infinisil December 22, 2024 23:39
…Attrs and dontRecurseIntoAttrs

Signed-off-by: lucasew <[email protected]>
@lucasew lucasew force-pushed the 20241222-poc-recurse-tests branch from 34fb292 to 1e5e58a Compare December 22, 2024 23:46
@infinisil
Copy link
Member

I don't get the motivation behind this change, can you explain it more?

@lucasew
Copy link
Contributor Author

lucasew commented Dec 23, 2024

I don't get the motivation behind this change, can you explain it more?

I want to parametrize whether some kind of builder would recurse into a attrset based on a condition and found this low hanging fruit while I was looking for a way to do it.

@lucasew
Copy link
Contributor Author

lucasew commented Dec 23, 2024

BTW the review bot is running now a test that will result in less messages sent.

Basically a map reduce that waits the workers to be done then send all their report.mds in one message.

Currently is just a cat reports/**/*.md > report.md but let's see how it goes.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4995

@lucasew lucasew requested a review from roberth December 30, 2024 01:47
@roberth
Copy link
Member

roberth commented Dec 30, 2024

I would expect this to behave as

rec {
/** Apply a function if the condition holds; return argument unmodified otherwise. */
applyIf = cond: f: if cond then f else (x: x);
recurseIntoAttrsIf = cond: applyIf cond recurseIntoAttrs;
}

With applyIf, we don't need any of the other *If helpers.
I don't think we have this yet?
I've used the name when for it in some Nix, Elm or Haskell code, but something more explicit is probably better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants