Re: [PATCH 1/2] remote-helpers: tests: use python directly

[David Aguilar <davvid@gmail.com>]

On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> These remote helpers use 'env python', not PYTHON_PATH, so that's where
>> we should check for the extensions. Otherwise, if 'python' is not
>> PYTHON_PATH (e.g. /usr/bin/python: Makefile's default), there will be a
>> mismatch between the python libraries actually accessible to the remote
>> helpers.
>
> What I am reading here is that what the "helper" uses and what the
> "test" checks to see if it can use the "helper" were different; and
> this patch fixes that misalignment by testing what the "helper"
> actually uses.
>
> So it is a right thing to do in that sense.
>
> I however am having this nagging feeling that I may be missing
> something subtle here.  Comments from others are very much welcome.

Yes, this is correct.  Another way to skin this cat would be to do
search/replace in a Makefile to burn in the PYTHON_PATH similar to how
we do for the .sh scripts and other .py files in the main Makefile.
The remote helpers are in contrib/ so they do not go through the main
Makefile, which is the root cause.

Longer-term, it would be good to treat these uniformly, but this is no
worse for now.

> Will queue but the result will be on tomorrow's pushout.
>
> Thanks.
>
>> Suggested by: Torsten Bögershausen <tboegi@web.de>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  contrib/remote-helpers/test-bzr.sh       | 2 +-
>>  contrib/remote-helpers/test-hg-bidi.sh   | 2 +-
>>  contrib/remote-helpers/test-hg-hg-git.sh | 4 ++--
>>  contrib/remote-helpers/test-hg.sh        | 2 +-
>>  4 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/contrib/remote-helpers/test-bzr.sh b/contrib/remote-helpers/test-bzr.sh
>> index 5dfa070..2c89caa 100755
>> --- a/contrib/remote-helpers/test-bzr.sh
>> +++ b/contrib/remote-helpers/test-bzr.sh
>> @@ -12,7 +12,7 @@ if ! test_have_prereq PYTHON; then
>>       test_done
>>  fi
>>
>> -if ! "$PYTHON_PATH" -c 'import bzrlib'; then
>> +if ! python -c 'import bzrlib'; then
>>       skip_all='skipping remote-bzr tests; bzr not available'
>>       test_done
>>  fi
>> diff --git a/contrib/remote-helpers/test-hg-bidi.sh b/contrib/remote-helpers/test-hg-bidi.sh
>> index f569697..2c693d0 100755
>> --- a/contrib/remote-helpers/test-hg-bidi.sh
>> +++ b/contrib/remote-helpers/test-hg-bidi.sh
>> @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then
>>       test_done
>>  fi
>>
>> -if ! "$PYTHON_PATH" -c 'import mercurial'; then
>> +if ! python -c 'import mercurial'; then
>>       skip_all='skipping remote-hg tests; mercurial not available'
>>       test_done
>>  fi
>> diff --git a/contrib/remote-helpers/test-hg-hg-git.sh b/contrib/remote-helpers/test-hg-hg-git.sh
>> index 8440341..fbad2b9 100755
>> --- a/contrib/remote-helpers/test-hg-hg-git.sh
>> +++ b/contrib/remote-helpers/test-hg-hg-git.sh
>> @@ -15,12 +15,12 @@ if ! test_have_prereq PYTHON; then
>>       test_done
>>  fi
>>
>> -if ! "$PYTHON_PATH" -c 'import mercurial'; then
>> +if ! python -c 'import mercurial'; then
>>       skip_all='skipping remote-hg tests; mercurial not available'
>>       test_done
>>  fi
>>
>> -if ! "$PYTHON_PATH" -c 'import hggit'; then
>> +if ! python -c 'import hggit'; then
>>       skip_all='skipping remote-hg tests; hg-git not available'
>>       test_done
>>  fi
>> diff --git a/contrib/remote-helpers/test-hg.sh b/contrib/remote-helpers/test-hg.sh
>> index 8de2aa7..ce03fa3 100755
>> --- a/contrib/remote-helpers/test-hg.sh
>> +++ b/contrib/remote-helpers/test-hg.sh
>> @@ -15,7 +15,7 @@ if ! test_have_prereq PYTHON; then
>>       test_done
>>  fi
>>
>> -if ! "$PYTHON_PATH" -c 'import mercurial'; then
>> +if ! python -c 'import mercurial'; then
>>       skip_all='skipping remote-hg tests; mercurial not available'
>>       test_done
>>  fi
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
David