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

Previous Next

Full log

View this message in rfc822 format

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

Hi Fredrik,

Fredrik Salomonsson <plattfot <at> posteo.net> skribis:

> * gnu/packages/gnupg.scm (pinentry-rofi): New variable.

Overall LGTM, with some minor issues highlighted below:

> +(define-public pinentry-rofi
> +  (package
> +  (name "pinentry-rofi")

Indentation is off here (should be offset by two).

> +  (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?

> +      #:phases
> +      (modify-phases
> +        %standard-phases
> +        (add-after
> +          'install
> +          'hall-wrap-binaries

Nitpick: please indent as in other files.

> +          (lambda* (#:key inputs outputs #:allow-other-keys)
> +            (let* ((compiled-dir
> +                     (lambda (out version)
> +                       (string-append out "/lib/guile/" version "/site-ccache")))
> +                   (uncompiled-dir
> +                     (lambda (out version)
> +                       (string-append
> +                         out
> +                         "/share/guile/site"
> +                         (if (string-null? version) "" "/")
> +                         version)))
> +                   (dep-path
> +                     (lambda (env modules path)
> +                       (list env
> +                             ":"
> +                             'prefix
> +                             (cons modules
> +                                   (map (lambda (input)
> +                                          (string-append
> +                                            (assoc-ref inputs input)
> +                                            path))
> +                                        ,''("rofi"))))))
> +                   (out (assoc-ref outputs "out"))
> +                   (bin (string-append out "/bin/"))
> +                   (site (uncompiled-dir out "")))
> +              (match (scandir site)
> +                     (("." ".." version)
> +                      (for-each
> +                        (lambda (file)
> +                          (wrap-program
> +                            (string-append bin file)
> +                            (dep-path
> +                              "GUILE_LOAD_PATH"
> +                              (uncompiled-dir out version)
> +                              (uncompiled-dir "" version))
> +                            (dep-path
> +                              "GUILE_LOAD_COMPILED_PATH"
> +                              (compiled-dir out version)
> +                              (compiled-dir "" version))))
> +                        ,''("pinentry-rofi"))
> +                      #t))))))))

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)))

?

> +  (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?

> +  (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”.

Could you send an updated patch?

Thanks in advance and sorry for the delay!

Ludo’.

Previous Next

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