Use SHELL_PATH

PATCH In review SungHyun Nam 4 participants 7 messages Jun 15, 2016 Kbuild

lore

Share & tools

lore

SungHyun Nam

author

2016-06-15 22:44 UTC #0 lore raw

PATCH 1/1 Use SHELL_PATH

Signed-off-by SungHyun Nam <[email protected]>

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

t/Makefile +1 -1

index a778865..0d65ced 100644
@@ -26,7 +26,7 @@ clean:
 	$(RM) -r 'trash directory' test-results
 
 aggregate-results:
-	./aggregate-results.sh test-results/t*-*
+	'$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*
 
 # we can test NO_OPTIMIZE_COMMITS independently of LC_ALL
 full-svn-test:

Signature · 1 line

1.5.6.3.350.g6c11a

Sverre Rabbelier

2016-06-15 22:44 UTC #1 lore raw

Quoted 20 lines · SungHyun Nam

Signed-off-by SungHyun Nam <[email protected]>

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

t/Makefile +1 -2

index a778865..0d65ced 100644
@@ -26,7 +26,7 @@ clean:
       $(RM) -r 'trash directory' test-results
 
 aggregate-results:
-       ./aggregate-results.sh test-results/t*-*
+       '$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*
 
 # we can test NO_OPTIMIZE_COMMITS independently of LC_ALL
 full-svn-test:
--

Quoted 1 line

1.5.6.3.350.g6c11a

It is not clear to me what this patch does, there is no justification in the commit msg either. Instead you say what is being done, which we can see from the commit diff. Please clarify?

Signature · 3 lines

Cheers,

Sverre Rabbelier

Junio C Hamano

2016-06-15 22:44 UTC #2 lore raw

Quoted 18 lines · Sverre Rabbelier

Quoted 17 lines · SungHyun Nam

Signed-off-by SungHyun Nam <[email protected]>

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

t/Makefile +1 -1

index a778865..0d65ced 100644
@@ -26,7 +26,7 @@ clean:
       $(RM) -r 'trash directory' test-results
 
 aggregate-results:
-       ./aggregate-results.sh test-results/t*-*
+       '$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*

Quoted 8 lines

Quoted 4 lines
t/Makefile +0 -1
 # we can test NO_OPTIMIZE_COMMITS independently of LC_ALL
 full-svn-test:
--
1.5.6.3.350.g6c11a
It is not clear to me what this patch does, there is no justification in the commit msg either. Instead you say what is being done, which we can see from the commit diff. Please clarify?

It wants to make sure that the shell specified from the toplevel Makefile (or from make command line) is used to run the aggregation script. It is often necessary when platform /bin/sh is not quite POSIX (e.g. the script in question uses arithmetic expansion "$(( $var1 + $var2 ))").

Just saying "Use specified shell to run shell scripts" on the title line would be sufficient, but I wonder if this is the only remaining place...

SungHyun Nam

author

2016-06-15 22:44 UTC #3 lore raw

Quoted 27 lines · Junio C Hamano

Quoted 17 lines · Sverre Rabbelier
Quoted 16 lines · SungHyun Nam
Signed-off-by SungHyun Nam <[email protected]>
---
 t/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
t/Makefile +1 -1
index a778865..0d65ced 100644
@@ -26,7 +26,7 @@ clean:
       $(RM) -r 'trash directory' test-results
 
 aggregate-results:
-       ./aggregate-results.sh test-results/t*-*
+       '$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*
 
Quoted 7 lines
Quoted 4 lines
t/Makefile +0 -1
 # we can test NO_OPTIMIZE_COMMITS independently of LC_ALL
 full-svn-test:
--
1.5.6.3.350.g6c11a
It is not clear to me what this patch does, there is no justification in the commit msg either. Instead you say what is being done, which we can see from the commit diff. Please clarify?

Quoted 7 lines

