Patchwork [3,of,3] extensions: also search for extension in the 'hgext3rd' package

# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1457692208 0
#      Fri Mar 11 10:30:08 2016 +0000
# Node ID 394b75d27c45e4f9409e8185c47229990db72af8
# Parent  22f8428e07e42b110ee72cfe2725f714617686b5
# EXP-Topic hgext3rd
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 394b75d27c45
extensions: also search for extension in the 'hgext3rd' package

Mercurial extensions are not meant to be normal python package/module. Yet the
lack of an official location to install them means that a lot of them actually
install as root level python package, polluting the global Python package
namespace and risking collision with more legit packages. As we recently
discovered, core python actually support namespace package. A way for multiples
distinct "distribution" to share a common top level package without fear of
installation headache. (Namespace package allow submodule installed in different
location (of the 'sys.path') to be imported properly. So we are fine as long as
extension includes a proper 'hgext3rd.__init__.py' to declare the namespace
package.)

Therefore we introduce a 'hgext3rd' namespace packages and search for extension
in it. We'll then recommend third extensions to install themselves in it.
Strictly speaking we could just get third party extensions to install in 'hgext'
as it is also a namespace package. However, this would make the integration of
formerly third party extensions in the main distribution more complicated as the third
party install would overwrite the file from the main install. Moreover, having an
explicit split between third party and core extensions seems like a good idea.

The name 'hgext3rd' have been picked because it is short and seems explicit enough.
Other alternative I could think of where:

- hgextcontrib
- hgextother
- hgextunofficial

On Fri, 11 Mar 2016 11:47:05 +0000, Pierre-Yves David wrote:
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1457692208 0
> #      Fri Mar 11 10:30:08 2016 +0000
> # Node ID 394b75d27c45e4f9409e8185c47229990db72af8
> # Parent  22f8428e07e42b110ee72cfe2725f714617686b5
> # EXP-Topic hgext3rd
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 394b75d27c45
> extensions: also search for extension in the 'hgext3rd' package
> 
> Mercurial extensions are not meant to be normal python package/module. Yet the
> lack of an official location to install them means that a lot of them actually
> install as root level python package, polluting the global Python package
> namespace and risking collision with more legit packages. As we recently
> discovered, core python actually support namespace package. A way for multiples
> distinct "distribution" to share a common top level package without fear of
> installation headache. (Namespace package allow submodule installed in different
> location (of the 'sys.path') to be imported properly. So we are fine as long as
> extension includes a proper 'hgext3rd.__init__.py' to declare the namespace
> package.)
> 
> Therefore we introduce a 'hgext3rd' namespace packages and search for extension
> in it. We'll then recommend third extensions to install themselves in it.
> Strictly speaking we could just get third party extensions to install in 'hgext'
> as it is also a namespace package. However, this would make the integration of
> formerly third party extensions in the main distribution more complicated as the third
> party install would overwrite the file from the main install. Moreover, having an
> explicit split between third party and core extensions seems like a good idea.
> 
> The name 'hgext3rd' have been picked because it is short and seems explicit enough.
> Other alternative I could think of where:
> 
> - hgextcontrib
> - hgextother
> - hgextunofficial

This will need a bikeshedding?

I prefer "hgextcontrib" a bit. I might look for 'hgext2nd' if there is 3rd.

> On Mar 12, 2016, at 9:38 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> 
>> The name 'hgext3rd' have been picked because it is short and seems explicit enough.
>> Other alternative I could think of where:
>> 
>> - hgextcontrib
>> - hgextother
>> - hgextunofficial
> 
> This will need a bikeshedding?
> 
> I prefer "hgextcontrib" a bit. I might look for 'hgext2nd' if there is 3rd.

I liked the 3rd as in third party, but I don’t feel strongly. hgextother might be my second choice I guess.

AF

