Patchwork hgext: officially turn 'hgext' into a namespace package

login
register
mail settings
Submitter Pierre-Yves David
Date Feb. 27, 2016, 12:44 p.m.
Message ID <5f942a1ffe3974a1827b.1456577095@marginatus.alto.octopoid.net>
Download mbox | patch
Permalink /patch/13429/
State Accepted
Headers show

Comments

Pierre-Yves David - Feb. 27, 2016, 12:44 p.m.
# HG changeset patch
# User Pierre-Yves David <pierre-yves.david@fb.com>
# Date 1456574186 -3600
#      Sat Feb 27 12:56:26 2016 +0100
# Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
# Parent  41dcd754526612c43b9695df8851557c851828ef
# Available At http://hg.netv6.net/marmoute-wip/mercurial/
#              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 5f942a1ffe39
hgext: officially turn 'hgext' into a namespace package

Actually since Python 2.3, there is some way to turn top level package into "namespace
package" so that multiple subpackage installed in different part of the path can
still be imported transparently. This feature was previously thought (at least
by myself) to be only provided by some setuptool black magic.

Turning hgext into such namespace package allows third extensions to install
themselves inside the "hgext" namespace package to avoid polluting the global
python module namespace. They will now be able to do so without making it a pain
to use a Mercurial "installed" in a different way/location than these
extensions.

The only constrains is that the extension ship a 'hgext/__init__.py' containing
the same call to 'pkgutil.extend_path' and nothing else. This seems realistic.

The main question that remains is: should we introduce a dedicated namespace for
third party extension (hgext3rd?) to make a clearer distinction between what is
officially supported and what is not? If so, this will be introduced in a follow
up patch.
Augie Fackler - Feb. 27, 2016, 5:21 p.m.
> On Feb 27, 2016, at 7:44 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> 
> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1456574186 -3600
> #      Sat Feb 27 12:56:26 2016 +0100
> # Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
> # Parent  41dcd754526612c43b9695df8851557c851828ef
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 5f942a1ffe39
> hgext: officially turn 'hgext' into a namespace package

Well I’ll be. I had no idea namespace packages were a mainline thing.

By all means let’s do this for core and evolve, that seems like a good plan at least for that situation. I think talking about an extra namespace (hgext.external?) for non-core extensions might be nice, but let’s think about that later.

(Please get at least one other review on this before pushing, and maybe check for perf implications of namespace packages for imports, but in general I’m a Big Fan.)

> Actually since Python 2.3, there is some way to turn top level package into "namespace
> package" so that multiple subpackage installed in different part of the path can
> still be imported transparently. This feature was previously thought (at least
> by myself) to be only provided by some setuptool black magic.
> 
> Turning hgext into such namespace package allows third extensions to install
> themselves inside the "hgext" namespace package to avoid polluting the global
> python module namespace. They will now be able to do so without making it a pain
> to use a Mercurial "installed" in a different way/location than these
> extensions.
> 
> The only constrains is that the extension ship a 'hgext/__init__.py' containing
> the same call to 'pkgutil.extend_path' and nothing else. This seems realistic.
> 
> The main question that remains is: should we introduce a dedicated namespace for
> third party extension (hgext3rd?) to make a clearer distinction between what is
> officially supported and what is not? If so, this will be introduced in a follow
> up patch.
> 
> diff --git a/hgext/__init__.py b/hgext/__init__.py
> --- a/hgext/__init__.py
> +++ b/hgext/__init__.py
> @@ -1,1 +1,2 @@
> -# placeholder
> +from pkgutil import extend_path
> +__path__ = extend_path(__path__, __name__)
Gregory Szorc - Feb. 28, 2016, 1:45 a.m.
On Sat, Feb 27, 2016 at 4:44 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

