[PATCH] To make GIT-VERSION-FILE, search for git more widely

[PATCH] To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Problem: when git is installed into /usr/local/bin, running 'sudo make
install' won't find git in $PATH (because sudo strips PATH, for instance
on openSUSE 11.1, and doesn't include /usr/local/whatever).

This causes GIT-VERSION-FILE to not find git for git describe, and fall
back to DEF_VER.

This in turn causes rebuilds of major parts of the system.

This patch passes GIT through to the shell script, and additionally
looks for the current version and in ${prefix}/bin.

Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 GIT-VERSION-GEN |    7 ++++---
 Makefile        |    6 +++++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 39cde78..66c171e 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -2,6 +2,7 @@
 
 GVF=GIT-VERSION-FILE
 DEF_VER=v1.6.3.GIT
+test -x "$GIT" || GIT=git
 
 LF='
 '
@@ -12,12 +13,12 @@ if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
 elif test -d .git -o -f .git &&
-	VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
+	VN=$(${GIT} describe --abbrev=4 HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
-		git update-index -q --refresh
-		test -z "$(git diff-index --name-only HEAD --)" ||
+		${GIT} update-index -q --refresh
+		test -z "$(${GIT} diff-index --name-only HEAD --)" ||
 		VN="$VN-dirty" ;;
 	esac
 then
diff --git a/Makefile b/Makefile
index 6e21643..8e34877 100644
--- a/Makefile
+++ b/Makefile
@@ -177,7 +177,11 @@ all::
 # away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@{ GIT=./git$X ; type $$GIT >/dev/null 2>&1 ; } \
+	    || { GIT=$(prefix)/bin/git$X ; type $$GIT >/dev/null 2>&1 ; }\
+	    || GIT=git ; \
+	    export GIT ; \
+	    $(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-- 
1.6.3.rc2.9.g97ad0

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Johannes Sixt]

Matthias Andree schrieb:
> Problem: when git is installed into /usr/local/bin, running 'sudo make
> install' won't find git in $PATH (because sudo strips PATH, for instance
> on openSUSE 11.1, and doesn't include /usr/local/whatever).
> 
> This causes GIT-VERSION-FILE to not find git for git describe, and fall
> back to DEF_VER.
> 
> This in turn causes rebuilds of major parts of the system.
> 
> This patch passes GIT through to the shell script, and additionally
> looks for the current version and in ${prefix}/bin.
> 
> Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
> ---
>  GIT-VERSION-GEN |    7 ++++---
>  Makefile        |    6 +++++-
>  2 files changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 39cde78..66c171e 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -2,6 +2,7 @@
>  
>  GVF=GIT-VERSION-FILE
>  DEF_VER=v1.6.3.GIT
> +test -x "$GIT" || GIT=git
>  
>  LF='
>  '
> @@ -12,12 +13,12 @@ if test -f version
>  then
>  	VN=$(cat version) || VN="$DEF_VER"
>  elif test -d .git -o -f .git &&
> -	VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
> +	VN=$(${GIT} describe --abbrev=4 HEAD 2>/dev/null) &&

+	VN=$(${GIT} describe --abbrev=4 HEAD 2>/dev/null) &&
>  	case "$VN" in
>  	*$LF*) (exit 1) ;;
>  	v[0-9]*)
> -		git update-index -q --refresh
> -		test -z "$(git diff-index --name-only HEAD --)" ||
> +		${GIT} update-index -q --refresh
> +		test -z "$(${GIT} diff-index --name-only HEAD --)" ||
>  		VN="$VN-dirty" ;;
>  	esac
>  then
> diff --git a/Makefile b/Makefile
> index 6e21643..8e34877 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -177,7 +177,11 @@ all::
>  # away (some NTFS drivers seem to zero the contents in that scenario).
>  
>  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
> -	@$(SHELL_PATH) ./GIT-VERSION-GEN
> +	@{ GIT=./git$X ; type $$GIT >/dev/null 2>&1 ; } \
> +	    || { GIT=$(prefix)/bin/git$X ; type $$GIT >/dev/null 2>&1 ; }\
> +	    || GIT=git ; \
> +	    export GIT ; \
> +	    $(SHELL_PATH) ./GIT-VERSION-GEN
>  -include GIT-VERSION-FILE
>  
>  uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')


-- 
DI Johannes Sixt
Leiter Softwareentwicklung
Viscovery Software GmbH
Landstrasse 11
A-4020 Linz, Austria

T: +43-1-532 0570-51
M: +43-676-8494 9571
F: +43-1-532 0570-33
j.sixt@viscovery.net
www.viscovery.net

t u r n   y o u r   d a t a   i n t o   r e a l   v a l u e !

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Junio C Hamano]

