GNU bug report logs - #45190
[PATCH 0/1] Add pinentry-rofi

Previous Next

Full log

Message #17 received at 45190 <at> debbugs.gnu.org (full text, mbox):

From: Fredrik Salomonsson <plattfot <at> posteo.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 45190 <at> debbugs.gnu.org
Subject: Re: [bug#45190] [PATCH 1/1] gnu: Add pinentry-rofi.
Date: Fri, 08 Jan 2021 18:14:01 -0800

Hi Ludovic,

Ludovic Courtès <ludo <at> gnu.org> writes:

>> +(define-public pinentry-rofi
>> +  (package
>> +  (name "pinentry-rofi")
>
> Indentation is off here (should be offset by two).

Fixed in v2.

>> +  (arguments
>> +    `(#:strip-binaries? #f ;; Has no binaries and the strip phase is failing
>
> Hmm the ‘strip’ phase should not fail.  Are you sure this is necessary?

It was failing with a warning as there are no binaries to strip. But
it's not an error so I removed this in v2.

>> +      #:phases
>> +      (modify-phases
>> +        %standard-phases
>> +        (add-after
>> +          'install
>> +          'hall-wrap-binaries
>
> Nitpick: please indent as in other files.

Fixed in v2.

> Since I think you’re also upstream :-), how about adding something like
> that at the top of the installed executable:
>
>   (eval-when (load expand eval)
>     (set! %load-path (cons "@moddir@" %load-path))
>     (set! %laod-compiled-path (cons "@godir@" %load-compiled-path)))
>
> ?

Yup, I'm upstream as well. I don't mind adding that, just need to know
what it solves :). I'm guessing that it removes the need to wrap the
executable, is that correct?

And are the "@moddir@" and "@godir@" expected to be expanded by
automake? I tested to just add it in the source code and automake did
nothing with them.

For now I left it out in v2 as I'm a bit unsure of it.

>> +  (propagated-inputs `(("rofi" ,rofi)))

> It’s best to avoid propagating.  Perhaps you can replace the “rofi”
> string in ‘pinentry-rofi’ by “/gnu/store/…/bin/rofi” in a post-install
> phase?

Is there a rule of thumb or something to know when to use propagating
inputs? I'm a bit confused when to use is it. Is it just when dealing
with libraries? What are the downsides of using propagating inputs?
Apologize if this is already mentioned in the manual. Only sections I
could find that mentions propagated inputs are section 5.2 and 8.2.1.

Anyway this is fixed in v2, I added the rofi's binary path to PATH for
the wrapper of pinentry-rofi. 

>> +  (synopsis "Rofi GUI for GnuPG's passphrase input")
>> +  (description "Simple pinentry GUI using rofi that allows users to enter a
>> +passphrase when required by @code{gpg} or other software.")
>
> Please make it a full sentence.  Also, to give context, perhaps replace
> “rofi” by “the Rofi application launcher”.

I fleshed out the description in v2, let me know if it sounds better.

> Could you send an updated patch?

All updates should be in PATCH v2

> Thanks in advance and sorry for the delay!

Thank you for reviewing my patch and no worries about the delay.

-- 
s/Fred[re]+i[ck]+/Fredrik/g

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.