I'm not a fan of getting the import error repeatedly. I don't mind getting
multiple parse errors if a module exists (broken) in multiple places. But
seeing something say "couldn't find x in a; couldn't find x in b; couldn't
find x in c" seems silly. Also, I don't see the point of "couldn't find x
in a; found it in b".
On Mar 11, 2016 6:48 AM, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org>
wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1457692208 0
> #      Fri Mar 11 10:30:08 2016 +0000
> # Node ID 394b75d27c45e4f9409e8185c47229990db72af8
> # Parent  22f8428e07e42b110ee72cfe2725f714617686b5
> # EXP-Topic hgext3rd
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> 394b75d27c45
> extensions: also search for extension in the 'hgext3rd' package
>
> Mercurial extensions are not meant to be normal python package/module. Yet
> the
> lack of an official location to install them means that a lot of them
> actually
> install as root level python package, polluting the global Python package
> namespace and risking collision with more legit packages. As we recently
> discovered, core python actually support namespace package. A way for
> multiples
> distinct "distribution" to share a common top level package without fear of
> installation headache. (Namespace package allow submodule installed in
> different
> location (of the 'sys.path') to be imported properly. So we are fine as
> long as
> extension includes a proper 'hgext3rd.__init__.py' to declare the namespace
> package.)
>
> Therefore we introduce a 'hgext3rd' namespace packages and search for
> extension
> in it. We'll then recommend third extensions to install themselves in it.
> Strictly speaking we could just get third party extensions to install in
> 'hgext'
> as it is also a namespace package. However, this would make the
> integration of
> formerly third party extensions in the main distribution more complicated
> as the third
> party install would overwrite the file from the main install. Moreover,
> having an
> explicit split between third party and core extensions seems like a good
> idea.
>
> The name 'hgext3rd' have been picked because it is short and seems
> explicit enough.
> Other alternative I could think of where:
>
> - hgextcontrib
> - hgextother
> - hgextunofficial
>
> diff --git a/hgext/__init__.py b/hgext3rd/__init__.py
> copy from hgext/__init__.py
> copy to hgext3rd/__init__.py
> --- a/hgext/__init__.py
> +++ b/hgext3rd/__init__.py
> @@ -1,3 +1,4 @@
> +# name space package to host third party extensions
>  from __future__ import absolute_import
>  import pkgutil
>  __path__ = pkgutil.extend_path(__path__, __name__)
> diff --git a/mercurial/extensions.py b/mercurial/extensions.py
> --- a/mercurial/extensions.py
> +++ b/mercurial/extensions.py
> @@ -103,11 +103,15 @@ def load(ui, name, path):
>      else:
>          try:
>              mod = _importh("hgext.%s" % name)
>          except ImportError as err:
>              _reportimporterror(ui, err, "hgext.%s" % name, name)
> -            mod = _importh(name)
> +            try:
> +                mod = _importh("hgext3rd.%s" % name)
> +            except ImportError as err:
> +                _reportimporterror(ui, err, "hgext3rd.%s" % name, name)
> +                mod = _importh(name)
>
>      # Before we do anything with the extension, check against minimum
> stated
>      # compatibility. This gives extension authors a mechanism to have
> their
>      # extensions short circuit when loaded with a known incompatible
> version
>      # of Mercurial.
> diff --git a/setup.py b/setup.py
> --- a/setup.py
> +++ b/setup.py
> @@ -504,11 +504,11 @@ cmdclass = {'build': hgbuild,
>              }
>
>  packages = ['mercurial', 'mercurial.hgweb', 'mercurial.httpclient',
>              'mercurial.pure',
>              'hgext', 'hgext.convert', 'hgext.highlight', 'hgext.zeroconf',
> -            'hgext.largefiles']
> +            'hgext.largefiles', 'hgext3rd']
>
>  common_depends = ['mercurial/util.h']
>
>  osutil_ldflags = []
>
> diff --git a/tests/test-bad-extension.t b/tests/test-bad-extension.t
> --- a/tests/test-bad-extension.t
> +++ b/tests/test-bad-extension.t
> @@ -54,10 +54,13 @@ show traceback for ImportError of hgext.
>    Traceback (most recent call last):
>    Exception: bit bucket overflow
>    could not import hgext.badext2 (No module named *badext2): trying
> badext2 (glob)
>    Traceback (most recent call last):
>    ImportError: No module named *badext2 (glob)
> +  could not import hgext3rd.badext2 (No module named badext2): trying
> badext2
> +  Traceback (most recent call last):
> +  ImportError: No module named badext2
>    *** failed to import extension badext2: No module named badext2
>    Traceback (most recent call last):
>    ImportError: No module named badext2
>
>  confirm that there's no crash when an extension's documentation is bad
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On 03/13/2016 01:05 AM, Augie Fackler wrote:
>
>> On Mar 12, 2016, at 9:38 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>
>>> The name 'hgext3rd' have been picked because it is short and seems explicit enough.
>>> Other alternative I could think of where:
>>>
>>> - hgextcontrib
>>> - hgextother
>>> - hgextunofficial
>>
>> This will need a bikeshedding?

