diff mbox series

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

Message ID 20200504131624.04c6d30e.raghavgururajan@disroot.org
State Accepted
Headers show
Series [bug#41083] gnu: xfe: Fix hard-coded fhs directories. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job

Commit Message

Raghav Gururajan May 4, 2020, 5:16 p.m. UTC

Comments

Ludovic Courtès May 4, 2020, 9:04 p.m. UTC | #1 
Hello!

Raghav Gururajan <raghavgururajan@disroot.org> skribis:

>>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.
>
> * gnu/packages/disk.scm (xfe): Fix hard-coded fhs directories.
> [arguments]<#:phases>['patch-xfe-paths]: Delete phase.
> [arguments]<#:phases>['patch-bin-dirs]: New phase.
> [arguments]<#:phases>['patch-share-dirs]: New phase.
> [inputs]<bash,coreutils,file,findutils>: New inputs.

Nitpick: You don’t need to mention #:phases above, and the angle
brackets are inappropriate for inputs.  See ‘git log’ for examples.

> -    (native-inputs
> -     `(("intltool" ,intltool)
> -       ("pkg-config" ,pkg-config)))
> -    (inputs
> -     `(("fox" ,fox)
> -       ("freetype" ,freetype)
> -       ("x11" ,libx11)
> -       ("xcb" ,libxcb)
> -       ("xcb-util" ,xcb-util)
> -       ("xft" ,libxft)
> -       ("xrandr" ,libxrandr)))

To reduce review time :-), it’s a good idea to avoid unnecessary
changes.  In this case, you should avoid moving things around because
that makes the patch harder to read.


> +               (substitute* "src/FilePanel.cpp"
> +                 (("/bin/sh") sh)
> +                 (("/usr/bin/du") du)
> +                 (("/usr/bin/sort") sort)
> +                 (("/usr/bin/cut") cut)
> +                 (("/usr/bin/xargs") xargs))
> +               (substitute* "src/help.h"
> +                 (("/bin/sh") sh)
> +                 (("/bin/ls") ls))
> +               (substitute* "src/SearchPanel.cpp"
> +                 (("/usr/bin/du") du)
> +                 (("/usr/bin/sort") sort)
> +                 (("/usr/bin/cut") cut)
> +                 (("/usr/bin/xargs") xargs))
> +               (substitute* "src/startupnotification.cpp"
> +                 (("/bin/sh") sh))
> +               (substitute* "src/xfeutils.cpp"
> +                 (("/usr/bin/file") file))

I think you can just define a variable like:

  (coreutils (assoc-ref inputs "coreutils"))

and then use (string-append coreutils "/bin/sort") and so on, instead of
defining many variables that have a single user.

>               (let*
> -                 ((out     (assoc-ref outputs "out"))
> -                  (share   (string-append out "/share"))
> -                  (xferc   (string-append out "/share/xfe/xferc"))
> -                  (xfe-theme   (string-append out "/share/xfe/icons/xfe-theme")))
> -               ;; Correct path for xfe registry.
> +                 ((out
> +                   (assoc-ref outputs "out"))
> +                  (share
> +                   (string-append out "/share"))
> +                  (xfe
> +                   (string-append out "/share/xfe"))

Avoid the indentation changes (previous version was fine, although we
usually start the list of bindings on the same line as ‘let*’).

> -               ;; Correct path for xfe icons.
> -               (substitute* "src/xfedefs.h"
> -                 (((string-append
> -                    "~/.config/xfe/icons/xfe-theme:"
> -                    "/usr/local/share/xfe/icons/xfe-theme:"
> -                    "/usr/share/xfe/icons/xfe-theme"))
> -                  xfe-theme))

The ~/.config/xfe bit is going away, right?

Could you send an updated patch?

Thanks,
Ludo’.
Nicolas Goaziou May 4, 2020, 9:18 p.m. UTC | #2 
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,
diff mbox series

Patch

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.

* gnu/packages/disk.scm (xfe): Fix hard-coded fhs directories.
[arguments]<#:phases>['patch-xfe-paths]: Delete phase.
[arguments]<#:phases>['patch-bin-dirs]: New phase.
[arguments]<#:phases>['patch-share-dirs]: New phase.
[inputs]<bash,coreutils,file,findutils>: New inputs.
---
 gnu/packages/disk.scm | 109 ++++++++++++++++++++++++++++++++----------
 1 file changed, 83 insertions(+), 26 deletions(-)

diff --git a/gnu/packages/disk.scm b/gnu/packages/disk.scm
index 484126a3ea..69f0a131bc 100644
--- a/gnu/packages/disk.scm
+++ b/gnu/packages/disk.scm
@@ -48,6 +48,7 @@ 
   #:use-module (gnu packages documentation)
   #:use-module (gnu packages elf)
   #:use-module (gnu packages file-systems)
+  #:use-module (gnu packages file)
   #:use-module (gnu packages fontutils)
   #:use-module (gnu packages gettext)
   #:use-module (gnu packages glib)
@@ -940,43 +941,99 @@  since they are better handled by external tools.")
        (sha256
         (base32 "1fl51k5jm2vrfc2g66agbikzirmp0yb0lqhmsssixfb4mky3hpzs"))))
     (build-system gnu-build-system)