> # HG changeset patch
> # User Pierre-Yves David <pierre-yves.david@fb.com>
> # Date 1456574186 -3600
> #      Sat Feb 27 12:56:26 2016 +0100
> # Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
> # Parent  41dcd754526612c43b9695df8851557c851828ef
> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r
> 5f942a1ffe39
> hgext: officially turn 'hgext' into a namespace package
>
> Actually since Python 2.3, there is some way to turn top level package
> into "namespace
> package" so that multiple subpackage installed in different part of the
> path can
> still be imported transparently. This feature was previously thought (at
> least
> by myself) to be only provided by some setuptool black magic.
>
> Turning hgext into such namespace package allows third extensions to
> install
> themselves inside the "hgext" namespace package to avoid polluting the
> global
> python module namespace. They will now be able to do so without making it
> a pain
> to use a Mercurial "installed" in a different way/location than these
> extensions.
>
> The only constrains is that the extension ship a 'hgext/__init__.py'
> containing
> the same call to 'pkgutil.extend_path' and nothing else. This seems
> realistic.
>
> The main question that remains is: should we introduce a dedicated
> namespace for
> third party extension (hgext3rd?) to make a clearer distinction between
> what is
> officially supported and what is not? If so, this will be introduced in a
> follow
> up patch.
>
> diff --git a/hgext/__init__.py b/hgext/__init__.py
> --- a/hgext/__init__.py
> +++ b/hgext/__init__.py
> @@ -1,1 +1,2 @@
> -# placeholder
> +from pkgutil import extend_path
>

