Extra checks for PERL_PATH and SHELL_PATH?

Extra checks for PERL_PATH and SHELL_PATH?

[Matt Kraai]

Hi,

The top-level Makefile currently contains

> ifndef SHELL_PATH
> 	SHELL_PATH = /bin/sh
> endif
> ifndef PERL_PATH
> 	PERL_PATH = /usr/bin/perl
> endif

The checks are only necessary if these variables need to be overridden
by environment variables, not just via the make command line.  Is this
the case?

-- 
Matt Kraai                                           http://ftbfs.org/

Re: Extra checks for PERL_PATH and SHELL_PATH?

[Junio C Hamano]

Matt Kraai <kraai@ftbfs.org> writes:

> The top-level Makefile currently contains
>
>> ifndef SHELL_PATH
>> 	SHELL_PATH = /bin/sh
>> endif
>> ifndef PERL_PATH
>> 	PERL_PATH = /usr/bin/perl
>> endif
>
> The checks are only necessary if these variables need to be overridden
> by environment variables, not just via the make command line.  Is this
> the case?

It may not have been the original intention, but the above would mean that
some people may have learned to run "SHELL_PATH=/bin/ksh make" and
changing it would break things for them, no?

I do not think changing them is bad per-se, but we would need to add extra
warnings in the release note to explain this change, that's all.  This
would only affect people who build from the source (including distro
people) so it is not really a big deal.

[PATCH] Makefile: set PERL_PATH and SHELL_PATH unconditionally

[Matt Kraai]

Do not check whether PERL_PATH and SHELL_PATH are undefined before
setting their default values.  This prevents them from being set via
environment variables.

Signed-off-by: Matt Kraai <kraai@ftbfs.org>
---
 On Mon, Oct 19, 2009 at 11:36:26PM -0700, Junio C Hamano wrote:
 > Matt Kraai <kraai@ftbfs.org> writes:
 > 
 > > The top-level Makefile currently contains
 > >
 > >> ifndef SHELL_PATH
 > >> 	SHELL_PATH = /bin/sh
 > >> endif
 > >> ifndef PERL_PATH
 > >> 	PERL_PATH = /usr/bin/perl
 > >> endif
 > >
 > > The checks are only necessary if these variables need to be overridden
 > > by environment variables, not just via the make command line.  Is this
 > > the case?
 > 
 > It may not have been the original intention, but the above would mean that
 > some people may have learned to run "SHELL_PATH=/bin/ksh make" and
 > changing it would break things for them, no?

 Yes, that's what I was concerned about.  This appears to be possible
 for PERL_PATH on all platforms and for SHELL_PATH on platforms other
 than SCO UnixWare, SunOS, and IRIX.

 > I do not think changing them is bad per-se, but we would need to add extra
 > warnings in the release note to explain this change, that's all.  This
 > would only affect people who build from the source (including distro
 > people) so it is not really a big deal.

 I hope this patch is OK.

 Documentation/RelNotes-1.6.6.txt |    3 +++
 Makefile                         |    8 ++------
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/RelNotes-1.6.6.txt b/Documentation/RelNotes-1.6.6.txt
index 5f1fecb..bfda14c 100644
--- a/Documentation/RelNotes-1.6.6.txt
+++ b/Documentation/RelNotes-1.6.6.txt
@@ -58,3 +58,6 @@ release, unless otherwise noted.
    whitespace attribute).  The 'trailing-space' whitespace error class has
    become a short-hand to cover both of these and there is no behaviour
    change for existing set-ups.
+
+ * PERL_PATH and SHELL_PATH may not be overridden using environment
+   variables during the build.
diff --git a/Makefile b/Makefile
index 42b7d60..5bac305 100644
--- a/Makefile
+++ b/Makefile
@@ -392,12 +392,8 @@ ALL_PROGRAMS = $(PROGRAMS) $(SCRIPTS)
 OTHER_PROGRAMS = git$X
 
 # Set paths to tools early so that they can be used for version tests.
-ifndef SHELL_PATH
-	SHELL_PATH = /bin/sh
-endif
-ifndef PERL_PATH
-	PERL_PATH = /usr/bin/perl
-endif
+SHELL_PATH = /bin/sh
+PERL_PATH = /usr/bin/perl
 
 export PERL_PATH
 
-- 
1.6.5

Re: [PATCH] Makefile: set PERL_PATH and SHELL_PATH unconditionally

[Junio C Hamano]

Matt Kraai <kraai@ftbfs.org> writes:

> Do not check whether PERL_PATH and SHELL_PATH are undefined before
> setting their default values.  This prevents them from being set via
> environment variables.

Is there an upside of "preventing them from getting set", by the way?

Re: [PATCH] Makefile: set PERL_PATH and SHELL_PATH unconditionally

[Matt Kraai]

On Tue, Oct 20, 2009 at 09:18:29AM -0700, Junio C Hamano wrote:
> Matt Kraai <kraai@ftbfs.org> writes:
> 
> > Do not check whether PERL_PATH and SHELL_PATH are undefined before
> > setting their default values.  This prevents them from being set via
> > environment variables.
> 
> Is there an upside of "preventing them from getting set", by the way?

Not that I know of.

I originally thought that the checks were superfluous, but now I just
believe they're inconsistent and confusing to people like me who think
they understand Makefiles but don't.  :)

-- 
Matt Kraai                                             http://ftbfs.org/