Hopefully not too much.

>>
>> I prefer "hgextcontrib" a bit. I might look for 'hgext2nd' if there is 3rd.
>
> I liked the 3rd as in third party, but I don’t feel strongly. hgextother might be my second choice I guess.

Maybe we could got for `hgext3rdparty` for clarity?

I prefer `hgextother` over `hgextcontrib` because we have an official 
contrib directory in the Mercurial repo (with some extension in it /o\)

On 03/13/2016 02:04 AM, timeless wrote:
> I'm not a fan of getting the import error repeatedly. I don't mind
> getting multiple parse errors if a module exists (broken) in multiple
> places. But seeing something say "couldn't find x in a; couldn't find x
> in b; couldn't find x in c" seems silly. Also, I don't see the point of
> "couldn't find x in a; found it in b".

I'm not sure what is the issue here, it is debug message that provide a 
clear depiction of the extensions search. I expect it to be useful.

It isn't useful.

If we want to have a --debug which lists the search path, that's fine.

But for normal "did load" cases, we shouldn't show failures to find
extensions in earlier parts of the path.

When I run `echo hi` with a PATH of:
/usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin

I don't get:
 -bash: /usr/local/bin/echo: command not found
hi

I just get:
hi

On Mon, Mar 14, 2016 at 9:00 AM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 03/13/2016 02:04 AM, timeless wrote:
>>
>> I'm not a fan of getting the import error repeatedly. I don't mind
>> getting multiple parse errors if a module exists (broken) in multiple
>> places. But seeing something say "couldn't find x in a; couldn't find x
>> in b; couldn't find x in c" seems silly. Also, I don't see the point of
>> "couldn't find x in a; found it in b".
>
>
> I'm not sure what is the issue here, it is debug message that provide a
> clear depiction of the extensions search. I expect it to be useful.
>
> --
> Pierre-Yves David

On 03/14/2016 01:41 PM, timeless wrote:
> It isn't useful.
>
> If we want to have a --debug which lists the search path, that's fine.
>
> But for normal "did load" cases, we shouldn't show failures to find
> extensions in earlier parts of the path.
>
> When I run `echo hi` with a PATH of:
> /usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin
>
> I don't get:
>   -bash: /usr/local/bin/echo: command not found
> hi
>
> I just get:
> hi

That's perpendicular to my patch, there is already such output without it.

On 3/14/2016 1:49 PM, Pierre-Yves David wrote:
>
>
> On 03/14/2016 01:41 PM, timeless wrote:
>> It isn't useful.
>>
>> If we want to have a --debug which lists the search path, that's fine.
>>
>> But for normal "did load" cases, we shouldn't show failures to find
>> extensions in earlier parts of the path.
>>
>> When I run `echo hi` with a PATH of:
>> /usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin
>>
>> I don't get:
>>   -bash: /usr/local/bin/echo: command not found
>> hi
>>
>> I just get:
>> hi
>
> That's perpendicular to my patch, there is already such output without 
> it.
>

Doesn't this patch make things strictly worse by adding another place to 
look?

