GNU bug report logs - #41083
gnu: xfe: Fix hard-coded fhs directories.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 41083 in the body.
You can then email your comments to 41083 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

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

From: Raghav Gururajan <raghavgururajan <at> disroot.org>
To: guix-patches <at> gnu.org
Subject: gnu: xfe: Fix hard-coded fhs directories.
Date: Mon, 4 May 2020 13:16:24 -0400

[Message part 1 (text/plain, inline)]

[0001-gnu-xfe-Fix-hard-coded-fhs-directories.patch (text/x-patch, attachment)]

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Raghav Gururajan <raghavgururajan <at> disroot.org>
Cc: 41083 <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Mon, 04 May 2020 23:04:48 +0200

Hello!

Raghav Gururajan <raghavgururajan <at> disroot.org> skribis:

>>From 660f134e15438e7ee7aec1c076dca93c68e4edc6 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan <at> 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’.

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

From: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
To: Raghav Gururajan <raghavgururajan <at> disroot.org>
Cc: 41083 <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Mon, 04 May 2020 23:18:45 +0200

Hello,

Raghav Gururajan <raghavgururajan <at> disroot.org> writes:

> From 660f134e15438e7ee7aec1c076dca93c68e4edc6 Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan <at> 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

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

From: Raghav Gururajan <raghavgururajan <at> disroot.org>
To: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
Cc: 41083 <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Tue, 5 May 2020 11:36:51 -0400

[Message part 1 (text/plain, inline)]

Hello Nicolas!

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

Thanks! I used this.

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

Thanks! I used this.

> > +                 ((out
> > +                   (assoc-ref outputs "out"))  
> 
> These should be on the same line.

My new patch somehow has correct indentation.

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

Thanks! I used this. But the above would conflict with "/usr/bin". So I used
"/(usr|opt)(/local)?/share".

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

Fixed!

> Could you send an updated patch?

Please find updated patch attached with this email.

Regards,
RG.

[0001-gnu-xfe-Fix-hard-coded-fhs-directories.patch (text/x-patch, attachment)]

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

From: Raghav Gururajan <raghavgururajan <at> disroot.org>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 41083 <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Tue, 5 May 2020 11:44:44 -0400

[Message part 1 (text/plain, inline)]

Hello Ludo!

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

Fixed!
 
> 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.

Sorry about that. I was making the definition consistent with the order
mentioned in
https://guix.gnu.org/manual/en/html_node/Defining-Packages.html#Defining-Packages

> 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.
>
> Avoid the indentation changes (previous version was fine, although we
> usually start the list of bindings on the same line as ‘let*’).

I have changed some things in my new patch.

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

No. It is not necessary to remove it, as it is only an alternative dir. Xfe now
looks in 'out', if ~/.config/xfe doesn't exist. :-) 

> Could you send an updated patch?

Please find the updated patch attached with this email.

Regards,
RG.

[0001-gnu-xfe-Fix-hard-coded-fhs-directories.patch (text/x-patch, attachment)]

Message #22 received at 41083-done <at> debbugs.gnu.org (full text, mbox):

From: Nicolas Goaziou <mail <at> nicolasgoaziou.fr>
To: Raghav Gururajan <raghavgururajan <at> disroot.org>
Cc: Ludovic Courtès <ludo <at> gnu.org>, 41083-done <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Wed, 06 May 2020 17:39:16 +0200

Hello,

Raghav Gururajan <raghavgururajan <at> disroot.org> writes:

> Please find the updated patch attached with this email.

Thank you. I applied it, with the following few cosmetics changes.

> Subject: [PATCH] gnu: xfe: Fix hard-coded fhs directories.
>
> * gnu/packages/disk.scm (xfe):
> [arguments]: Remove phase 'patch-xfe-paths.
> [arguments]: Add phases 'patch-bin-dirs and 'patch-share-dirs.

I merged these two lines.

> +           (lambda* (#:key inputs #:allow-other-keys)
> +             (let*
> +                 ((bash (assoc-ref inputs "bash"))

I moved the bash binding onto the same line as let*.

> +           (lambda* (#:key outputs #:allow-other-keys)
> +             (let*
> +                 ((out (assoc-ref outputs "out"))

Ditto.

Regards,

-- 
Nicolas Goaziou

Message #25 received at 41083-done <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Raghav Gururajan <raghavgururajan <at> disroot.org>
Cc: 41083-done <at> debbugs.gnu.org
Subject: Re: [bug#41083] gnu: xfe: Fix hard-coded fhs directories.
Date: Wed, 06 May 2020 17:52:30 +0200

Hi,

Raghav Gururajan <raghavgururajan <at> disroot.org> skribis:

>>From e7032ff0032bbdbaefa8505f9f882fc6df58adaf Mon Sep 17 00:00:00 2001
> From: Raghav Gururajan <raghavgururajan <at> disroot.org>
> Date: Tue, 5 May 2020 11:22:01 -0400
> Subject: [PATCH] gnu: xfe: Fix hard-coded fhs directories.
>
> * gnu/packages/disk.scm (xfe):
> [arguments]: Remove phase 'patch-xfe-paths.
> [arguments]: Add phases 'patch-bin-dirs and 'patch-share-dirs.
> [inputs]: Add bash, coreutils, file and findutils.

Great.  Applied, thanks!

(And thanks to Nicolas as well.)

Ludo’.

Previous Next

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