From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Kost Subject: Re: [PATCH] gnu: pdf: Fix installing desktop files of zathura packages. Date: Tue, 07 Jul 2015 22:25:32 +0300 Message-ID: <87d203ixcj.fsf@gmail.com> References: <87twtgicz4.fsf@gmail.com> <87zj380zxg.fsf@netris.org> <87io9vj28o.fsf@gmail.com> <87r3oj24sh.fsf@netris.org> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:38108) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCYUt-0005YI-8T for guix-devel@gnu.org; Tue, 07 Jul 2015 15:25:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZCYUp-0005mu-OX for guix-devel@gnu.org; Tue, 07 Jul 2015 15:25:39 -0400 Received: from mail-la0-x22c.google.com ([2a00:1450:4010:c03::22c]:35251) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZCYUp-0005mi-0P for guix-devel@gnu.org; Tue, 07 Jul 2015 15:25:35 -0400 Received: by labgy5 with SMTP id gy5so56208146lab.2 for ; Tue, 07 Jul 2015 12:25:34 -0700 (PDT) In-Reply-To: <87r3oj24sh.fsf@netris.org> (Mark H. Weaver's message of "Tue, 07 Jul 2015 14:36:46 -0400") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Mark H Weaver Cc: guix-devel@gnu.org Mark H Weaver (2015-07-07 21:36 +0300) wrote: > Alex Kost 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