[bug#45190] [PATCH 1/1] gnu: Add pinentry-rofi.

["Ludovic Courtès" <ludo@gnu.org>]

Hi Fredrik,

Fredrik Salomonsson <plattfot@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’.