Test failures with python versions when building git 1.8.1

Test failures with python versions when building git 1.8.1

[Dan McGee]

A test case snuck in this release that assumes /usr/bin/python is
python2 and causes test failures. Unlike all other tests and code
depending on python, this one does not respect PYTHON_PATH, which we
explicitly set when building git on Arch Linux due to python2 vs
python3 differences.

-Dan

make[1]: Entering directory `/build/src/git-1.8.1/t'
rm -f -r test-results
*** prove ***

Test Summary Report
-------------------
t9020-remote-svn.sh                              (Wstat: 256 Tests: 6 Failed: 4)
  Failed tests:  1-2, 5-6
  Non-zero exit status: 1
Files=608, Tests=8772, 76 wallclock secs ( 4.07 usr  0.65 sys + 91.83
cusr 37.14 csys = 133.69 CPU)
Result: FAIL
make[1]: *** [prove] Error 1
make[1]: Leaving directory `/build/src/git-1.8.1/t'
make: *** [test] Error 2


$ contrib/svn-fe/svnrdump_sim.py
  File "contrib/svn-fe/svnrdump_sim.py", line 43
    print "usage: %s dump URL -rLOWER:UPPER"
                                           ^
SyntaxError: invalid syntax

diff --git a/contrib/svn-fe/svnrdump_sim.py b/contrib/svn-fe/svnrdump_sim.py
index 1cfac4a..7e6148d 100755
--- a/contrib/svn-fe/svnrdump_sim.py
+++ b/contrib/svn-fe/svnrdump_sim.py
@@ -40,7 +40,7 @@ def writedump(url, lower, upper):

 if __name__ == "__main__":
         if not (len(sys.argv) in (3, 4, 5)):
