[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages.
From: |
Alex Kost |
Subject: |
Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages. |
Date: |
Tue, 07 Jul 2015 22:25:32 +0300 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Mark H Weaver (2015-07-07 21:36 +0300) wrote:
> Alex Kost <address@hidden> 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