Skip to content

[WIP] Multiple by-name directory support#180

Draft
mdaniels5757 wants to merge 21 commits intoNixOS:mainfrom
mdaniels5757:multi-by-name
Draft

[WIP] Multiple by-name directory support#180
mdaniels5757 wants to merge 21 commits intoNixOS:mainfrom
mdaniels5757:multi-by-name

Conversation

@mdaniels5757
Copy link
Member

@mdaniels5757 mdaniels5757 commented Oct 17, 2025

This works in tests only.

Known issues:

  1. This is my first time really programming in Rust, and, um, that's probably evident.
  2. Memory usage during eval is absurd at the moment (45GB+ per eval), and then the stack overflows/infinite recursion.
  3. This takes about 2.5x the amount of time it did before. I need to profile.

One thing that may or may not be an issue, is that I mostly did not follow the instructions @infinisil so kindly left on #31. Not sure which of those should or should not be implemented.

Feedback is welcome! I would ask you please refrain for now from e.g. pointing out commented-out code and the like. (I'm well aware this needs cleanup.) But more function- or design- focused feedback, or feedback related to the points above, is definitely welcome (and probably needed :) )!

@fgaz
Copy link
Member

fgaz commented Oct 17, 2025

This doesn't yet handle the case in which, e.g., tclPackages.expect and expect are both defined.

fyi we'll soon have tcl8Packages and tcl9Packages as well NixOS/nixpkgs#433414

@mdaniels5757 mdaniels5757 force-pushed the multi-by-name branch 2 times, most recently from 92623ed to 43a6a7e Compare November 11, 2025 21:04
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Finally got around to taking a closer look at this, and overall this is already looking very solid, thanks a lot!

For now I'll just comment for the biggest issue I've noticed, I can do another review when that's addressed.

{
id = "py";
path = "pkgs/development/python-modules/by-name";
attr_path_regex = "^(python3\\d*Packages|python3\\d*.pkgs)\\..*$";
Copy link
Member

Choose a reason for hiding this comment

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

The biggest issue with how it's done currently closely relates to this here. As far as I understand, the new eval.nix/eval.rs does a recursive Nixpkgs evaluation, lists all resulting attribute paths and then checks which ones match the regex defined here.

This complicates the eval.nix because it needs to handle all the random failing evaluations in Nixpkgs, making it very brittle and slow.

Instead, please change this configuration options to be like suggested in #31:

attr_path = [ "python3Packages" ];

Notably we don't need to check all the different version sets like python311Packages, etc., because the only reason we need to evaluate anything at all is for the eval-time checks and ratchet checks. Since all the python3*Packages sets only differ in very minor ways, checking one of them is good enough. This will be much more efficient and simplify the code a lot.

On the implementation side, the code then only needs to evaluate the attributes directly under that attribute path, just like the eval.nix in the main branch (I'm afraid a lot of your eval.nix changes won't be needed 😅) . I also recommend changing what eval.nix outputs to be more suited to the new functionality, e.g. something like

{
  py = [ [ name value ] ... ];
  main = [ [ name value ] ... ];
  tcl = [ [ name value ] ... ];
}

This should make the diff to what it was fairly small.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants