msmtp: compiles with keyring support by default #30584
Conversation
|
How much does this increase the closure size? I wonder because |
|
I also used passwordeval but it is a more brittle setup IMO, you need an extra setup. For instance in my case I override msmtp to have access to the python keyring utilities (it's not counted in the msmtp closure size but maybe it should). |
|
As long as it doesn't pull in all of gnome or something crazy like that it's not so bad ... I'm happy to defer to the maintainer in any case :) |
| @@ -1,5 +1,6 @@ | |||
| { stdenv, lib, fetchurl, autoreconfHook, pkgconfig | |||
| , openssl, netcat-gnu, gnutls, gsasl, libidn, Security | |||
| , libsecret, withKeyring ? true | |||
There was a problem hiding this comment.
I believe the canonical way to do this is:
, withKeyring ? true, libsecret ? null
There was a problem hiding this comment.
why do it only for libsecret ? I don't see libidn ? null, gnutls ? null
There was a problem hiding this comment.
The difference I see is that those are already required dependencies, but as you are proposing libsecret as an optional dependency.
My way of doing it would be without the withKeyring parameter and use the isNull builtin for the configure flags (nothing would be needed for buildInputs IIRC).
There was a problem hiding this comment.
I see the point of not having to mirror every configure flag into the nix file.
I removed the configureFlag ("--with-libsecret" is enabled by default) but if I remove libsecret from buildinputs, it won't be found by the configure (confirmed by trial).
There was a problem hiding this comment.
The withFoo pattern as opposed to passing libsecret = null; and then use isNull is to check if something should be enabled or disabled is that often multiple derivations are added to buildInputs as a consequence of toggling a single flag. So using withFoo makes it easier to read and easier to adjust. When it's only a single derivation, it really doesn't make much of a difference, but for the sake of uniformity I personally prefer it that way.
There was a problem hiding this comment.
not sure I got your last sentence: so you would prefer with withKeyring ? and the libsecret ? null is just to hint at thepackage being optional ?
which maintainer should I listen to :D ?
I prefer the withKeyring approach (explicit over implicit)
There was a problem hiding this comment.
you would prefer with withKeyring ? and the libsecret ?
, withKeyring ? true, libsecret ? null
and then:
buildInputs = [ openssl gnutls gsasl libidn ]
++ stdenv.lib.optional withKeyring libsecret
++ stdenv.lib.optional stdenv.isDarwin Security;
which maintainer should I listen to :D ?
Me of course! Why would you even ask that?!?! 😃
|
I'm cool with this although I'm also using passwordeval to retrieve passwords from kwallet, so I will not be able to test using libsecret. |
|
I haven't squashed the commits because I believe you can do it from github UI. Otherwise, I will do it. |
|
Thanks @teto ! |
As it is recommended by msmtp http://msmtp.sourceforge.net/doc/msmtp.html#Authentication.
* msmtp: compiles with keyring support by default As it is recommended by msmtp http://msmtp.sourceforge.net/doc/msmtp.html#Authentication (cherry picked from commit cd4df56)
As it is recommended by msmtp http://msmtp.sourceforge.net/doc/msmtp.html#Authentication.
Motivation for this change
That's the "cleaner" way to retrieve password and recommended way as well.
Not sure it will pass on mac.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)