From: Felipe Contreras <felipe.contreras@gmail.com>
To: David Aguilar <davvid@gmail.com>
Cc: "Junio C Hamano" <gitster@pobox.com>,
"Git Mailing List" <git@vger.kernel.org>,
"Jeff King" <peff@peff.net>,
"Torsten Bögershausen" <tboegi@web.de>
Subject: Re: [PATCH 1/2] remote-helpers: tests: use python directly
Date: Fri, 17 May 2013 22:04:49 -0500 [thread overview]
Message-ID: <CAMP44s1w98hDbJx10PFDnS3YeYhFAHDyw=O2LdwCendLG6DVQQ@mail.gmail.com> (raw)
In-Reply-To: <CAJDDKr6Qnx5ddBn=6reNOY44CxaDgD254H7M3K2mb8bbd8jpmA@mail.gmail.com>
On Fri, May 17, 2013 at 9:51 PM, David Aguilar <davvid@gmail.com> wrote:
> 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.
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
next prev parent reply other threads:[~2013-05-18 3:05 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-17 21:10 [PATCH 0/2] remote-helpers: test fixes Felipe Contreras
2013-05-17 21:10 ` [PATCH 1/2] remote-helpers: tests: use python directly Felipe Contreras
2013-05-18 2:12 ` Junio C Hamano
2013-05-18 2:51 ` David Aguilar
2013-05-18 3:04 ` Felipe Contreras [this message]
2013-05-19 6:24 ` Junio C Hamano
2013-05-19 8:25 ` Felipe Contreras
2013-05-17 21:10 ` [PATCH 2/2] remote-hg: tests: fix hg merge Felipe Contreras
2013-05-20 22:47 ` [PATCH 0/2] remote-helpers: test fixes Junio C Hamano
2013-05-20 22:53 ` Felipe Contreras
2013-05-20 23:13 ` Junio C Hamano
2013-05-20 23:22 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMP44s1w98hDbJx10PFDnS3YeYhFAHDyw=O2LdwCendLG6DVQQ@mail.gmail.com' \
--to=felipe.contreras@gmail.com \
--cc=davvid@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=tboegi@web.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).