nixos/pam: fix infinite recursion

[gkleen]

#429203 introduced an infinite recursion on my machine.

Use of optionalAttrs instead of mkIf seemed dubious anyways.

Things done

• Built on platform:
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• Tested, as applicable:
  • NixOS tests in nixos/tests.
  • Package tests at passthru.tests.
  • Tests in lib/tests or pkgs/test for functions and "core" functionality.
• Ran nixpkgs-review on this PR. See nixpkgs-review usage.
• Tested basic functionality of all binary files, usually in ./result/bin/.
• Nixpkgs Release Notes
  • Package update: when the change is major or breaking.
• NixOS Release Notes
  • Module addition: when adding a new NixOS module.
  • Module update: when the change is significant.
• Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

---

Add a 👍 reaction to pull requests you find important.

[LordGrimmauld]

Do you have a log of that infinite recursion? I am not really opposed to the changes here, but i am somewhat surprised that supposed infinite recursion did not surface earlier (e.g. in the VM test)

[gkleen]

Do you have a log of that infinite recursion? I am not really opposed to the changes here, but i am somewhat surprised that supposed infinite recursion did not surface earlier (e.g. in the VM test)

I've attached a stack trace which I found entirely unhelpful – I just bisected and made an educated guess 🤷

trace.txt

[LordGrimmauld]

Right, this is a weird one!
I guess the test doesn't break because it explicitly sets security.pam.services.systemd-run0 = { ... };, so the evaluator doesn't need to check the systemd package version. And other people are either not running lastlog, or also setting the run0 pam service unconditionally and explicitly. That trace does make some sense. I am still surprised you are the first one to report this, but oh well!

[LordGrimmauld]

thank you