I'd argue for fixing the output problem before making it more of a problem.

Question: If this output already exists, why don't we run into it at FB? 
Is it because we put everything in site-packaged today?

Also, FWIW, I prefer hgextcontrib myself, but I don't care that much.

On Mon, 14 Mar 2016 12:59:49 +0000, Pierre-Yves David wrote:
> On 03/13/2016 01:05 AM, Augie Fackler wrote:
> >> On Mar 12, 2016, at 9:38 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >>
> >>> The name 'hgext3rd' have been picked because it is short and seems explicit enough.
> >>> Other alternative I could think of where:
> >>>
> >>> - hgextcontrib
> >>> - hgextother
> >>> - hgextunofficial
> >>
> >> This will need a bikeshedding?
> 
> Hopefully not too much.
> 
> >>
> >> I prefer "hgextcontrib" a bit. I might look for 'hgext2nd' if there is 3rd.
> >
> > I liked the 3rd as in third party, but I don’t feel strongly. hgextother might be my second choice I guess.
> 
> Maybe we could got for `hgext3rdparty` for clarity?

Perhaps we'd want to avoid longer name?

I have no strong opinion, and I couldn't find a better alternative by scanning
the filesystem. I'm okay with "hgext3rd".

On 03/14/2016 07:24 AM, Ryan McElroy wrote:
> On 3/14/2016 1:49 PM, Pierre-Yves David wrote:
>>
>>
>> On 03/14/2016 01:41 PM, timeless wrote:
>>> It isn't useful.
>>>
>>> If we want to have a --debug which lists the search path, that's fine.
>>>
>>> But for normal "did load" cases, we shouldn't show failures to find
>>> extensions in earlier parts of the path.
>>>
>>> When I run `echo hi` with a PATH of:
>>> /usr/local/bin:/usr/bin:/usr/local/sbin:/usr/sbin
>>>
>>> I don't get:
>>>   -bash: /usr/local/bin/echo: command not found
>>> hi
>>>
>>> I just get:
>>> hi
>>
>> That's perpendicular to my patch, there is already such output without
>> it.
>>
>
> Doesn't this patch make things strictly worse by adding another place to
> look?

Not sure worth apply. This provide with useful debug information and 
I've never heard a complain about them until someone spotted them in 
this patch. So I'm not eager to consider them as a bad things and even 
less a blocker since it seems fairly out of scope here.

Thoughs?

On 03/14/2016 07:30 AM, Yuya Nishihara wrote:
> On Mon, 14 Mar 2016 12:59:49 +0000, Pierre-Yves David wrote:
>> On 03/13/2016 01:05 AM, Augie Fackler wrote:
>>>> On Mar 12, 2016, at 9:38 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>>>
>>>>> The name 'hgext3rd' have been picked because it is short and seems explicit enough.
>>>>> Other alternative I could think of where:
>>>>>
>>>>> - hgextcontrib
>>>>> - hgextother
>>>>> - hgextunofficial
>>>>
>>>> This will need a bikeshedding?
>>
>> Hopefully not too much.
>>
>>>>
>>>> I prefer "hgextcontrib" a bit. I might look for 'hgext2nd' if there is 3rd.
>>>
>>> I liked the 3rd as in third party, but I don’t feel strongly. hgextother might be my second choice I guess.
>>
>> Maybe we could got for `hgext3rdparty` for clarity?
>
> Perhaps we'd want to avoid longer name?
>
> I have no strong opinion, and I couldn't find a better alternative by scanning
> the filesystem. I'm okay with "hgext3rd".

Okay so we seems to have a consensus-ish for hgext3rd,

yuya:
  - hgext3rd: okay
  - hgextcontrib: initially preferred
durin42:
  - hgext3rd: like it (but don't feel strongly
  - hgextother: second choice
Ryan:
  - hgextcontrib: prefered
Pierre-Yves:
  - hgext3rd: prefered,
  - hgextother: second choice

What's the next step here. Shall I push this patch to the clowncopter?