pinging the OWNERS of ci/, since this will be of interest in getting maintainer pings for nixos PRs.

@infinisil @philiptaron @wolfgangwalther

This is great, thanks for working on this!

If we want to run this in ci, it should be somewhere in ci/. The fact that you had to ping us manually is one reason ;). The other one is that by putting this in there, we can potentially do sparse checkouts for ci/ (which we currently do for the reviewers.yml workflow).

Since this will run based on the changed files, it should be part of ci/eval/compare - where we also determine the package maintainers. The list of changed files is already available in there.

To be able to integrate in the current workflow nicely, the scripting part should run in the nix sandbox. If we need it at all, can this be pure nix?

Edit: I think the script is mostly a wrapper anyway, we should be able to just use the nix file and put that into ci/eval/compare?

Re failure modes: Can we do a check for the file's name in module-list.nix? If not in there, don't even try?

I improved the logic to support non-lambda modules and to handle missing inputs, now the only eval failures are the two invalid meta.maintainers.

Result after running it on all files in module-list.nix:

https://gist.github.com/pbsds/72c02e03b1632baf2d7aa48a63a42267

Given the advent of meta.teams, I wonder if it would be in scope to add support for modules as well.

If not I can certainly work around this for now, I already love that this is happening at all!

Result after running it on all files in module-list.nix:

I think this broke support for nixos tests.

❯ ./ci/eval/compare/get-nixos-module-maintainers/main.sh nixos/tests/firefox.nix
fetching path input 'path:/nix/store/np9j7pd0ckddf59c4jdfj52xq1h0mm2l-source'
ERROR: 'nixos/tests/firefox.nix' is not relative to '/home/hexa/git/nixos/master'

Supporting meta.teams proved quite simple, but i don't do the same merging logic into meta.maintainers that derivations do. I extended the example jq invocation to compensate.

I think this broke support for nixos tests.

That is due to

Re failure modes: Can we do a check for the file's name in module-list.nix? If not in there, don't even try?

I added a comment about how --force bypasses the check.

Aren't tests in the same bucket as nixos modules? They have all-tests.nix and are essentially modules.

I'm in favor of just removing the module-list.nix check and let the tool fail, is this okay @wolfgangwalther?

The PR originally targeted maintainers/. But the scope has now crept from moving it to ci/, to now fully integrating it with the GHA flow? I leave that for a followup PR.

I'm not sure what the intent of a standalone script to get the maintainers is. We want to integrate this into CI, right?

If so, this should be part of ci/eval/compare/maintainers.nix, not something separate. [...]

[...] The intent to add a module-list.nix check was to prevent the CI job from failing - what's the alternative?

The shebang-script form and it being used in CI is not mutually exclusive. In its current form you can both use it locally from the shell and in CI via import. If you only map it over import ./ci/eval/compare/get-nixos-module-maintainers.nix you don't even need the check:

nix eval --file default.nix  --apply 'pkgs_: let pkgs = pkgs_ {}; moduleList = import ./nixos/modules/module-list.nix; evalModule = import ./ci/eval/compare/get-nixos-module-maintainers.nix; inherit (pkgs) lib; in lib.map (moduleFname: { inherit moduleFname; meta = if lib.isPath moduleFname then evalModule { inherit pkgs lib moduleFname; } else null;}) moduleList' --json  | jq .

output: https://gist.github.com/pbsds/ffc6a2b70d7c79fda9ef0c786f7c6ebe

