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: > https://public-inbox.org/git/20171129153436.24471-1-avarab@xxxxxxxxx/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.