It wants to make sure that the shell specified from the toplevel Makefile (or from make command line) is used to run the aggregation script. It is often necessary when platform /bin/sh is not quite POSIX (e.g. the script in question uses arithmetic expansion "$(( $var1 + $var2 ))").
Just saying "Use specified shell to run shell scripts" on the title line would be sufficient, but I wonder if this is the only remaining place...

For the 'make test', yes, it's the only remaining place to me. Well, I skip the cvs/svn/.. tests which I never use. And I need this patch for the Solaris.

And I also need to run a script function below between 'make all' and 'make test/install'. I hope GIT does this. Of course, GIT's Makefile would use SHELL_PATH and PERL_PATH.

git_fix_interpreter()

{
    p="/usr/local/bin/perl"
    if test -x "$p"
    then
        for f in $(find . -name '*.perl')
        do
            e=$(dirname $f)/$(basename $f .perl)
            [ -f "$e" -a -x "$e" ] \
                && "$p" -pi -e 
's@^#!/usr/(bin/perl.*)$@#!/usr/local/\1@g' "$e"
        done
    fi
    if test -x /bin/bash
    then
        for f in $(find . -name '*.sh')
        do
            e=$(dirname $f)/$(basename $f .sh)
            [ -f "$e" -a -x "$e" ] \
                && "$p" -pi -e 's@^#!/bin/sh(.*)$@#!/bin/bash\1@g' "$e"
        done
    fi
}

SungHyun Nam

author

2016-06-15 22:44 UTC #4 lore raw

Quoted 3 lines · SungHyun Nam

And I also need to run a script function below between 'make all' and 'make test/install'. I hope GIT does this. Of course, GIT's Makefile would use SHELL_PATH and PERL_PATH.

Oops! I'm sorry! Ignore this part. GIT alreay does it, just i didn't realize that.

Regards,
namsh

Junio C Hamano

2016-06-15 22:44 UTC #5 lore raw

In reply to Junio C Hamano

Quoted 7 lines · Junio C Hamano

It wants to make sure that the shell specified from the toplevel Makefile (or from make command line) is used to run the aggregation script. It is often necessary when platform /bin/sh is not quite POSIX (e.g. the script in question uses arithmetic expansion "$(( $var1 + $var2 ))").
Just saying "Use specified shell to run shell scripts" on the title line would be sufficient, but I wonder if this is the only remaining place...

People on funny platforms might find this one useful, which is consistent with the way the patch under discussion called the shell. This lets you to say

SHELL_PATH = /Program Files/GNU/Bash

or something silly like that ;-)

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

Makefile +1 -1

index 9b52071..a2b2627 100644
@@ -153,7 +153,7 @@ all::
 # broken, or spawning external process is slower than built-in grep git has).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@'$(SHELL_PATH_SQ)' ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')

Johannes Schindelin

2016-06-15 22:44 UTC #6 lore raw

In reply to Sverre Rabbelier

Hi,

Quoted 18 lines · Sverre Rabbelier

Quoted 17 lines · SungHyun Nam

Signed-off-by SungHyun Nam <[email protected]>

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

t/Makefile +1 -1

index a778865..0d65ced 100644
@@ -26,7 +26,7 @@ clean:
       $(RM) -r 'trash directory' test-results
 
 aggregate-results:
-       ./aggregate-results.sh test-results/t*-*
+       '$(SHELL_PATH_SQ)' ./aggregate-results.sh test-results/t*-*

Quoted 8 lines

Quoted 4 lines
t/Makefile +0 -1
 # we can test NO_OPTIMIZE_COMMITS independently of LC_ALL
 full-svn-test:
--
1.5.6.3.350.g6c11a
It is not clear to me what this patch does, there is no justification in the commit msg either. Instead you say what is being done, which we can see from the commit diff. Please clarify?

My _guess_ is that this comes from a platform like Solaris, where /bin/sh is not even POSIX. And I'd expect aggregate-results to use some non-trivial shell constructs which break with such a broken shell.

But I completely agree, the commit message desperately wants to include some justification.

Ciao,
Dscho