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

2013-05-19 Thread Junio C Hamano
David Aguilar dav...@gmail.com writes:

 On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote:

 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.

Ahh, so my nagging feeling was that remote-helpers could in theory
be updated to follow the PYTHON_PATH like the rest of the system and
matching these two in that direction is the better longer-term fix?

Ok, with that in mind, I still think the patch under discussion is
an immediate solution that is fine as-is.

I somehow thought that there were valid reasons that we could not do
so for some technical reason (e.g. the instalation of python used to
run hg and/or bzr via these remote helpers and the installation of
python we use may need to be different?).  As long as the right
longer-term direction is not we cannot fundamentally unify them,
then I am very happy.

I was worried that we might end up having to define random fine
gained knobs like PYTHON_FOR_HG_PATH to allow tuning these.

Thanks for a clarification.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-19 Thread Felipe Contreras
Junio C Hamano wrote:
 David Aguilar dav...@gmail.com writes:
 
  On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote:
 
  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.
 
 Ahh, so my nagging feeling was that remote-helpers could in theory
 be updated to follow the PYTHON_PATH like the rest of the system and
 matching these two in that direction is the better longer-term fix?

Indeed, and I already have patches for that. FTR, the Makefile does loop
through the main Makefile, which is what made implementing this so easy.

 I somehow thought that there were valid reasons that we could not do
 so for some technical reason (e.g. the instalation of python used to
 run hg and/or bzr via these remote helpers and the installation of
 python we use may need to be different?).  As long as the right
 longer-term direction is not we cannot fundamentally unify them,
 then I am very happy.

I didn't think of that, and it might actually be a problem: I don't think
Mercurial works with python 3, and Bazaar probably never will.

But I don't think the current situation of always using 'python' helps either
way.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-17 Thread David Aguilar
On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@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 tbo...@web.de
 Signed-off-by: Felipe Contreras felipe.contre...@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 majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

2013-05-17 Thread Felipe Contreras
On Fri, May 17, 2013 at 9:51 PM, David Aguilar dav...@gmail.com wrote:
 On Fri, May 17, 2013 at 7:12 PM, Junio C Hamano gits...@pobox.com wrote:
 Felipe Contreras felipe.contre...@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.

Yeap, I initially thought it would be tricky to implement in the
rather minimal Makefile, but it seems it wouldn't. Whoever, I still
find it useful that I don't have to run 'make' to test these, and I
think it's nice that people can download directly as the final name
('git-remote-hg'), and don't have to rename. And since these aren't
installed with 'make install' anyway, I don't see any big hurry.

Cheers.

-- 
Felipe Contreras
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html