-                print "usage: %s dump URL -rLOWER:UPPER"
+                print("usage: %s dump URL -rLOWER:UPPER")
                 sys.exit(1)
         if not sys.argv[1] == 'dump': raise NotImplementedError('only
"dump" is suppported.')
         url = sys.argv[2]

Re: Test failures with python versions when building git 1.8.1

[Junio C Hamano]

Dan McGee <dan@archlinux.org> writes:

> A test case snuck in this release that assumes /usr/bin/python is
> python2 and causes test failures. Unlike all other tests and code
> depending on python, this one does not respect PYTHON_PATH, which we
> explicitly set when building git on Arch Linux due to python2 vs
> python3 differences.

I had an impression that you are not supposed to run our scripts
with python3 yet (no python scripts have been checked for python3
compatibility), even though some people have expressed interests in
doing so.

Eric?

Re: Test failures with python versions when building git 1.8.1

[Eric S. Raymond]

Junio C Hamano <gitster@pobox.com>:
> Dan McGee <dan@archlinux.org> writes:
> 
> > A test case snuck in this release that assumes /usr/bin/python is
> > python2 and causes test failures. Unlike all other tests and code
> > depending on python, this one does not respect PYTHON_PATH, which we
> > explicitly set when building git on Arch Linux due to python2 vs
> > python3 differences.
> 
> I had an impression that you are not supposed to run our scripts
> with python3 yet (no python scripts have been checked for python3
> compatibility), even though some people have expressed interests in
> doing so.
> 
> Eric?

Yeah, git's stuff is nowhere even *near* python3 ready.

I have it on my to-do list to run 2to3 on the in-tree scripts as a
diagnostic, but I haven't had time to do that yet...mainly because
cvsps/cvsimport blew up in my face when I poked at them.

Now I need to go beat parsecvs into shape and run both it and cvs2git
against the CVS torture tests I'm developing, so the 2to3 check won't
happen for a week or two at least. Sorry.

As in a general thing, I wouldn't advise worrying too much about python3
compatibility.  That version is not gaining adoption very fast, mainly
due to the rat's nest around plain strings vs. UTF-8 which can make
code conversion a serious pain in the ass.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Re: Test failures with python versions when building git 1.8.1

[Jeff King]

On Tue, Jan 01, 2013 at 09:19:13PM -0800, Junio C Hamano wrote:

> Dan McGee <dan@archlinux.org> writes:
> 
> > A test case snuck in this release that assumes /usr/bin/python is
> > python2 and causes test failures. Unlike all other tests and code
> > depending on python, this one does not respect PYTHON_PATH, which we
> > explicitly set when building git on Arch Linux due to python2 vs
> > python3 differences.
> 
> I had an impression that you are not supposed to run our scripts
> with python3 yet (no python scripts have been checked for python3
> compatibility), even though some people have expressed interests in
> doing so.
> 
> Eric?

Yeah, but the worrying thing to me is that we are not respecting
PYTHON_PATH. So even if Arch does everything right, it is getting
punished just for having python3 on the system at all.

I think we need to either:

  1. Build contrib/svn-fe/svnrdump_sim.py into svnrdump using our normal
     build procedure, which handles $PYTHON_PATH (right now we seem to
     just symlink[1] it into place as svnrdump during the test script
     run).

  2. Create svnrdump as a shell script in the test suite to do something
     like[2]:

       $PYTHON_PATH "$TEST_DIRECTORY"/../contrib/svn-fe/svnrdump_sim.py

-Peff

[1] This symlink is doubly wrong, because any use of symbolic links
    in the test scripts needs to depend on the SYMLINKS prereq, and this
    does not.

[2] In both the current code and what I showed above, the test scripts
    depend on things in contrib/. This is probably a bad idea in
    general, as the quality of what goes into contrib is not as closely
    watched (especially with respect to things like portability).
    Certainly I would not have known to look more carefully at a patch
    to contrib/svn-fe for breakage to the test suite.

    FWIW, we also have this situation with t9902 (bash completion),
    though the dependency is a little more obvious there. It might be
    worth promoting bash completion out of contrib (or alternatively,
    demoting t9902 into contrib/completion/, possibly with a feature to
    make it easier to run tests out of contrib).

Re: Test failures with python versions when building git 1.8.1

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [1] This symlink is doubly wrong, because any use of symbolic links
>     in the test scripts needs to depend on the SYMLINKS prereq, and this
>     does not.

Yeah, I think we have discussed this once already in

http://thread.gmane.org/gmane.comp.version-control.git/210688/focus=210714

> [2] In both the current code and what I showed above, the test scripts
>     depend on things in contrib/. This is probably a bad idea in
>     general, as the quality of what goes into contrib is not as closely
>     watched (especially with respect to things like portability).
>     Certainly I would not have known to look more carefully at a patch
>     to contrib/svn-fe for breakage to the test suite.

As long as such tests are made skippable with appropriate
prerequisites, I do not think it is bad to have their tests in t/; I
would say it is rather better than having them in contrib/ and leave
it not run by anybody, which happened to some of the stuff in
contrib/ already.

>     ... possibly with a feature to
>     make it easier to run tests out of contrib).

Yes, that certainly is a workable alternative.

Re: Test failures with python versions when building git 1.8.1

[Jeff King]

On Tue, Jan 01, 2013 at 11:18:46PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > [1] This symlink is doubly wrong, because any use of symbolic links
> >     in the test scripts needs to depend on the SYMLINKS prereq, and this
> >     does not.
> 
> Yeah, I think we have discussed this once already in
> 
> http://thread.gmane.org/gmane.comp.version-control.git/210688/focus=210714

Thanks for the pointer; it looks like nothing productive came of the
earlier discussion. To give a hat trick of failure to this line of code,
I notice that the existing code also does not properly put quotes around
$GIT_BUILD_DIR.

> > [2] In both the current code and what I showed above, the test scripts
> >     depend on things in contrib/. This is probably a bad idea in
> >     general, as the quality of what goes into contrib is not as closely
> >     watched (especially with respect to things like portability).
> >     Certainly I would not have known to look more carefully at a patch
> >     to contrib/svn-fe for breakage to the test suite.
> 
> As long as such tests are made skippable with appropriate
> prerequisites, I do not think it is bad to have their tests in t/; I
> would say it is rather better than having them in contrib/ and leave
> it not run by anybody, which happened to some of the stuff in
> contrib/ already.

