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
kwallet-pam: wrap pam_kwallet_init #70140
Conversation
8fc9d9b
to
47a1c9b
Compare
This is weird, it doesn't get automatically wrapped at |
Ahh, that's because it's a bash script
|
@@ -4,7 +4,8 @@ mkDerivation { | |||
name = "kwallet-pam"; | |||
nativeBuildInputs = [ extra-cmake-modules ]; | |||
buildInputs = [ pam socat libgcrypt qtbase kwallet ]; | |||
postPatch = '' | |||
sed -i pam_kwallet_init -e "s|socat|${lib.getBin socat}/bin/socat|" | |||
qtWrapperArgs = [ "--prefix PATH : ${lib.getBin socat}/bin" ]; |
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.
Can we add a dontWrapQtApps = true;
?
Just to make it clear pam_kwallet_init
is a script and we have to wrap manually.
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 with the above comment.
@@ -4,7 +4,8 @@ mkDerivation { | |||
name = "kwallet-pam"; | |||
nativeBuildInputs = [ extra-cmake-modules ]; | |||
buildInputs = [ pam socat libgcrypt qtbase kwallet ]; | |||
postPatch = '' | |||
sed -i pam_kwallet_init -e "s|socat|${lib.getBin socat}/bin/socat|" | |||
qtWrapperArgs = [ "--prefix PATH : ${lib.getBin socat}/bin" ]; |
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 would prefer to leave the patch in place for the absolute path of socat
. I don't like prefixing PATH
when we know exactly what program we are looking for; there's not telling in the future what other programs might end up at that PATH
entry.
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.
Seems reasonable, although the other side of this is we have a sed expression that may or may not always work.
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.
That's true. We should change that to substituteInPlace
so the build will fail if it stops working. (Not necessary for this PR.)
This needs a compatible env as kwalletd daemon. Need to wrap it to correct this. Fixes NixOS#68316
Updated |
47a1c9b
to
a296cc2
Compare
backported in d079834 |
This needs a compatible env as kwalletd daemon. Need to wrap it to
correct this.
Fixes #68316
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @