[PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Alex Kost]

[-- Attachment #1: Type: text/plain, Size: 206 bytes --]

I've noticed that the packages for zathura plugins install .desktop
files into:

/gnu/store/…/usr/share/applications/ instead of:
/gnu/store/…/share/applications/

The attached patch fixes it.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gnu-pdf-Fix-installing-desktop-files-of-zathura-pack.patch --]
[-- Type: text/x-patch, Size: 2100 bytes --]

From 74efced6076ef28b321cb2a6a69344f55b714b59 Mon Sep 17 00:00:00 2001
From: Alex Kost <alezost@gmail.com>
Date: Tue, 7 Jul 2015 11:17:06 +0300
Subject: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

* gnu/packages/pdf.scm (zathura-cb, zathura-ps, zathura-djvu,
  zathura-pdf-poppler): Add PREFIX to 'make-flags' to install
  ".desktop" files into "share", not "usr/share".
---
 gnu/packages/pdf.scm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
index 82e8c88..0e8817d 100644
--- a/gnu/packages/pdf.scm
+++ b/gnu/packages/pdf.scm
@@ -173,7 +173,7 @@
     (arguments
      `(#:make-flags
        `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -204,7 +204,7 @@ using libarchive.")
     (arguments
      `(#:make-flags
        `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -236,7 +236,7 @@ using libspectre.")
     (arguments
      `(#:make-flags
        `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -269,7 +269,7 @@ using the DjVuLibre library.")
     (arguments
      `(#:make-flags
        `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
        #:tests? #f ; Package does not include tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
-- 
2.4.3

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Paul van der Walt]

Hey Alex,

On 2015-07-07 at 10:33, quoth Alex Kost:
> I've noticed that the packages for zathura plugins install .desktop
> files into:
>
> /gnu/store/…/usr/share/applications/ instead of:
> /gnu/store/…/share/applications/
>
> The attached patch fixes it.

My bad!  Thanks for catching that.  I admit that with my workflow this
is something i'd never noticed / figured out why it was important, so
i'm glad someone else has taken a look.  Woo, zathura on Guix has more
than one user now, presumably! :)

Ciao,
p.

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Mark H Weaver]

Alex Kost <alezost@gmail.com> writes:

> I've noticed that the packages for zathura plugins install .desktop
> files into:
>
> /gnu/store/…/usr/share/applications/ instead of:
> /gnu/store/…/share/applications/
>
> The attached patch fixes it.

Good catch, but see below:

> From 74efced6076ef28b321cb2a6a69344f55b714b59 Mon Sep 17 00:00:00 2001
> From: Alex Kost <alezost@gmail.com>
> Date: Tue, 7 Jul 2015 11:17:06 +0300
> Subject: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.
>
> * gnu/packages/pdf.scm (zathura-cb, zathura-ps, zathura-djvu,
>   zathura-pdf-poppler): Add PREFIX to 'make-flags' to install
>   ".desktop" files into "share", not "usr/share".
> ---
>  gnu/packages/pdf.scm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
> index 82e8c88..0e8817d 100644
> --- a/gnu/packages/pdf.scm
> +++ b/gnu/packages/pdf.scm
> @@ -173,7 +173,7 @@
>      (arguments
>       `(#:make-flags
>         `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
> +          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")

It would be better to leave DESTDIR empty and set PREFIX=<%output>, so:

> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
> +       `(,(string-append "PREFIX=" (assoc-ref %outputs "out"))

There is a conceptual difference between PREFIX and DESTDIR: at install
time, files are copied to ${DESTDIR}${PREFIX}, and then at run time
files are expected to be at ${PREFIX}.  So in general, we don't want to
use DESTDIR in Guix, and we want to set PREFIX to the output directory.

     Thanks!
       Mark

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Alex Kost]

Mark H Weaver (2015-07-07 18:07 +0300) wrote:

[...]
>>      (arguments
>>       `(#:make-flags
>>         `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
>> +          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
>
> It would be better to leave DESTDIR empty and set PREFIX=<%output>, so:

It wouldn't work for these packages.

>> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>> +       `(,(string-append "PREFIX=" (assoc-ref %outputs "out"))
>
> There is a conceptual difference between PREFIX and DESTDIR: at install
> time, files are copied to ${DESTDIR}${PREFIX}, and then at run time
> files are expected to be at ${PREFIX}.  So in general, we don't want to
> use DESTDIR in Guix, and we want to set PREFIX to the output directory.

I know, but these zathura plugins do not provide configure stages, and
PREFIX is not even used in the manually written "Makefile"s.  PREFIX is
used in "config.mk" (which is included in a Makefile) to define LIBDIR
and DESKTOPPREFIX.

And due to Makefile things are installed in ${DESTDIR}${PLUGINDIR} and
${DESTDIR}${DESKTOPPREFIX}.

-- 
Alex

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Alex Kost]

Paul van der Walt (2015-07-07 11:43 +0300) wrote:

> Hey Alex,
>
> On 2015-07-07 at 10:33, quoth Alex Kost:
>> I've noticed that the packages for zathura plugins install .desktop
>> files into:
>>
>> /gnu/store/…/usr/share/applications/ instead of:
>> /gnu/store/…/share/applications/
>>
>> The attached patch fixes it.
>
> My bad!  Thanks for catching that.  I admit that with my workflow this
> is something i'd never noticed / figured out why it was important, so
> i'm glad someone else has taken a look.  Woo, zathura on Guix has more
> than one user now, presumably! :)

Yeah :-) I've been using it for some time (thanks for packaging it!) and
I just accidentally noticed "usr" directory in my .guix-profile.

-- 
Alex

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Mark H Weaver]

