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

[Alex Kost <alezost@gmail.com>]

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