This violates our import conventions (we only allow direct symbol imports
from mercurial.i18n and mercurial.node). Does test-module-imports.t not
complain? (A possibility since this file doesn't use absolute_import.)


> +__path__ = extend_path(__path__, __name__)
>

I'd test this on Windows with the zip importer before queuing since the zip
importer has historically caused problems.
Pierre-Yves David - March 7, 2016, 3:29 p.m.
On 02/27/2016 06:21 PM, Augie Fackler wrote:
>
>> On Feb 27, 2016, at 7:44 AM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
>>
>> # HG changeset patch
>> # User Pierre-Yves David <pierre-yves.david@fb.com>
>> # Date 1456574186 -3600
>> #      Sat Feb 27 12:56:26 2016 +0100
>> # Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
>> # Parent  41dcd754526612c43b9695df8851557c851828ef
>> # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>> #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/ -r 5f942a1ffe39
>> hgext: officially turn 'hgext' into a namespace package
>
> Well I’ll be. I had no idea namespace packages were a mainline thing.
>
> By all means let’s do this for core and evolve, that seems like a good plan at least for that situation. I think talking about an extra namespace (hgext.external?) for non-core extensions might be nice, but let’s think about that later.
>
> (Please get at least one other review on this before pushing, and maybe check for perf implications of namespace packages for imports, but in general I’m a Big Fan.)

Gentle ping for a second review.

I'm not too sure how to properly benchmark this, but some basic testing 
does not show any significant time change.

Cheers
Pierre-Yves David - March 9, 2016, 3:05 p.m.
On 02/28/2016 01:45 AM, Gregory Szorc wrote:
> On Sat, Feb 27, 2016 at 4:44 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>     # HG changeset patch
>     # User Pierre-Yves David <pierre-yves.david@fb.com
>     <mailto:pierre-yves.david@fb.com>>
>     # Date 1456574186 -3600
>     #      Sat Feb 27 12:56:26 2016 +0100
>     # Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
>     # Parent  41dcd754526612c43b9695df8851557c851828ef
>     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
>     #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/
>     -r 5f942a1ffe39
>     hgext: officially turn 'hgext' into a namespace package
>
>     Actually since Python 2.3, there is some way to turn top level
>     package into "namespace
>     package" so that multiple subpackage installed in different part of
>     the path can
>     still be imported transparently. This feature was previously thought
>     (at least
>     by myself) to be only provided by some setuptool black magic.
>
>     Turning hgext into such namespace package allows third extensions to
>     install
>     themselves inside the "hgext" namespace package to avoid polluting
>     the global
>     python module namespace. They will now be able to do so without
>     making it a pain
>     to use a Mercurial "installed" in a different way/location than these
>     extensions.
>
>     The only constrains is that the extension ship a 'hgext/__init__.py'
>     containing
>     the same call to 'pkgutil.extend_path' and nothing else. This seems
>     realistic.
>
>     The main question that remains is: should we introduce a dedicated
>     namespace for
>     third party extension (hgext3rd?) to make a clearer distinction
>     between what is
>     officially supported and what is not? If so, this will be introduced
>     in a follow
>     up patch.
>
>     diff --git a/hgext/__init__.py b/hgext/__init__.py
>     --- a/hgext/__init__.py
>     +++ b/hgext/__init__.py
>     @@ -1,1 +1,2 @@
>     -# placeholder
>     +from pkgutil import extend_path
>
>
> This violates our import conventions (we only allow direct symbol
> imports from mercurial.i18n and mercurial.node). Does
> test-module-imports.t not complain? (A possibility since this file
> doesn't use absolute_import.)

No complains, not sure why.
This is a special enough case that that I think we should stick to the 
canonical path (as in thi patch)

>     +__path__ = extend_path(__path__, __name__)
>
>
> I'd test this on Windows with the zip importer before queuing since the
> zip importer has historically caused problems.

I don't have access to the windows machine but since this comes from the 
standard library I'm going to assume this works fine with the other 
standard library crazyness.
Yuya Nishihara - March 11, 2016, 2:22 a.m.
On Wed, 9 Mar 2016 15:05:14 +0000, Pierre-Yves David wrote:
> On 02/28/2016 01:45 AM, Gregory Szorc wrote:
> > On Sat, Feb 27, 2016 at 4:44 AM, Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> > wrote:
> >
> >     # HG changeset patch
> >     # User Pierre-Yves David <pierre-yves.david@fb.com
> >     <mailto:pierre-yves.david@fb.com>>
> >     # Date 1456574186 -3600
> >     #      Sat Feb 27 12:56:26 2016 +0100
> >     # Node ID 5f942a1ffe3974a1827b02bb21dcb2d4494852bb
> >     # Parent  41dcd754526612c43b9695df8851557c851828ef
> >     # Available At http://hg.netv6.net/marmoute-wip/mercurial/
> >     #              hg pull http://hg.netv6.net/marmoute-wip/mercurial/
> >     -r 5f942a1ffe39
> >     hgext: officially turn 'hgext' into a namespace package
> >
> >     Actually since Python 2.3, there is some way to turn top level
> >     package into "namespace
> >     package" so that multiple subpackage installed in different part of
> >     the path can
> >     still be imported transparently. This feature was previously thought
> >     (at least
> >     by myself) to be only provided by some setuptool black magic.
> >
> >     Turning hgext into such namespace package allows third extensions to
> >     install
> >     themselves inside the "hgext" namespace package to avoid polluting
> >     the global
> >     python module namespace. They will now be able to do so without
> >     making it a pain
> >     to use a Mercurial "installed" in a different way/location than these
> >     extensions.
> >
> >     The only constrains is that the extension ship a 'hgext/__init__.py'
> >     containing
> >     the same call to 'pkgutil.extend_path' and nothing else. This seems
> >     realistic.
> >
> >     The main question that remains is: should we introduce a dedicated
> >     namespace for
> >     third party extension (hgext3rd?) to make a clearer distinction
> >     between what is
> >     officially supported and what is not? If so, this will be introduced
> >     in a follow
> >     up patch.
> >
> >     diff --git a/hgext/__init__.py b/hgext/__init__.py
> >     --- a/hgext/__init__.py
> >     +++ b/hgext/__init__.py
> >     @@ -1,1 +1,2 @@
> >     -# placeholder
> >     +from pkgutil import extend_path
> >
> >
> > This violates our import conventions (we only allow direct symbol
> > imports from mercurial.i18n and mercurial.node). Does
> > test-module-imports.t not complain? (A possibility since this file
> > doesn't use absolute_import.)
> 
> No complains, not sure why.

Actually there was. check-py3-compat.py fails to ignore an empty file that
only contains comments.

I've updated this patch to absolute_import and pushed to the clowncopter,
thanks.

> This is a special enough case that that I think we should stick to the
> canonical path (as in thi patch)
> 
> >     +__path__ = extend_path(__path__, __name__)
> >
> >
> > I'd test this on Windows with the zip importer before queuing since the
> > zip importer has historically caused problems.
> 
> I don't have access to the windows machine but since this comes from the 
> standard library I'm going to assume this works fine with the other 
> standard library crazyness.

This appears to be harmless with py2exe. hgext.__path__ is the same as before
this patch.

Patch

diff --git a/hgext/__init__.py b/hgext/__init__.py
--- a/hgext/__init__.py
+++ b/hgext/__init__.py
@@ -1,1 +1,2 @@ 
-# placeholder
+from pkgutil import extend_path
+__path__ = extend_path(__path__, __name__)