On Wed, Apr 05, 2023 at 12:54:14PM -0400, Jeff King wrote:
> On Wed, Apr 05, 2023 at 04:52:40PM +0200, Patrick Steinhardt wrote:
> 
> > > Is there a reason to not set PERL_PATH, which is the
> > > documented method to handle this?  From the Makefike:
> > > 
> > > # Define PERL_PATH to the path of your Perl binary (usually /usr/bin/perl).
> > 
> > Setting PERL_PATH helps with a subset of invocations where the Makefile
> > either executes Perl directly or where it writes the shebang itself. But
> > the majority of scripts I'm touching have `#!/usr/bin/perl` as shebang,
> > and that path is not adjusted by setting PERL_PATH.
> 
> Which scripts? If I do:
> 
>   mkdir /tmp/foo
>   ln -s /usr/bin/perl /tmp/foo/my-perl
>   make PERL_PATH=/tmp/foo/my-perl prefix=/tmp/foo install
> 
>   head -n 1 /tmp/foo/bin/git-cvsserver
> 
> Then I see:
> 
>   #!/tmp/foo/my-perl
> 
> And that is due to this segment in the Makefile:
> 
>   $(SCRIPT_PERL_GEN): % : %.perl GIT-PERL-DEFINES GIT-PERL-HEADER GIT-VERSION-FILE
>           $(QUIET_GEN) \
>           sed -e '1{' \
>               -e '        s|#!.*perl|#!$(PERL_PATH_SQ)|' \
>               -e '        r GIT-PERL-HEADER' \
>               -e '        G' \
>               -e '}' \
>               -e 's/@@GIT_VERSION@@/$(GIT_VERSION)/g' \
>               $< >$@+ && \
>           chmod +x $@+ && \
>           mv $@+ $@
> 
> And that behavior goes all the way back to bc6146d2abc ('build' scripts
> before installing., 2005-09-08). If there are some perl scripts we are
> "building" outside of this rule, then that is probably a bug.
> 
> The only thing I found via:
> 
>   find /tmp/foo -type | xargs grep /usr/bin/perl
> 
> was a sample hook (which is probably a bug; we do munge the hook scripts
> to replace @PERL_PATH@, etc, but I think the Makefile never learned that
> the template hook scripts might be something other than shell scripts).

Yeah, agreed, the scripts we install are fine from all I can tell. I
should've clarified, but what I care about is our build infra as well as
our test scripts. That's neither clear from the commit description nor
from the changes that I'm doing.

I'd be happy to keep the current state of installed scripts as-is and
resend another iteration of this patch that only addresses shebangs used
in internal scripts.

Patrick