-    (native-inputs
-     `(("intltool" ,intltool)
-       ("pkg-config" ,pkg-config)))
-    (inputs
-     `(("fox" ,fox)
-       ("freetype" ,freetype)
-       ("x11" ,libx11)
-       ("xcb" ,libxcb)
-       ("xcb-util" ,xcb-util)
-       ("xft" ,libxft)
-       ("xrandr" ,libxrandr)))
     (arguments
      `(#:phases
        (modify-phases %standard-phases
-         (add-after 'unpack 'patch-xfe-paths
+         (add-after 'unpack 'patch-bin-dirs
+           (lambda* (#:key inputs #:allow-other-keys)
+             (let*
+                 ((sh
+                   (string-append
+                    (assoc-ref inputs "bash") "/bin/sh"))
+                  (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"))
+                  (ls
+                   (string-append
+                    (assoc-ref inputs "coreutils") "/bin/ls"))
+                  (xargs
+                   (string-append
+                    (assoc-ref inputs "findutils") "/bin/xargs"))
+                  (file
+                   (string-append
+                    (assoc-ref inputs "file") "/bin/file")))
+               (substitute* "src/FilePanel.cpp"
+                 (("/bin/sh") sh)
+                 (("/usr/bin/du") du)
+                 (("/usr/bin/sort") sort)
+                 (("/usr/bin/cut") cut)
+                 (("/usr/bin/xargs") xargs))
+               (substitute* "src/help.h"
+                 (("/bin/sh") sh)
+                 (("/bin/ls") ls))
+               (substitute* "src/SearchPanel.cpp"
+                 (("/usr/bin/du") du)
+                 (("/usr/bin/sort") sort)
+                 (("/usr/bin/cut") cut)
+                 (("/usr/bin/xargs") xargs))
+               (substitute* "src/startupnotification.cpp"
+                 (("/bin/sh") sh))
+               (substitute* "src/xfeutils.cpp"
+                 (("/usr/bin/file") file))
+               #t)))
+         (add-after 'unpack 'patch-share-dirs
            (lambda* (#:key outputs #:allow-other-keys)
              (let*
-                 ((out     (assoc-ref outputs "out"))
-                  (share   (string-append out "/share"))
-                  (xferc   (string-append out "/share/xfe/xferc"))
-                  (xfe-theme   (string-append out "/share/xfe/icons/xfe-theme")))
-               ;; Correct path for xfe registry.
+                 ((out
+                   (assoc-ref outputs "out"))
+                  (share
+                   (string-append out "/share"))
+                  (xfe
+                   (string-append out "/share/xfe"))
+                  (xferc
+                   (string-append out "/share/xfe/xferc"))
+                  (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))
                (substitute* "src/XFileExplorer.cpp"
                  (("/usr/share/xfe/xferc") xferc)
                  (("/usr/local/share/xfe/xferc") xferc)
                  (("/opt/local/share/xfe/xferc") xferc))
-               ;; Correct path for xfe icons.
-               (substitute* "src/xfedefs.h"
-                 (((string-append
-                    "~/.config/xfe/icons/xfe-theme:"
-                    "/usr/local/share/xfe/icons/xfe-theme:"
-                    "/usr/share/xfe/icons/xfe-theme"))
-                  xfe-theme))
                #t))))))
+    (native-inputs
+     `(("intltool" ,intltool)
+       ("pkg-config" ,pkg-config)))
+    (inputs
+     `(("bash" ,bash)
+       ("coreutils" ,coreutils)
+       ("file" ,file)
+       ("findutils" ,findutils)
+       ("fox" ,fox)
+       ("freetype" ,freetype)
+       ("x11" ,libx11)
+       ("xcb" ,libxcb)
+       ("xcb-util" ,xcb-util)
+       ("xft" ,libxft)
+       ("xrandr" ,libxrandr)))
     (synopsis "File Manager for X-Based Graphical Systems")
     (description"XFE (X File Explorer) is a file manager for X.  It is based on
 the popular but discontinued, X Win Commander.  It aims to be the file manager
-- 
2.26.2