(i had to apply #413092 in this branch for it to eval)

The PR originally targeted maintainers/. But the scope has now crept from moving it to ci/, to now fully integrating it with the GHA flow?

Hm, then I misunderstood your initial comment pinging CI owners. I had not only understood this to be well in scope, but to be the (only) purpose of this PR.

Sorry for the bad advice then, but a standalone script, supposed to be called manually, makes no sense in ci/eval/compare.

I leave that for a followup PR.

What if that follow up PR merged the existing maintainers file and the new module-maintainers file, essentially removing the "call as standalone" part? Would that be a negative for you, because you consider calling this script manually a "feature"? Would you want the follow up work keep that?

Whatever i contribute to nixpkgs is MIT and free to be changed.

EDIT: i have also not claimed ownership of the file in OWNERS

Whatever i contribute to nixpkgs is MIT and free to be changed.

EDIT: i have also not claimed ownership of the file in OWNERS

Well, that was not my question. My question was more along the lines of whether the ability to call this standalone was a deliberate decision, because you wanted to have that property - or because that was just the easiest way to iterate on it and is not a feature you ultimately depend on.

I'm left confused about the intent of this PR.

Making a script was the initial goal. It becoming both upstreamable and also a proper nix function and not a shell script is a happy accident. Inlining this file in its current form into ci/eval/compare/maintainers.nix serves IMO no other purpose than to make the file tree in ci/ look slightly slimmer at the expense of ergonomics. I don't mind it if you opt to do so, i am perfectly able to maintain a copy outside of nixpkgs for personal use, as i already do with a bunch of other scripts.

I don't understand however why you consider scripts in ci/ illegal. If you consider this blocking i am perfectly happy to move it back to maintainers/scripts/

I don't understand however why you consider scripts in ci/ illegal.

Well ci/ should contain things used by CI. I don't see a script in this form being used by CI, neither in here (obviously), nor in the future - because the best way to do this for CI, would be to integrate it into maintainers.nix.

(Another reason is error-handling - throwing an error on some files is not going to work for CI)

Making a script was the initial goal. It becoming both upstreamable and also a proper nix function and not a shell script is a happy accident.

This very much sounds like maintainers/scripts/ is the right place. Sorry again for the bad advice of moving it, then. I'd just pick the relevant code from it, in case I'll want to work on this.

Inlining this file in its current form into ci/eval/compare/maintainers.nix serves IMO no other purpose than to make the file tree in ci/ look slightly slimmer at the expense of ergonomics.

Eh, no. Integrating this file into maintainers.nix would result in CI actually pinging module maintainers and requesting their review.

Eh, no. Integrating this file into maintainers.nix would result in CI actually pinging module maintainers and requesting their review.

I assume your brain did a s/Inlining/Integrating/. I demonstrated that a import from nix works.

I fear we both have been a bit too stubborn to be able to progress this PR, and I want to fix that. What should I do?

• Should I answer more questions?
• Should I try and make it never throw?
• Should I amend the nixos module documentation to require that the meta attributes must be "trivially" evaluatable?
• Should we drop this lazy-eval path -> maintainers based approach in favor of something that uses lib.evalModules to produce an attribute set mapping nixos module paths to maintainers? (may require lib.evalModules: add graph attribute #403839, haven't looked into this yet)
• Should I move it to maintainers/scripts/?

The reason I have not been too eager to inline this into ci/eval/compare/maintainers.nix is that I try not to get sucked into any more rabbitholes while finishing up my PhD over the next few months (it may not be too long before I force myself to log out from GitHub if my current rate of progress continues. This is why I have not consideered integrating this into CI myself.)

Just to make sure we're on the same page regarding the current state of the PR:

• It adds a new script/nix lambda to ci/
  • Works from the shell
  • Works from nix
• It evaluates successfully for all nixos modules listed in module-list.nix
• It does not try to block any attempt of evaluating a file not listed in module-list.nix
  • This doesn't hinder the caller from filtering the files first.

I think the path forward depends on the goal. And I can't really tell what your goal is, so far. I still don't understand whether the ability to call this script as a standalone script is a goal in itself. That's because I don't see the value in that, I wouldn't know how to use it (which is the same for me for a few other maintainer scripts, I think). The only value I see is in doing this in CI. But that might be different for you.

I entirely understand when you're saying you don't have the capacity to be pulled into another rabbit hole - but that's a different problem to solve. We shouldn't merge a half-baked solution, because of that. Instead we can meet half-way, I can do the CI part for example.

For me, it's simple:

• If the goal is to have this call-able as a standalone script for maintainers, then it should be in maintainers/scripts. That's where maintainers look for stuff to run, it would be entirely hidden to them in ci/eval/compare. We can then look how we're dealing with CI later.
• But if the goal is to have this only in CI, then we don't need that. But, then we can also immediately change a few things about the script - for example the error handling / checking for whether the file is actually a module. Or the way we call it (doesn't need to be a standalone script anymore) etc.

If we take the 2nd path, work will ultimately become more and more about the CI-rabbithole. Feel free to stop at any point (even just now) - I can continue with the CI stuff, no problem.

(One goal is clear to me: Pinging module maintainers should not negatively affect your PhD ;) )

Having this be call-able as a standalone script for maintainers is a nice-to-have but ultimately not my primary goal nor a requirement. I want this in CI.

I opted however to take the path of making a PR which gets us half-way there and leave for it to someone else to continue integrating it fully with the CI, maybe over new-years once I've switched from academia to industry will I involve myself more with CI in general.

Please feel free to take over the PR as you see fit, be it merging it and continue working on it or cherry-picking the commit and reworking the approach as you see fit before up-streaming it. :)

🙀

Yeah, once you take the very much simplified approach in that screenshot further and consider all edge-cases... you will essentially end up with this PR, right? Or did I misunderstand?

@pbsds can we get this going?

This is waiting on me getting around to doing the CI part for it.

I'm not sure I see the value in this approach.

The meta.maintainers option already evaluates to an attrset mapping "filenames" to "maintainer entries".

If we want to know who are the maintainers of a specific module, we can:

1. Evaluate a NixOS configuration that imports the module*
2. Read config.meta.maintainers."filename"

*It doesn't need to be a fully blown NixOS configuration with all modules, but that's probably the simplest approach. The minimal eval would just import { _module.check = false; }, the module that declares the meta.maintainers option, and the module we want to know the maintainers of. With this minimal approach, we will often need to stub some module args (e.g. pkgs), so using a "real" (but empty) NixOS configuration is simpler.

The meta.maintainers option already evaluates to an attrset mapping "filenames" to "maintainer entries".

Was this possibly introduced after the creation of this PR? Just asking - I have literally no idea.