lib.evalModules: add graph attribute#403839
Conversation
b8928a3 to
2a5c8ff
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/obtaining-a-nixpkgs-module-system-configuration-modules-graph/63286/4 |
e2f87c8 to
724fccd
Compare
|
Should this new attribute be nested inside another attrset to reserve a place for more metrics in the future? Does the design otherwise make sense? To us it seems like this does not change a hot evaluation path. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/obtaining-a-nixpkgs-module-system-configuration-modules-graph/63286/5 |
|
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/5547 |
|
Since our CI only performs a nixpkgs evaluation, for this PR it would be nice to also include a full nixos evaluation comparison. Before: nixos = import ./nixos/lib/eval-config.nix {
modules = [
{
fileSystems."/".device = "/dev/null";
boot.loader.systemd-boot.enable = true;
}
{
nixpkgs.system = "x86_64-linux";
}
];
};Command: Note: CPU, and time is noisy, but i.e.
|
hsjobeki
left a comment
There was a problem hiding this comment.
Overall looking very nice. Some minor things should be fixed.
lib/modules.nix
Outdated
There was a problem hiding this comment.
The term StructuredModule is never defined, would be nice to add a defining comment somewhere
There was a problem hiding this comment.
I kind of don't want to. Perhaps unnecesarry?
roberth
left a comment
There was a problem hiding this comment.
I believe it's possible to significantly reduce the amount of memory retained during normal, non-graph evaluation.
Unfortunately, I can't reproduce the increase in gc.heapSize due to rounding (NixOS/nix#13336), so I'm unable to confirm as of now.
There was a problem hiding this comment.
file seems useful for troubleshooting and should also be in the module attrset.
This is not implemented yet.
| - `key`: `string` | |
| - `key`: `string` for the purpose of module deduplication and `disabledModules` | |
| - `file`: `string` for the purpose of error messages and warnings |
There was a problem hiding this comment.
We applied the docstring for key and added a file. Please review to make sure we did that right thing.
lib/tests/modules.sh
Outdated
There was a problem hiding this comment.
I'd prefer keeping the nix-instantiate as is and provide the additional arguments that you need via checkExpression
That change has a subtle altering affect to checkConfigOutput like adding --strict and removing -A "$attr.
That change to the testing framework is probably possible, but i wouldn't change it, if there is no motivation behind that.
There was a problem hiding this comment.
Fiy: Will need a bit more time to review the hard part of the changes 😄
There was a problem hiding this comment.
You're right about the --strict. We've now moved --strict out of local-nix-instantiate and into `checkExpression.
Not sure what you mean with the -A "$attr".
It seems that now behavior of existing tests is preserved.
lib/modules.nix
Outdated
There was a problem hiding this comment.
Here's the doCollect. Is the comment correct?
dfa25a7 to
a9ea851
Compare
|
I used this recently and it was helpful to me 🪄 |
It would be great to see a practical use case for this, to illustrate a bit more what this does. |
|
It gave me a list of all files in which I was defining |
Co-authored-by: Shahar "Dawn" Or <mightyiampresence@gmail.com>
Co-authored-by: Ali Jamadi <jamadi1377@gmail.com>
a9ea851 to
5186921
Compare
|
Spent some time rebasing this. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/nixpkgs-module-system-config-modules-graph/67722/1 |
For various module evaluation design/refactor analysis purposes one may want to obtain a data structure that represents all the modules that took part of a configuration evaluation.
This PR adds an attribute to the return value of
lib.evalModuleswith such a data structure.Initially discussed here.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Co-authored-by: @A-jay98
Add a 👍 reaction to pull requests you find important.