[-- Attachment #1: Type: text/plain, Size: 2166 bytes --]

Alex Kost <alezost@gmail.com> writes:

> Mark H Weaver (2015-07-07 18:07 +0300) wrote:
>
> [...]
>>>      (arguments
>>>       `(#:make-flags
>>>         `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>>> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
>>> +          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
>>
>> It would be better to leave DESTDIR empty and set PREFIX=<%output>, so:
>
> It wouldn't work for these packages.

I made this change and the packages seem to build properly.  See below
for my proposed patch.

>>> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>>> +       `(,(string-append "PREFIX=" (assoc-ref %outputs "out"))
>>
>> There is a conceptual difference between PREFIX and DESTDIR: at install
>> time, files are copied to ${DESTDIR}${PREFIX}, and then at run time
>> files are expected to be at ${PREFIX}.  So in general, we don't want to
>> use DESTDIR in Guix, and we want to set PREFIX to the output directory.
>
> I know, but these zathura plugins do not provide configure stages, and
> PREFIX is not even used in the manually written "Makefile"s.  PREFIX is
> used in "config.mk" (which is included in a Makefile) to define LIBDIR
> and DESKTOPPREFIX.
>
> And due to Makefile things are installed in ${DESTDIR}${PLUGINDIR} and
> ${DESTDIR}${DESKTOPPREFIX}.

Okay, but the default values of PLUGINDIR and DESKTOPPREFIX are defined
in terms of PREFIX.  Those variables should contain the absolute
pathnames of where these files will be located at run time.

As is, these zathura-* packages are setting DESTDIR, PREFIX, and
PLUGINDIR incorrectly.  Now, it may be that in this case, it'll work
properly anyway (for now), but I'd still prefer to set them correctly
for two reasons:

* It is very common for packagers to look at existing packages for
  examples of good practices, and I don't want to spread this confusion
  about DESTDIR and PREFIX.

* Even if these build systems don't currently depend on proper settings
  of PREFIX, DESTDIR, and PLUGINDIR today, there's no guarantee that
  this will remain the case tomorrow.

So, how about the following patch instead?  What do you think?

      Mark



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: [PATCH] gnu: zathura-{cb,ps,djvu,pdf-poppler}: Fix installation of desktop files --]
[-- Type: text/x-patch, Size: 2623 bytes --]

From 62c2cc48e6a34c596e1e5a0bd0f873cd3d3f9218 Mon Sep 17 00:00:00 2001
From: Mark H Weaver <mhw@netris.org>
Date: Tue, 7 Jul 2015 14:20:54 -0400
Subject: [PATCH] gnu: zathura-{cb,ps,djvu,pdf-poppler}: Fix installation of
 desktop files.

Based on a patch by Alex Kost <alezost@gmail.com>.

* gnu/packages/pdf.scm (zathura-cb, zathura-ps, zathura-djvu)
  (zathura-pdf-poppler)[arguments]: In make-flags, set PREFIX instead of
  DESTDIR and adjust PLUGINDIR accordingly.
---
 gnu/packages/pdf.scm | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
index 82e8c88..6cdc846 100644
--- a/gnu/packages/pdf.scm
+++ b/gnu/packages/pdf.scm
@@ -172,8 +172,9 @@
     (build-system gnu-build-system)
     (arguments
      `(#:make-flags
-       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+       `(,(string-append "PREFIX=" %output)
+         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
+         "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -203,8 +204,9 @@ using libarchive.")
     (build-system gnu-build-system)
     (arguments
      `(#:make-flags
-       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+       `(,(string-append "PREFIX=" %output)
+         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
+         "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -235,8 +237,9 @@ using libspectre.")
     (build-system gnu-build-system)
     (arguments
      `(#:make-flags
-       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+       `(,(string-append "PREFIX=" %output)
+         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
+         "CC=gcc")
        #:tests? #f ; Package does not contain tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
@@ -268,8 +271,9 @@ using the DjVuLibre library.")
     (build-system gnu-build-system)
     (arguments
      `(#:make-flags
-       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
-          "PLUGINDIR=/lib/zathura" "CC=gcc")
+       `(,(string-append "PREFIX=" %output)
+         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
+         "CC=gcc")
        #:tests? #f ; Package does not include tests.
        #:phases
        (alist-delete 'configure %standard-phases)))
-- 
2.4.3

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Alex Kost]

Mark H Weaver (2015-07-07 21:36 +0300) wrote:

> Alex Kost <alezost@gmail.com> writes:
>
>> Mark H Weaver (2015-07-07 18:07 +0300) wrote:
>>
>> [...]
>>>>      (arguments
>>>>       `(#:make-flags
>>>>         `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>>>> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
>>>> +          "PREFIX=" "PLUGINDIR=/lib/zathura" "CC=gcc")
>>>
>>> It would be better to leave DESTDIR empty and set PREFIX=<%output>, so:
>>
>> It wouldn't work for these packages.
>
> I made this change and the packages seem to build properly.  See below
> for my proposed patch.

It's not fair!  The packages are built properly with the change of
PLUGINDIR that you didn't mention.

I'm joking :-)

>>>> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>>>> +       `(,(string-append "PREFIX=" (assoc-ref %outputs "out"))
>>>
>>> There is a conceptual difference between PREFIX and DESTDIR: at install
>>> time, files are copied to ${DESTDIR}${PREFIX}, and then at run time
>>> files are expected to be at ${PREFIX}.  So in general, we don't want to
>>> use DESTDIR in Guix, and we want to set PREFIX to the output directory.
>>
>> I know, but these zathura plugins do not provide configure stages, and
>> PREFIX is not even used in the manually written "Makefile"s.  PREFIX is
>> used in "config.mk" (which is included in a Makefile) to define LIBDIR
>> and DESKTOPPREFIX.
>>
>> And due to Makefile things are installed in ${DESTDIR}${PLUGINDIR} and
>> ${DESTDIR}${DESKTOPPREFIX}.
>
> Okay, but the default values of PLUGINDIR and DESKTOPPREFIX are defined
> in terms of PREFIX.  Those variables should contain the absolute
> pathnames of where these files will be located at run time.
>
> As is, these zathura-* packages are setting DESTDIR, PREFIX, and
> PLUGINDIR incorrectly.  Now, it may be that in this case, it'll work
> properly anyway (for now), but I'd still prefer to set them correctly
> for two reasons:
>
> * It is very common for packagers to look at existing packages for
>   examples of good practices, and I don't want to spread this confusion
>   about DESTDIR and PREFIX.
>
> * Even if these build systems don't currently depend on proper settings
>   of PREFIX, DESTDIR, and PLUGINDIR today, there's no guarantee that
>   this will remain the case tomorrow.
>
> So, how about the following patch instead?  What do you think?

I absolutely agree, thanks for the detailed description!

[...]
> diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
> index 82e8c88..6cdc846 100644
> --- a/gnu/packages/pdf.scm
> +++ b/gnu/packages/pdf.scm
> @@ -172,8 +172,9 @@
>      (build-system gnu-build-system)
>      (arguments
>       `(#:make-flags
> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
> +       `(,(string-append "PREFIX=" %output)
> +         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
> +         "CC=gcc")

As you change the whole clause anyway, wouldn't it be more good-looking
to remove those "`" / "," and just use "list" instead?:

          (list (string-append "PREFIX=" %output)
                (string-append "PLUGINDIR=" %output "/lib/zathura")
                "CC=gcc")

-- 
Alex

Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.

[Mark H Weaver]

Alex Kost <alezost@gmail.com> writes:

> Mark H Weaver (2015-07-07 21:36 +0300) wrote:
>
>> diff --git a/gnu/packages/pdf.scm b/gnu/packages/pdf.scm
>> index 82e8c88..6cdc846 100644
>> --- a/gnu/packages/pdf.scm
>> +++ b/gnu/packages/pdf.scm
>> @@ -172,8 +172,9 @@
>>      (build-system gnu-build-system)
>>      (arguments
>>       `(#:make-flags
>> -       `(,(string-append "DESTDIR=" (assoc-ref %outputs "out"))
>> -          "PLUGINDIR=/lib/zathura" "CC=gcc")
>> +       `(,(string-append "PREFIX=" %output)
>> +         ,(string-append "PLUGINDIR=" %output "/lib/zathura")
>> +         "CC=gcc")
>
> As you change the whole clause anyway, wouldn't it be more good-looking
> to remove those "`" / "," and just use "list" instead?:
>
>           (list (string-append "PREFIX=" %output)
>                 (string-append "PLUGINDIR=" %output "/lib/zathura")
>                 "CC=gcc")

Agreed.  Pushed with this change.

    Thanks!
      Mark