On Tue, Nov 21 2017, Dan Jacques jotted:
> Ævar Arnfjörð Bjarmason @ 2017-11-20 21:50 UTC suggested:
>
>> So LeonT over at #p5p helped me with this. He believes this'll work
>> (unless MakeMaker INSTALL_BASE is set, but that should break the Git
>> install anyway):
>
> I think that the problem with this approach is that it uses the local
> "Config" module. The primary purpose of RUNTIME_PREFIX(_PERL) is that one
> can build/install Git into a directory, then either move that directory
> somewhere else or archive it and put it on a different (binary-compatible)
> system altogether.
>
> The latter case concerns me here. If the "Config" module is loading local
> system paths, then the relative pathing between "$Config{installsitelib}"
> and "$Config{siteprefixexp}" may not be consistent between systems, so an
> archive built from one system may not have a compatible relative
> directory structure when resolved with the Config module on another
> system.
I don't see how this is different from any other option we build git
with. When we dynamically link to e.g. PCRE under RUNTIME_PREFIX*=Yes
you can move the installed git from /tmp/git to /tmp/git2, but it'll
still expect the specific version of the *.so libraries to be in
/usr/lib or whatever.
Similarly we under the default MakeMaker path add the perl version to
the directories we have, you can move git from /tmp/git to /tmp/git2 no
problem, since that won't change the perl version, but if you upgrade
the global perl itself from 5.20 to 5.24 you'll need to re-build.
> Since we control the installation process and paths, we know that the
> directory structure looks someting like:
>
> .../prefix/$GITEXECDIR/git-perl-script
> .../prefix/$RELPERLPATH/Git.pm
>
Having said the above, I don't understand why we're using MakeMaker at
all and I think we should just get rid of it.
We're not using the perldoc toolchain to build manpages from *.pod for
these, and we don't have any C bindings, it seems to me that we could
simply replace this whole thing with a removal of all things Make-y from
perl/* and just do the minor work of creating a top-levle git-perl-lib
in our install $PREFIX and make the perl stuff use that.
Looking at the history of the Makefile.PL it originally had XS stuff
(which you'd need a Makefile.PL for), but this was removed in 18b0fc1ce1
before it made it to master.
My comment on your patch series was just that with the method I posted
there's no reason for why RUNTIME_PREFIX*=Yes and MakeMaker need to be
mutually exclusive, so if we're keeping the MakeMaker it seems to me
that we can support both.
But we can probably just get rid of MakeMaker.
> Our goal is to, given the directory that "git-perl-script" belongs to,
> first identify the path for ".../prefix" and then append "$RELPERLPATH" to
> it to generate the full library path.
>
> The most straightforward way to do this, to me, is to:
>
> 1) Have the Makefile hard-code "$RELPERLPATH" and "$GITEXECDIR" (relative
> paths) into the header code.
> 2) Assert that "$FindBin::Bin" (the directory containing the script) ends
> with "$GITEXECDIR".
> 3) Remove "$GITEXECDIR" from the end of "$FindBin::Bin" to obtain
> ".../prefix" ($prefix). Simple string truncation is probably fine for
> this.
> 4) Add "File::Spec->catdir($prefix, $RELPERLPATH)" to "lib".
>
> I don't think path separators are a problem, since the Makefile uses "/"
> indiscriminately. Even Git-for-Windows seems to run its PERL scripts in
> a POSIX environment (mingw).
Right. I don't know that they are either, it just stuck me as an odd
inconsistency that you're going out of your way to do catdir($p, "lib")
in one place and then hardcoding unix-like paths in another place.
If it's not neede dwe can just do "$p/lib".
> Does this sound like a reasonable way to proceed?
I think the best way to proceed is probalby to just start getting rid of
the perl/* make stuff as discussed above.
Is that something you're interested in / have time for, otherwise I
could see if I could find some time, but I don't have a lot these days.
Which is not to say that I think that should block this patch series or
anything. If it really needs to disable MakeMaker to work we're no worse
off than before.
