Re: [PATCH] global: resolve Perl executable via PATH

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Apr 05 2023, Jeff King wrote:

> On Wed, Apr 05, 2023 at 09:18:20PM -0500, Felipe Contreras wrote:
>
>> On Wed, Apr 5, 2023 at 2:09 PM Jeff King <peff@peff.net> wrote:
>> > On Wed, Apr 05, 2023 at 07:32:22PM +0200, Patrick Steinhardt wrote:
>> 
>> > IMHO we should aim for fixing those inconsistencies, and then letting
>> > people set PERL_PATH as appropriate (even to something that will find it
>> > via $PATH if they want to).
>> 
>> We can aim to fix all those inconsistencies *eventually* while in the
>> meantime make them runnable for most people *today*.
>> 
>> It's not a dichotomy.
>
> It is if the proposed patches change the behavior in such a way as to
> make things less consistent.
>
> There are three plausible perls to run (whether intentionally or
> accidentally):
>
>   1. the one in PERL_PATH
>
>   2. /usr/bin/perl
>
>   3. the first one in $PATH
>
> What the code tries to do now is to consistently use (1). If there are
> cases that accidentally use (2), which is what I took Patrick's patch to
> mean, then that is a problem for people who set PERL_PATH to something
> else, but not for people who leave it as /usr/bin/perl. If we "fix"
> those cases by switching them to (3), then now things are less
> consistent for such people than when we started.
>
> But I am not clear on what those cases are (if any), and we have not
> seen Patrick's follow-up proposed patch.
>
> I did find one case that is accidentally doing (3), and posted a patch
> elsewhere in the thread to convert it to (1). If you prefer behavior
> (3), you might consider that a regression, but it seems meaningless
> given the 99% of other cases that are using (1). If you want (3) to be
> the behavior everywhere, then we'd need to completely change our stance
> on how we invoke perl, or we need to teach PERL_PATH to handle this case
> so that people building Git can choose their own preference (sadly I
> don't think "make PERL_PATH='/usr/bin/env perl'" quite works because we
> have to shell-quote it in some contexts before evaluating).

I just want to chime in to say that I've read this whole
thread-at-large, and I think what you're pointing out here is correct,
and that we should keep hardcoding "#!/usr/bin/perl", and then just have
"PERL_PATH" set.

I.e. most of Patrick's original patch is unnecessary, as we either use
"$PERL_PATH" in the Makefile already, or munge the shebang when we
install.

Then the only change we should need is the one you suggested in
<20230406032647.GA2092142@coredump.intra.peff.net> in the side-thread.

Using "env" liket his is also incorrect. I might have a "perl" in my
"$PATH" which I expect to use for e.g. by .bashrc, but I don't want that
perl to take priority over "$PERL_PATH" for git when I run some test
script.

I also wonder if something like this (untested) wouldn't be useful to
provide an earlier warning of this, instead of failing when we fail to
invoke the relevant scripts.

	diff --git a/Makefile b/Makefile
	index 60ab1a8b4f4..9abc2e52cfa 100644
	--- a/Makefile
	+++ b/Makefile
	@@ -910,6 +910,15 @@ ifndef PYTHON_PATH
	 	PYTHON_PATH = /usr/bin/python
	 endif
	 
	+define check-path-exists
	+ifeq ($$(wildcard $$($(1))),)
	+$$(error $(1) set to '$(2)', which does not exist)
	+endif
	+endef
	+
	+$(eval $(call check-path-exists,SHELL_PATH,$(SHELL_PATH)))
	+$(eval $(call check-path-exists,PERL_PATH,$(PERL_PATH)))
	+
	 export PERL_PATH
	 export PYTHON_PATH

That should not break anything in principle, as we already rely on these
to build git itself, but note that that's not the case with
"PYTHON_PATH".

In the case of "PERL_PATH" I think that's limited to building the docs,
so if we did something like this perhaps it should be in shared.mak, and
that check should be in Documentation/Makefile.

But perhaps it's all reduntant to just running into an error when we try
to generate-cmdlist.sh or whatever...