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
doas: add enablePAM option #94040
doas: add enablePAM option #94040
Conversation
pkgs/tools/security/doas/default.nix
Outdated
, enablePAM ? true | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping with the established style of the file:
, enablePAM ? true | |
, withPAM ? true |
pkgs/tools/security/doas/default.nix
Outdated
@@ -24,7 +25,7 @@ stdenv.mkDerivation rec { | |||
configureFlags = [ | |||
(lib.optionalString withTimestamp "--with-timestamp") # to allow the "persist" setting | |||
"--pamdir=${placeholder "out"}/etc/pam.d" | |||
]; | |||
] ++ lib.optional (!enablePAM) "--without-pam"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this went inside the configureFlags
list, just as withTimestamp
above.
New option "withPAM" controls whether to build support for pluggable authetincation modules. Default value is "true", which correspond to existing behaviour. Futhermore, with default configuration, this change do not cause rebuild.
@cole-h Applied your suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks. One minor question below, but approval stands either way.
[2 built, 1 copied (0.1 MiB), 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/94040
1 package built:
doas
@@ -23,6 +24,7 @@ stdenv.mkDerivation rec { | |||
|
|||
configureFlags = [ | |||
(lib.optionalString withTimestamp "--with-timestamp") # to allow the "persist" setting | |||
(lib.optionalString (!withPAM) "--without-pam") | |||
"--pamdir=${placeholder "out"}/etc/pam.d" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also gate this with lib.optionalString withPAM "..."
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, upstream build system defaults to "--with-pam", so it is not strictly necessary.
Ofborg seems unhappy, but I don't understand why: |
@ofborg eval Master was broken a few times yesterday, causing this issue. |
@cole-h Looks like everything is ready now. |
New option "enablePAM" controls whether to build support for pluggable
authetincation modules. Default value is "true", which correspond to
existing behaviour. Futhermore, with default configuration, this change
do not cause rebuild.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)