DESTDIR implementation [Was:: ATTENTION: is the way DESTDIR was introduced completely wrong?]

[Gábor Kövesdán]

Dmitry Marakasov wrote:
> Hi!
>
> I'm now exploring new DESTDIR-related stuff, and I find it far from
> being useable. More of that, DESTDIR-related changes seem dangerous
> to me.
>
> As far as I understand, for port to support DESTDIR, it's files
> should be installed into ${DESTDIR}/${PREFIX}/foo (aka ${TARGETDIR}/foo),
> but all paths compiled into binaries (or config files) should remain
> `local' (i.e. ${PREFIX}/foo). Thus, when DESTDIR used, port will
> be installed into (for example) jail environment specified in
> DESTDIR, and will correctly work in that environment after chroot'ing
> into in. Am I right?
>   
Yes.
> Now, I think the way DESTDIR-related changes were done to bsd.port.mk is
> absolutely wrong. For example, X11BASE, LOCALBASE, DATADIR now contain
> DESTDIR. But, these variables are frequently used when changing paths
> hardcoded in port's sources (see many of my ports, for example
> games/fishsupper:
>
> @${REINPLACE_CMD} -e 's|data/|${DATADIR}/|' ${WRKSRC}/src/getreadydisplay.cc
>
> After change, all these ports will not support DESTDIR automatically.
> Incomplete list of such ports:
>
> find /usr/ports -name Makefile -exec grep -E \
> 	"REINPLACE_CMD.*(DATADIR|LOCALBASE|X11BASE)" {}
>
> Now, if we want to fix these, we'll need to change variables in
> REINPLACE_CMD lines: LOCALBASE to LOCALBASE_REL, X11BASE to X11BASE_REL
> and DATADIR... hmm, we have no DATADIR_REL, so all is left is
> ${PREFIX}/share/${PORTNAME}. That's unforgivable.
>   
Yes, that's right as well, but note if we had left LOCALBASE, LINUXBASE, 
X11BASE unchanged, we would need to change them in the *_DEPENDS lines. 
That's true that we don't have DATADIR_REL, etc. We could introduce them 
if needed, but that substitution can be done with make :S as well. 
DESTDIR implementation is complicated in many ways, no perfect 
implementation exist. As you said LOCALBASE is used in substitutions, bu 
also used in *_DEPENDS as well. Now those two cases have to be 
distinguished. One of them should have been changed at all... That's why 
I say no perfect implementation exist. If we implement new functions 
later, it can be very difficult...
> Also I see that new TARGETDIR/DESTDIR/...DIR scheme brings confusion.
> Latest DESTDIR-related port update that I see:
>
> http://www.freshports.org/x11-wm/jwm/files.php?message_id=200608141657.k7EGvce2008218@repoman.freebsd.org
>
> notice this:
>
> (http://www.freebsd.org/cgi/cvsweb.cgi/ports/x11-wm/jwm/Makefile.diff?r1=1.11&r2=1.12)
>
> -@${REINPLACE_CMD} -e 's|%%PREFIX%%|${PREFIX}|' ${WRKSRC}/example.jwmrc
> +@${REINPLACE_CMD} -e 's|%%PREFIX%%|${TARGETDIR}|' ${WRKSRC}/example.jwmrc
>
> Now example.jwmrc will contain GLOBAL system path, not jail-local one,
> which is of course wrong.
>
> Please tell that I'm stupid if I am wrong somewhere, but I think
> DESTDIR support, being introduced into ports collection in a way it's
> currently introduced, will bring much pain.
>   
It will, but I tried to avoid the most of the necessary pain. Changing 
all of *_DEPENDS would be good? I don't think so... As for this change, 
it is actually wrong. I CC'd the maintainer.
In the HEADS-UP message I sent, I wrote that one can feel free to 
contact me to review and test patches. Some people already did so. As my 
time lets me doing so, I test every patches sent to me.
> What I propose is:
> - Change variable naming scheme.
> All *BASE and *DIR vars should be reverted to their original meanings
> (i.e. local paths). Instead, INSTALL_ vars should be introduced:
> INSTALL_LOCALBASE=${DESTDIR}/${LOCALBASE}
> INSTALL_X11BASE=${DESTDIR}/${X11BASE}
> INSTALL_PREFIX=${DESTDIR}/${PREFIX}
> INSTALL_DATADIR=${DESTDIR}/${DATADIR}
>   
I don't think it will happen. It just makes the whole thing much more 
complicated.
> etc. These should be used in do-install target.
>
> * This is far more clean and understandable, 
> * This allows us to make all ports (around 5k) that define do-install target
>   DESTDIR-compatible (there still may be issues, but nevertheless).
>
> - Introduce variable DESTDIR_COMPATIBLE to explititely mark
>   DESTDIR-compatible ports.
>   
This is going to happen but in the opposite manner. We are planning to 
run an -exp run in the pointyhat cluster with DESTDIR set, and fix at 
least some of the most common ports, and mark the other ones NO_DESTDIR 
or something like that.
> * I don't think DESTDIR compatibility can be tested automatically, so
>   this would make freebsd user's life easier (user will be sure that after he
>   installs ports into [jail|other freebsd installation mounted via
>   nfs|locally] being set corresponging DESTDIR, nothing will break).
>   Without such variable, he'll never be sure.
> * Port maintainers will know what ports still are to be converted.
>   Nothing will be forgotten.
>
>   
P.S.: Please don't make other people panic with such subject. Other 
people also reviewed my code. We are just people, so we can make 
mistakes as well, but you can be sure that such a big change like 
DESTDIR got enough reviewal and test before committed into Mk. Portmgr 
is for ensuring people that only rational and working code gets 
committed into bsd.port.mk.
John, the author of an earlier DESTDIR implementation also reviewed my 
changes and helped to fix some minor issues.

-- 
Cheers,

Gabor