Matthias Andree <matthias.andree@gmx.de> writes:

> Problem: when git is installed into /usr/local/bin, running 'sudo make
> install' won't find git in $PATH (because sudo strips PATH, for instance
> on openSUSE 11.1, and doesn't include /usr/local/whatever).

That sounds like a bug/misfeature in sudo (which I do not use) to me.

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Am 07.05.2009, 10:33 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> Matthias Andree <matthias.andree@gmx.de> writes:
>
>> Problem: when git is installed into /usr/local/bin, running 'sudo make
>> install' won't find git in $PATH (because sudo strips PATH, for instance
>> on openSUSE 11.1, and doesn't include /usr/local/whatever).
>
> That sounds like a bug/misfeature in sudo (which I do not use) to me.

Some points:

1. sudo is the default sysadmin setup on Ubuntu (more so than on openSUSE)

2. sudo strips the environment by default for security reasons

3. why would we want to use an old git version (rather than an up to date  
one) to extract the data?

-- 
Matthias Andree

[PATCH] Re: To make GIT-VERSION-FILE, search for git more widely

[Nicolas Sebrecht]

The 07/05/09, Matthias Andree wrote:

> Some points:
>
> 1. sudo is the default sysadmin setup on Ubuntu (more so than on openSUSE)
>
> 2. sudo strips the environment by default for security reasons

This is a configuration option that the sysadmin has to set (see
env_keep in man sudoers (5) or http://linux.die.net/man/5/sudoers).

I don't see any good reason to bypass this behaviour since it is what
sysadmins expect.

-- 
Nicolas Sebrecht

Re: [PATCH] Re: To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Am 07.05.2009, 10:54 Uhr, schrieb Nicolas Sebrecht <nicolas.s.dev@gmx.fr>:

> This is a configuration option that the sysadmin has to set (see
> env_keep in man sudoers (5) or http://linux.die.net/man/5/sudoers).

Salut Nicolas,

I'm aware of that.

> I don't see any good reason to bypass this behaviour since it is what
> sysadmins expect.

Are you agreeing or objecting?

Best regards

-- 
Matthias Andree

[PATCH] Re: To make GIT-VERSION-FILE, search for git more widely

[Nicolas Sebrecht]

The 07/05/09, Matthias Andree wrote:
> Am 07.05.2009, 10:54 Uhr, schrieb Nicolas Sebrecht <nicolas.s.dev@gmx.fr>:
>
> > This is a configuration option that the sysadmin has to set (see
> > env_keep in man sudoers (5) or http://linux.die.net/man/5/sudoers).
>
> Salut Nicolas,
>
> I'm aware of that.
>
> > I don't see any good reason to bypass this behaviour since it is what
> > sysadmins expect.
>
> Are you agreeing or objecting?

Objecting. By "this behaviour" I meant "the fact that sudo strips
environment variables".

-- 
Nicolas Sebrecht

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Johannes Sixt]

Junio C Hamano schrieb:
> Matthias Andree <matthias.andree@gmx.de> writes:
> 
>> Problem: when git is installed into /usr/local/bin, running 'sudo make
>> install' won't find git in $PATH (because sudo strips PATH, for instance
>> on openSUSE 11.1, and doesn't include /usr/local/whatever).
> 
> That sounds like a bug/misfeature in sudo (which I do not use) to me.

sudo resets the environment, in particular also PATH. Why would this be a bug?

Current distros set env_reset in /etc/soduers for a reason. Not that I
know the reason in detail, but I won't claim that I'm more clever with
regards to security issues than distro packagers; so I trust that if they
do it, then it makes sense.

-- Hannes

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Michael J Gruber]

Johannes Sixt venit, vidit, dixit 07.05.2009 10:47:
> Junio C Hamano schrieb:
>> Matthias Andree <matthias.andree@gmx.de> writes:
>>
>>> Problem: when git is installed into /usr/local/bin, running 'sudo make
>>> install' won't find git in $PATH (because sudo strips PATH, for instance
>>> on openSUSE 11.1, and doesn't include /usr/local/whatever).
>>
>> That sounds like a bug/misfeature in sudo (which I do not use) to me.
> 
> sudo resets the environment, in particular also PATH. Why would this be a bug?
> 
> Current distros set env_reset in /etc/soduers for a reason. Not that I
> know the reason in detail, but I won't claim that I'm more clever with
> regards to security issues than distro packagers; so I trust that if they
> do it, then it makes sense.
> 
> -- Hannes

sudo's behaviour is fine, but the OP's is not: doing "sudo make install"
amounts to building git as root, unless you have done a make as non-root
before. If you have done that make then GIT-VERSION-FILE will be
up-to-date, and GIT-VERSION-GEN will not even be called by "make install".

So I think the proposed patch would encourage wrong behaviour. (also,
the commit message mixes up ...-GEN and ...-FILE)

Michael

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Am 07.05.2009, 11:02 Uhr, schrieb Michael J Gruber  
<git@drmicha.warpmail.net>:

> Johannes Sixt venit, vidit, dixit 07.05.2009 10:47:
>> Junio C Hamano schrieb:
>>> Matthias Andree <matthias.andree@gmx.de> writes:
>>>
>>>> Problem: when git is installed into /usr/local/bin, running 'sudo make
>>>> install' won't find git in $PATH (because sudo strips PATH, for  
>>>> instance
>>>> on openSUSE 11.1, and doesn't include /usr/local/whatever).
>>>
>>> That sounds like a bug/misfeature in sudo (which I do not use) to me.
>>
>> sudo resets the environment, in particular also PATH. Why would this be  
>> a bug?
>>
>> Current distros set env_reset in /etc/soduers for a reason. Not that I
>> know the reason in detail, but I won't claim that I'm more clever with
>> regards to security issues than distro packagers; so I trust that if  
>> they
>> do it, then it makes sense.
>>
>> -- Hannes
>
> sudo's behaviour is fine, but the OP's is not: doing "sudo make install"
> amounts to building git as root, unless you have done a make as non-root
> before. If you have done that make then GIT-VERSION-FILE will be
> up-to-date, and GIT-VERSION-GEN will not even be called by "make  
> install".

Michael,

believe me that I've thoroughly analyzed this before proposing the change.

More details in the followup - I need to fix the commit message :-)

> So I think the proposed patch would encourage wrong behaviour. (also,
> the commit message mixes up ...-GEN and ...-FILE)

Whoops, good catch. PATCH v3 in a minute.

Best regards

-- 
Matthias Andree

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Am 07.05.2009, 10:47 Uhr, schrieb Johannes Sixt <j.sixt@viscovery.net>:

> Junio C Hamano schrieb:
>> Matthias Andree <matthias.andree@gmx.de> writes:
>>
>>> Problem: when git is installed into /usr/local/bin, running 'sudo make
>>> install' won't find git in $PATH (because sudo strips PATH, for  
>>> instance
>>> on openSUSE 11.1, and doesn't include /usr/local/whatever).
>>
>> That sounds like a bug/misfeature in sudo (which I do not use) to me.
>
> sudo resets the environment, in particular also PATH. Why would this be  
> a bug?
>
> Current distros set env_reset in /etc/soduers for a reason. Not that I
> know the reason in detail, but I won't claim that I'm more clever with
> regards to security issues than distro packagers; so I trust that if they
> do it, then it makes sense.

comments in my suoders file:

| ...
| # Prevent environment variables from influencing programs in an
| # unexpected or harmful way (CVE-2005-2959, CVE-2005-4158, CVE-2006-0151)
| Defaults always_set_home
| Defaults env_reset
| ...

I don't bother, this is a convenience-and-usability feature. It has always  
irritated me why "make && sudo make install" would build twice.

Let's fix this for good unless someone can show that this is harmful. (OK,  
the $(prefix)/bin/git part might grab an old version, but we may want to  
use that for bootstrapping, and it should usually be good enough.)

-- 
Matthias Andree

Re: [PATCH] To make GIT-VERSION-FILE, search for git more widely

[Johannes Sixt]

(sorry for the previous message; fingers were too fast)

Matthias Andree schrieb:
> +	VN=$(${GIT} describe --abbrev=4 HEAD 2>/dev/null) &&

> +		${GIT} update-index -q --refresh
> +		test -z "$(${GIT} diff-index --name-only HEAD --)" ||

Use "$GIT" in all those cases

>  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
> -	@$(SHELL_PATH) ./GIT-VERSION-GEN
> +	@{ GIT=./git$X ; type $$GIT >/dev/null 2>&1 ; } \

+	@{ GIT=./git$X ; type "$$GIT" >/dev/null 2>&1 ; } \

> +	    || { GIT=$(prefix)/bin/git$X ; type $$GIT >/dev/null 2>&1 ; }\

+	    || { GIT='$(prefix_SQ)/bin/git$X' ; type "$$GIT" >/dev/null 2>&1 ; }\

BTW, isn't `test -x "$GIT"` better in some way than `type "$GIT"`?

> +	    || GIT=git ; \
> +	    export GIT ; \
> +	    $(SHELL_PATH) ./GIT-VERSION-GEN

-- Hannes

[PATCH v2] To make GIT-VERSION-FILE, search for git more widely

[Matthias Andree]

Problem: when git is installed into /usr/local/bin, running 'sudo make
install' won't find git in $PATH (because sudo strips PATH for security
reasons).

This causes GIT-VERSION-FILE to not find git for git describe, and fall
back to DEF_VER.  This in turn causes rebuilds of major parts of the system.

Typical failing scenario:

make configure
./configure    # defaults to --prefix=/usr/local
make all doc
sudo make install install-doc install-html # REBUILDS HAPPEN HERE

This patch passes GIT through to the shell script, and additionally
looks for the current built version and in ${prefix}/bin.

Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 GIT-VERSION-GEN |    9 ++++-----
 Makefile        |    6 +++++-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 39cde78..d0dfef3 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -2,6 +2,7 @@
 
 GVF=GIT-VERSION-FILE
 DEF_VER=v1.6.3.GIT
+test -x "$GIT" || GIT=git
 
 LF='
 '
@@ -12,12 +13,12 @@ if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
 elif test -d .git -o -f .git &&
-	VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
+	VN=$($GIT describe --abbrev=4 HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
-		git update-index -q --refresh
-		test -z "$(git diff-index --name-only HEAD --)" ||
+		$GIT update-index -q --refresh
+		test -z "$($GIT diff-index --name-only HEAD --)" ||
 		VN="$VN-dirty" ;;
 	esac
 then
@@ -38,5 +39,3 @@ test "$VN" = "$VC" || {
 	echo >&2 "GIT_VERSION = $VN"
 	echo "GIT_VERSION = $VN" >$GVF
 }
-
-
diff --git a/Makefile b/Makefile
index 6e21643..d6be483 100644
--- a/Makefile
+++ b/Makefile
@@ -177,7 +177,11 @@ all::
 # away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@{ GIT=./git$X ; test -x "$$GIT" ; } \
+	    || { GIT=$(prefix)/bin/git$X ; test -x "$$GIT" ; }\
+	    || GIT=git ; \
+	    export GIT ; \
+	    $(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')
-- 
1.6.3.rc2.9.g97ad0