Re: [PATCH v4 0/4] RUNTIME_PREFIX relocatable Git

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 29, 2017, 21:12 UTC
Message-ID: <87h8tcvlew.fsf@evledraar.booking.com>
In-Reply-To: <20171129155637.89075-1-dnj@google.com>

On Wed, Nov 29 2017, Dan Jacques jotted:

Show 21 quoted lines

> This is a small update to incorporate some Windows fixes from Johannes.
> At this point, it passes the full test suite on Linux, Mac, and FreeBSD,
> as well as the Travis.ci test suites, with and without
> RUNTIME_PREFIX/RUNTIME_PREFIX_PERL flags.
>
> I'm happy with the patch set, and feel that it is ready to move forward.
> However, while it's been looked at by several people, and I have
> incorporated reviewer feedback, the patch set hasn't received any formal
> LGTM-style responses yet. I'm not sure what standard of review is required
> to move forward with a patch on this project, so maybe this is totally
> fine, but I wanted to make sure to point this out.
>
> I also want to note Ævar Arnfjörð Bjarmason's RFC:
> https://public-inbox.org/git/20171129153436.24471-1-avarab@gmail.com/T/
>
> The proposed patch set conflicts with the Perl installation directory
> changes in this patch set, as avarab@ notes. The proposed Perl installation
> process would simplify patch 0002 in this patch set. I don't think the
> landing order is terribly impactful - if this lands first, the patch in the
> RFC would delete a few more lines, and if this lands later, patch 0002
> would largely not be necessary.

In general this whole thing structurally looks good to me with the caveats noted in other review E-Mails.

I haven't done anything but skim the details of the "where's my executable" C code though, just looked at what it's doing structurally.

I think it makes sense for this to land first ahead of my patch. This is an actual feature you need, whereas I just hate our use of MakeMaker, but that can wait, unless you're keen to rebase this on my patch. Would probably make your whole diff a bit shorter.

The whole converting our absolute to relative paths in the make code is unavoidably ugly, but after having an initial knee-jerk reaction to it I don't see how it can be avoided. I was hoping most of these paths could/would just be a fixed path away from our libexec path, but alas due to having had these configurable all along that simplicity seems out of reach.

Maybe I asked this before, but I don't see any obvious reason for why RUNTIME_PREFIX_PERL is a different thing than RUNTIME_PREFIX as opposed to us just doing the right thing for the perl scripts.