Re: [PATCH 1/2] remote-helpers: tests: use python directly
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
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
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
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