threads / patch / 14849

patchMakefile: use backticks rather than $() notation to support ancient shells

Subject: [PATCH] Makefile: use backticks rather than $() notation to support ancient shells

## tl;dr

5 messages between Aug 5, 2008 and Aug 8, 2008. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Brandon Casey· Aug 5, 2008, 23:22 UTC · lore

Since make is using /bin/sh to execute shell code, avoid the newish shell construct $() so older (ancient) shells can execute the shell code in the Makefile.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

I know that $() is preferred in the main scripts, but the Makefile is using /bin/sh to execute shell code and there are already a few places in the Makefile using back-ticks, so it doesn't seem like going against the flow too much.

Otherwise, should we set the SHELL variable to the configured SHELL_PATH at some point in the Makefile?

-brandon

 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

Show changes to Makefile +2 −2

diff --git a/Makefile b/Makefile
index 608a185..421af24 100644
--- a/Makefile
+++ b/Makefile
@@ -1366,8 +1366,8 @@ endif
 ifneq (,$X)
 	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$(DESTDIR_SQ)$(gitexec_instdir_SQ)/$p';)
 endif
-	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \
-	execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
+	bindir=`cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd` && \
+	execdir=`cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd` && \
 	if test "z$$bindir" != "z$$execdir"; \
 	then \
 		ln -f "$$bindir/git$X" "$$execdir/git$X" || \

-- 
1.6.0.rc1.87.g56c9f.dirty

Johannes Schindelin· Aug 6, 2008, 00:10 UTC · re: Brandon Casey · lore

Re: [PATCH] Makefile: use backticks rather than $() notation to support ancient shells

Hi,

On Tue, 5 Aug 2008, Brandon Casey wrote:

> Otherwise, should we set the SHELL variable to the configured SHELL_PATH 
> at some point in the Makefile?

I think that would make more sense, especially since it would catch wrong SHELL_PATH early.

Maybe we can even have some sanity check that tests if SHELL_PATH groks $()?

Ciao, Dscho

Brandon Casey· Aug 7, 2008, 19:03 UTC · re: Johannes Schindelin · lore

[PATCH] Makefile: set SHELL to value of SHELL_PATH

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Johannes Schindelin wrote:

Show 9 quoted lines

> Hi,
> 
> On Tue, 5 Aug 2008, Brandon Casey wrote:
> 
>> Otherwise, should we set the SHELL variable to the configured SHELL_PATH 
>> at some point in the Makefile?
> 
> I think that would make more sense, especially since it would catch 
> wrong SHELL_PATH early.

You're right, this makes more sense.

-brandon

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

Show changes to Makefile +2 −0

diff --git a/Makefile b/Makefile
index 52c67c1..61fc86d 100644
--- a/Makefile
+++ b/Makefile
@@ -1060,6 +1060,8 @@ export TAR INSTALL DESTDIR SHELL_PATH
 
 ### Build rules
 
+SHELL = $(SHELL_PATH)
+
 all:: $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
 	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$p';)

-- 
1.5.6.2

Brandon Casey· Aug 7, 2008, 19:06 UTC · re: Brandon Casey · lore

[PATCH] Makefile: add a target which will abort compilation with ancient shells

This adds a make target which can be used to try to execute certain shell constructs which are required for compiling and running git.

This patch provides a test for the $() notation for command substition which is used in the Makefile and extensively in the git scripts.

The make target is named in such a way as to be a hint to the user that SHELL_PATH should be set to an appropriate shell. If the shell command fails, the user should receive a message similar to the following:

make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 2

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---

Johannes Schindelin wrote:

> Maybe we can even have some sanity check that tests if SHELL_PATH groks 
> $()?

how about this?

-brandon

 Makefile |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

Show changes to Makefile +7 −1

diff --git a/Makefile b/Makefile
index 61fc86d..90c5a13 100644
--- a/Makefile
+++ b/Makefile
@@ -1062,7 +1062,7 @@ export TAR INSTALL DESTDIR SHELL_PATH
 
 SHELL = $(SHELL_PATH)
 
-all:: $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
+all:: shell_compatibility_test $(ALL_PROGRAMS) $(BUILT_INS) $(OTHER_PROGRAMS) GIT-BUILD-OPTIONS
 ifneq (,$X)
 	$(foreach p,$(patsubst %$X,%,$(filter %$X,$(ALL_PROGRAMS) $(BUILT_INS) git$X)), $(RM) '$p';)
 endif
@@ -1075,6 +1075,11 @@ endif
 	$(QUIET_SUBDIR0)perl $(QUIET_SUBDIR1) PERL_PATH='$(PERL_PATH_SQ)' prefix='$(prefix_SQ)' all
 	$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1)
 
+please_set_SHELL_PATH_to_a_more_modern_shell:
+	@$$(:)
+
+shell_compatibility_test: please_set_SHELL_PATH_to_a_more_modern_shell
+
 strip: $(PROGRAMS) git$X
 	$(STRIP) $(STRIP_OPTS) $(PROGRAMS) git$X
 
@@ -1457,6 +1462,7 @@ endif
 	$(RM) GIT-VERSION-FILE GIT-CFLAGS GIT-GUI-VARS GIT-BUILD-OPTIONS
 
 .PHONY: all install clean strip
+.PHONY: shell_compatibility_test please_set_SHELL_PATH_to_a_more_modern_shell
 .PHONY: .FORCE-GIT-VERSION-FILE TAGS tags cscope .FORCE-GIT-CFLAGS
 .PHONY: .FORCE-GIT-BUILD-OPTIONS

-- 
1.5.6.2

Junio C Hamano· Aug 8, 2008, 20:19 UTC · re: Brandon Casey · lore

Re: [PATCH] Makefile: add a target which will abort compilation with ancient shells

Brandon Casey <casey@nrlssc.navy.mil> writes:

Show 21 quoted lines

> This adds a make target which can be used to try to execute certain shell
> constructs which are required for compiling and running git.
>
> This patch provides a test for the $() notation for command substition
> which is used in the Makefile and extensively in the git scripts.
>
> The make target is named in such a way as to be a hint to the user that
> SHELL_PATH should be set to an appropriate shell. If the shell command
> fails, the user should receive a message similar to the following:
>
> make: *** [please_set_SHELL_PATH_to_a_more_modern_shell] Error 2
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---
>
>
> Johannes Schindelin wrote:
>> Maybe we can even have some sanity check that tests if SHELL_PATH groks 
>> $()?
>
> how about this?

Thanks, both. I think this is cute and at the same time the right thing to do ;-).