Re: [PATCH v4 0/4] RUNTIME_PREFIX relocatable Git
- Date: Wed, 29 Nov 2017 22:12:55 +0100
- From: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
- Subject: Re: [PATCH v4 0/4] RUNTIME_PREFIX relocatable Git
On Wed, Nov 29 2017, Dan Jacques jotted:
> 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:
> 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
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.