[bug#41083] gnu: xfe: Fix hard-coded fhs directories.

[Nicolas Goaziou <mail@nicolasgoaziou.fr>]

Hello,

Raghav Gururajan <raghavgururajan@disroot.org> writes:

> From 660f134e15438e7ee7aec1c076dca93c68e4edc6 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan@disroot.org>
> Date: Mon, 4 May 2020 13:07:02 -0400
> Subject: [PATCH] gnu: xfe: Fix hard-coded fhs directories.

Thank you. Some cosmetics comments follow.

> +         (add-after 'unpack 'patch-bin-dirs
> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let*
> +                 ((sh
> +                   (string-append
> +                    (assoc-ref inputs "bash") "/bin/sh"))

This indentation is unusual. I think it would be clearer to write

  (let* ((sh (string-append (assoc-ref inputs "bash")
                            "/bin/sh"))))

I suggest the following simplification, however:

  (let* ((bash (assoc-ref inputs "bash"))
         (coreutils (assoc-ref inputs "coreutils"))
         (findutils (assoc-ref inputs "findutils"))
         (file (assoc-ref inputs "file")))
   ...)

See below for the consequences of this modification.

> +                  (du
> +                   (string-append
> +                    (assoc-ref inputs "coreutils") "/bin/du"))
> +                  (sort
> +                   (string-append
> +                    (assoc-ref inputs "coreutils") "/bin/sort"))
> +                  (cut
> +                      (string-append
> +                       (assoc-ref inputs "coreutils") "/bin/cut"))

Indentation is off here.

> +               (substitute* "src/FilePanel.cpp"

`substitute*' accepts a list of files as its first argument. Please
consider using the following, assuming you applied the simplification
above.

  (with-directory-excursion "src"
   (substitute* '("FilePanel.cpp" "help.h" "SearchPanel.cpp" ...)
    (("/bin/sh" file) (string-append bash file))
    (("/usr(/bin/du)" _ file) (string-append coreutils file))
    ...))

> +                 ((out
> +                   (assoc-ref outputs "out"))

These should be on the same line.

> +                  (share
> +                   (string-append out "/share"))
> +                  (xfe
> +                   (string-append out "/share/xfe"))

Ditto. You can also re-use share.

> +                  (xferc
> +                   (string-append out "/share/xfe/xferc"))

Ditto. You can re-use xfe.

> +                  (icons
> +                   (string-append out "/share/xfe/icons"))
> +                  (xfe-theme
> +                   (string-append out "/share/xfe/icons/xfe-theme")))
>                 (substitute* "src/foxhacks.cpp"
> -                 (("/etc:/usr/share:/usr/local/share") share))
> -               ;; Correct path for xfe configuration.
> +                 (("/usr/share") share)
> +                 (("/usr/local/share") share))
> +               (substitute* "src/help.h"
> +                 (("/usr/share/xfe") xfe)
> +                 (("/usr/local/share/xfe") xfe)
> +                 (("/opt/local/share/xfe") xfe)
> +                 (("/usr/share/xfe/icons/xfe-theme") xfe-theme)
> +                 (("/usr/local/share/xfe/icons/xfe-theme") xfe-theme))
> +               (substitute* "src/xfedefs.h"
> +                 (("/usr/share/xfe/icons") icons)
> +                 (("/usr/local/share/xfe/icons") icons))

Wouldn't it be simpler to replace "/(usr|opt)(/local)?" with `out' in
all files?

>      (description"XFE (X File Explorer) is a file manager for X.  It is based on
                ^^^^^^
           missing space here

Could you send an updated patch?

Regards,

-- 
Nicolas Goaziou