Good point. While my sense of decorum wants to keep contrib totally
split out, from a practical point of view, it is better to have more
people run the tests and report failures than not.

Whether we end up doing something with contrib and tests or not, the
patch below gives a minimal fix in the meantime. Dan, does it fix your
problem?

-- >8 --
Subject: [PATCH] t9020: don't run python from $PATH

In t9020, we symlink in a python script from contrib to help
with the testing. However, we don't munge its #!-line, which
means we may run the wrong python (we want the one in
PYTHON_PATH). On top of this, we use a symlink without
checking the SYMLINKS prereq, and we fail to properly quote
GIT_BUILD_DIR, which may have spaces.

Instead of symlinking, let's just write a small script which
will feed the contrib script to PYTHON_PATH. To avoid
quoting issues, we just export the variables the script
needs to run.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9020-remote-svn.sh | 5 ++++-
 t/test-lib.sh         | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
index 4f2dfe0..416623b 100755
--- a/t/t9020-remote-svn.sh
+++ b/t/t9020-remote-svn.sh
@@ -14,7 +14,10 @@ export PATH="$HOME:$PATH"
 
 # We override svnrdump by placing a symlink to the svnrdump-emulator in .
 export PATH="$HOME:$PATH"
-ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
+export GIT_BUILD_DIR
+write_script svnrdump <<\EOF
+exec "$PYTHON_PATH" "$GIT_BUILD_DIR"/contrib/svn-fe/svnrdump_sim.py "$@"
+EOF
 
 init_git () {
 	rm -fr .git &&
diff --git a/t/test-lib.sh b/t/test-lib.sh
index f50f834..c17db19 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -45,7 +45,7 @@ fi
 fi
 
 . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
-export PERL_PATH SHELL_PATH
+export PERL_PATH SHELL_PATH PYTHON_PATH
 
 # if --tee was passed, write the output not only to the terminal, but
 # additionally to the file test-results/$BASENAME.out, too.
-- 
1.8.1.rc3.4.gf3a2f57

Re: Test failures with python versions when building git 1.8.1

[Joachim Schmitz]

Jeff King wrote:
> On Tue, Jan 01, 2013 at 11:18:46PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> [1] This symlink is doubly wrong, because any use of symbolic links
>>>     in the test scripts needs to depend on the SYMLINKS prereq, and
>>>     this does not.
>>
>> Yeah, I think we have discussed this once already in
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/210688/focus=210714
>
> Thanks for the pointer; it looks like nothing productive came of the
> earlier discussion. To give a hat trick of failure to this line of
> code, I notice that the existing code also does not properly put
> quotes around $GIT_BUILD_DIR.
>
>>> [2] In both the current code and what I showed above, the test
>>>     scripts depend on things in contrib/. This is probably a bad
>>>     idea in general, as the quality of what goes into contrib is
>>>     not as closely watched (especially with respect to things like
>>>     portability). Certainly I would not have known to look more
>>>     carefully at a patch to contrib/svn-fe for breakage to the test
>>> suite.
>>
>> As long as such tests are made skippable with appropriate
>> prerequisites, I do not think it is bad to have their tests in t/; I
>> would say it is rather better than having them in contrib/ and leave
>> it not run by anybody, which happened to some of the stuff in
>> contrib/ already.
>
> Good point. While my sense of decorum wants to keep contrib totally
> split out, from a practical point of view, it is better to have more
> people run the tests and report failures than not.
>
> Whether we end up doing something with contrib and tests or not, the
> patch below gives a minimal fix in the meantime. Dan, does it fix your
> problem?
>
> -- >8 --
> Subject: [PATCH] t9020: don't run python from $PATH
>
> In t9020, we symlink in a python script from contrib to help
> with the testing. However, we don't munge its #!-line, which
> means we may run the wrong python (we want the one in
> PYTHON_PATH). On top of this, we use a symlink without
> checking the SYMLINKS prereq, and we fail to properly quote
> GIT_BUILD_DIR, which may have spaces.
>
> Instead of symlinking, let's just write a small script which
> will feed the contrib script to PYTHON_PATH. To avoid
> quoting issues, we just export the variables the script
> needs to run.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/t9020-remote-svn.sh | 5 ++++-
> t/test-lib.sh         | 2 +-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
> index 4f2dfe0..416623b 100755
> --- a/t/t9020-remote-svn.sh
> +++ b/t/t9020-remote-svn.sh
> @@ -14,7 +14,10 @@ export PATH="$HOME:$PATH"
>
> # We override svnrdump by placing a symlink to the svnrdump-emulator
> in . export PATH="$HOME:$PATH"

With this patch that comment is no longer true.

> -ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
> +export GIT_BUILD_DIR
> +write_script svnrdump <<\EOF
> +exec "$PYTHON_PATH" "$GIT_BUILD_DIR"/contrib/svn-fe/svnrdump_sim.py
> "$@" +EOF
>
> init_git () {
>  rm -fr .git &&

Re: Test failures with python versions when building git 1.8.1

[Dan McGee]

On Wed, Jan 2, 2013 at 2:59 AM, Jeff King <peff@peff.net> wrote:
> On Tue, Jan 01, 2013 at 11:18:46PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > [1] This symlink is doubly wrong, because any use of symbolic links
>> >     in the test scripts needs to depend on the SYMLINKS prereq, and this
>> >     does not.
>>
>> Yeah, I think we have discussed this once already in
>>
>> http://thread.gmane.org/gmane.comp.version-control.git/210688/focus=210714
>
> Thanks for the pointer; it looks like nothing productive came of the
> earlier discussion. To give a hat trick of failure to this line of code,
> I notice that the existing code also does not properly put quotes around
> $GIT_BUILD_DIR.
>
>> > [2] In both the current code and what I showed above, the test scripts
>> >     depend on things in contrib/. This is probably a bad idea in
>> >     general, as the quality of what goes into contrib is not as closely
>> >     watched (especially with respect to things like portability).
>> >     Certainly I would not have known to look more carefully at a patch
>> >     to contrib/svn-fe for breakage to the test suite.
>>
>> As long as such tests are made skippable with appropriate
>> prerequisites, I do not think it is bad to have their tests in t/; I
>> would say it is rather better than having them in contrib/ and leave
>> it not run by anybody, which happened to some of the stuff in
>> contrib/ already.
>
> Good point. While my sense of decorum wants to keep contrib totally
> split out, from a practical point of view, it is better to have more
> people run the tests and report failures than not.
>
> Whether we end up doing something with contrib and tests or not, the
> patch below gives a minimal fix in the meantime. Dan, does it fix your
> problem?

This works great now, thanks! I ran it through our package build
scripts and all tests now pass as expected.

Signed-off-by: Dan McGee <dan@archlinux.org>

> -- >8 --
> Subject: [PATCH] t9020: don't run python from $PATH
>
> In t9020, we symlink in a python script from contrib to help
> with the testing. However, we don't munge its #!-line, which
> means we may run the wrong python (we want the one in
> PYTHON_PATH). On top of this, we use a symlink without
> checking the SYMLINKS prereq, and we fail to properly quote
> GIT_BUILD_DIR, which may have spaces.
>
> Instead of symlinking, let's just write a small script which
> will feed the contrib script to PYTHON_PATH. To avoid
> quoting issues, we just export the variables the script
> needs to run.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t9020-remote-svn.sh | 5 ++++-
>  t/test-lib.sh         | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
> index 4f2dfe0..416623b 100755
> --- a/t/t9020-remote-svn.sh
> +++ b/t/t9020-remote-svn.sh
> @@ -14,7 +14,10 @@ export PATH="$HOME:$PATH"
>
>  # We override svnrdump by placing a symlink to the svnrdump-emulator in .
>  export PATH="$HOME:$PATH"
> -ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
> +export GIT_BUILD_DIR
> +write_script svnrdump <<\EOF
> +exec "$PYTHON_PATH" "$GIT_BUILD_DIR"/contrib/svn-fe/svnrdump_sim.py "$@"
> +EOF
>
>  init_git () {
>         rm -fr .git &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f50f834..c17db19 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -45,7 +45,7 @@ fi
>  fi
>
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> -export PERL_PATH SHELL_PATH
> +export PERL_PATH SHELL_PATH PYTHON_PATH
>
>  # if --tee was passed, write the output not only to the terminal, but
>  # additionally to the file test-results/$BASENAME.out, too.
> --
> 1.8.1.rc3.4.gf3a2f57
>

Re: Test failures with python versions when building git 1.8.1

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Whether we end up doing something with contrib and tests or not, the
> patch below gives a minimal fix in the meantime.

Replacing the symbolic link with write_script that uses exported
variables looks like a familiar pattern.  I like it.

Oh, wait.  That pattern is of course familiar, because 5a02966
(t9020: use configured Python to run the test helper, 2012-12-18)
has been in 'next', and is planned for the first batch.



> -- >8 --
> Subject: [PATCH] t9020: don't run python from $PATH
>
> In t9020, we symlink in a python script from contrib to help
> with the testing. However, we don't munge its #!-line, which
> means we may run the wrong python (we want the one in
> PYTHON_PATH). On top of this, we use a symlink without
> checking the SYMLINKS prereq, and we fail to properly quote
> GIT_BUILD_DIR, which may have spaces.
>
> Instead of symlinking, let's just write a small script which
> will feed the contrib script to PYTHON_PATH. To avoid
> quoting issues, we just export the variables the script
> needs to run.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  t/t9020-remote-svn.sh | 5 ++++-
>  t/test-lib.sh         | 2 +-
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
> index 4f2dfe0..416623b 100755
> --- a/t/t9020-remote-svn.sh
> +++ b/t/t9020-remote-svn.sh
> @@ -14,7 +14,10 @@ export PATH="$HOME:$PATH"
>  
>  # We override svnrdump by placing a symlink to the svnrdump-emulator in .
>  export PATH="$HOME:$PATH"
> -ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
> +export GIT_BUILD_DIR
> +write_script svnrdump <<\EOF
> +exec "$PYTHON_PATH" "$GIT_BUILD_DIR"/contrib/svn-fe/svnrdump_sim.py "$@"
> +EOF
>  
>  init_git () {
>  	rm -fr .git &&
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index f50f834..c17db19 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -45,7 +45,7 @@ fi
>  fi
>  
>  . "$GIT_BUILD_DIR"/GIT-BUILD-OPTIONS
> -export PERL_PATH SHELL_PATH
> +export PERL_PATH SHELL_PATH PYTHON_PATH
>  
>  # if --tee was passed, write the output not only to the terminal, but
>  # additionally to the file test-results/$BASENAME.out, too.

Re: Test failures with python versions when building git 1.8.1

[Junio C Hamano]

Dan McGee <dan@archlinux.org> writes:

> This works great now, thanks! I ran it through our package build
> scripts and all tests now pass as expected.

If you have a chance, could you try tip of the 'next' branch without
this patch applied?  We had an equivalent patch cooking there for
some time by now.

Thanks.

Re: Test failures with python versions when building git 1.8.1

[Jeff King]

On Wed, Jan 02, 2013 at 08:34:42AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Whether we end up doing something with contrib and tests or not, the
> > patch below gives a minimal fix in the meantime.
> 
> Replacing the symbolic link with write_script that uses exported
> variables looks like a familiar pattern.  I like it.
> 
> Oh, wait.  That pattern is of course familiar, because 5a02966
> (t9020: use configured Python to run the test helper, 2012-12-18)
> has been in 'next', and is planned for the first batch.

Great minds think alike, I guess?

Would have been nice to mention that you had done a patch when you
pointed to the unproductive thread. :P But I think you can take the
similarity of our patches and commit messages as my endorsement of
5a02966. :)

-Peff

Re: Test failures with python versions when building git 1.8.1

[Dan McGee]

On Wed, Jan 2, 2013 at 10:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Dan McGee <dan@archlinux.org> writes:
>
>> This works great now, thanks! I ran it through our package build
>> scripts and all tests now pass as expected.
>
> If you have a chance, could you try tip of the 'next' branch without
> this patch applied?  We had an equivalent patch cooking there for
> some time by now.

Yeah, this worked great as well. Thanks again everyone.

-Dan