Patchwork demandimport: fix level passed to loader of sub-modules

login
register
mail settings
Submitter Yuya Nishihara
Date Nov. 2, 2015, 2:10 p.m.
Message ID <d779ddd9edd51273de27.1446473453@mimosa>
Download mbox | patch
Permalink /patch/11258/
State Superseded
Commit 78d05778907b935385e1529d2db16f4680eed484
Headers show

Comments

Yuya Nishihara - Nov. 2, 2015, 2:10 p.m.
# HG changeset patch
# User Yuya Nishihara <yuya@tcha.org>
# Date 1446380349 -32400
#      Sun Nov 01 21:19:09 2015 +0900
# Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
# Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
demandimport: fix level passed to loader of sub-modules

As the fromlist gives the names of sub-modules, they should be searched in
the parent directory of the package's __init__.py, which is level=1.

I got the following error by rewriting hgweb to use absolute_import, where
the "mercurial" package is referenced as ".." (level=2):

  ValueError: Attempted relative import beyond toplevel package

I know little about the import mechanism, but this change seems correct.
Before this patch, the following code did import the os module with no error:

  from mercurial import demandimport
  demandimport.enable()
  from mercurial import os
  print os.name
Gregory Szorc - Nov. 7, 2015, 3:49 p.m.
On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya@tcha.org>
> # Date 1446380349 -32400
> #      Sun Nov 01 21:19:09 2015 +0900
> # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
> # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
> demandimport: fix level passed to loader of sub-modules
>
> As the fromlist gives the names of sub-modules, they should be searched in
> the parent directory of the package's __init__.py, which is level=1.
>
> I got the following error by rewriting hgweb to use absolute_import, where
> the "mercurial" package is referenced as ".." (level=2):
>
>   ValueError: Attempted relative import beyond toplevel package
>
> I know little about the import mechanism, but this change seems correct.
> Before this patch, the following code did import the os module with no
> error:
>
>   from mercurial import demandimport
>   demandimport.enable()
>   from mercurial import os
>   print os.name
>
> diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
> --- a/mercurial/demandimport.py
> +++ b/mercurial/demandimport.py
> @@ -164,7 +164,7 @@ def _demandimport(name, globals=None, lo
>          # The name of the module the import statement is located in.
>          globalname = globals.get('__name__')
>
> -        def processfromitem(mod, attr, **kwargs):
> +        def processfromitem(mod, attr):
>              """Process an imported symbol in the import statement.
>
>              If the symbol doesn't exist in the parent module, it must be a
> @@ -172,7 +172,7 @@ def _demandimport(name, globals=None, lo
>              """
>              symbol = getattr(mod, attr, nothing)
>              if symbol is nothing:
> -                symbol = _demandmod(attr, mod.__dict__, locals, **kwargs)
> +                symbol = _demandmod(attr, mod.__dict__, locals, level=1)
>                  setattr(mod, attr, symbol)
>
>              # Record the importing module references this symbol so we can
> @@ -194,7 +194,7 @@ def _demandimport(name, globals=None, lo
>              mod = _hgextimport(_origimport, name, globals, locals,
> level=level)
>
>              for x in fromlist:
> -                processfromitem(mod, x, level=level)
> +                processfromitem(mod, x)
>
>              return mod
>

This patch looks good to me.
Pierre-Yves David - Nov. 7, 2015, 7:39 p.m.
On 11/07/2015 10:49 AM, Gregory Szorc wrote:
> On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org
> <mailto:yuya@tcha.org>> wrote:
>
>     # HG changeset patch
>     # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>>
>     # Date 1446380349 -32400
>     #      Sun Nov 01 21:19:09 2015 +0900
>     # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
>     # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
>     demandimport: fix level passed to loader of sub-modules

I've pushed that one to the clowncopter.

Thanks goes to Greg for the review.
Gregory Szorc - Nov. 8, 2015, 4:05 p.m.
On Sat, Nov 7, 2015 at 11:39 AM, Pierre-Yves David <
pierre-yves.david@ens-lyon.org> wrote:

>
>
> On 11/07/2015 10:49 AM, Gregory Szorc wrote:
>
>> On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org
>> <mailto:yuya@tcha.org>> wrote:
>>
>>     # HG changeset patch
>>     # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>>
>>     # Date 1446380349 -32400
>>     #      Sun Nov 01 21:19:09 2015 +0900
>>     # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
>>     # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
>>     demandimport: fix level passed to loader of sub-modules
>>
>
> I've pushed that one to the clowncopter.
>
> Thanks goes to Greg for the review.


Come to think of it, this patch should probably be for stable because
without it, "from ..X import Y" will be broken. This could prevent 3rd
party extensions which load 3rd party Python packages from working
correctly. I /think/ this is a regression in 3.6 from my demandimport
refactoring.
Pierre-Yves David - Nov. 8, 2015, 5:04 p.m.
On 11/08/2015 11:05 AM, Gregory Szorc wrote:
> On Sat, Nov 7, 2015 at 11:39 AM, Pierre-Yves David
> <pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> wrote:
>
>
>
>     On 11/07/2015 10:49 AM, Gregory Szorc wrote:
>
>         On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org
>         <mailto:yuya@tcha.org>
>         <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>> wrote:
>
>              # HG changeset patch
>              # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>
>         <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>>
>              # Date 1446380349 -32400
>              #      Sun Nov 01 21:19:09 2015 +0900
>              # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
>              # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
>              demandimport: fix level passed to loader of sub-modules
>
>
>     I've pushed that one to the clowncopter.
>
>     Thanks goes to Greg for the review.
>
>
> Come to think of it, this patch should probably be for stable because
> without it, "from ..X import Y" will be broken. This could prevent 3rd
> party extensions which load 3rd party Python packages from working
> correctly. I /think/ this is a regression in 3.6 from my demandimport
> refactoring.

I've move the fix to stable. Thanks for your input.
Danek Duvall - Feb. 4, 2016, 9:34 p.m.
Pierre-Yves David wrote:

> On 11/08/2015 11:05 AM, Gregory Szorc wrote:
> >On Sat, Nov 7, 2015 at 11:39 AM, Pierre-Yves David
> ><pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> >wrote:
> >
> >
> >
> >    On 11/07/2015 10:49 AM, Gregory Szorc wrote:
> >
> >        On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org
> >        <mailto:yuya@tcha.org>
> >        <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>> wrote:
> >
> >             # HG changeset patch
> >             # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>
> >        <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>>
> >             # Date 1446380349 -32400
> >             #      Sun Nov 01 21:19:09 2015 +0900
> >             # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
> >             # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
> >             demandimport: fix level passed to loader of sub-modules
> >
> >
> >    I've pushed that one to the clowncopter.
> >
> >    Thanks goes to Greg for the review.
> >
> >
> >Come to think of it, this patch should probably be for stable because
> >without it, "from ..X import Y" will be broken. This could prevent 3rd
> >party extensions which load 3rd party Python packages from working
> >correctly. I /think/ this is a regression in 3.6 from my demandimport
> >refactoring.
> 
> I've move the fix to stable. Thanks for your input.

My apologies for the late testing; I was running the internal testsuite,
but hadn't tried yet to update our internal extension, which uses
SQLAlchemy, to work with the latest bits.  With 3.7.1:

    $ python
    >>> from mercurial import demandimport
    >>> demandimport.enable()
    >>> from sqlalchemy import create_engine
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 141, in _demandimport
	return _hgextimport(_import, name, globals, locals, fromlist, level)
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
	return importfunc(name, globals, *args, **kwargs)
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/__init__.py", line 9, in <module>
	from .sql import (
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 192, in _demandimport
	fromlist, level)
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
	return importfunc(name, globals, *args, **kwargs)
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/__init__.py", line 91, in <module>
	__go(locals())
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/__init__.py", line 89, in __go
	from . import naming
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 141, in _demandimport
	return _hgextimport(_import, name, globals, locals, fromlist, level)
      File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
	return importfunc(name, globals, *args, **kwargs)
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/naming.py", line 129, in <module>
	@event.listens_for(Index, "after_parent_attach")
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 94, in decorate
	listen(target, identifier, fn, *args, **kw)
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 63, in listen
	_event_key(target, identifier, fn).listen(*args, **kw)
      File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 28, in _event_key
	(identifier, target))
    sqlalchemy.exc.InvalidRequestError: No such event 'after_parent_attach' for target '<class 'sqlalchemy.sql.schema.Index'>'

Backing out this changeset (78d05778907b) fixes this particular problem.

I tried adding

    >>> demandimport.ignore.append("sqlalchemy")

before the import, but it doesn't seem to make any difference.  I also
tried "sqlalchemy.sql.naming" as suggested by the stack trace, and
"sqlalchemy.sql" for good measure, but none of that helped.

SQLAlchemy is version 0.9.8.

What's worrying me right now is that while I've been testing and updating
our extension for a few days now, I just started seeing this last night.  I
noticed it for the first time when running a command that isn't part of the
testsuite, and is where the sqlalchemy bits are imported.  It seemed like
something to track down, so I started poking.

My first thought was to add

    from mercurial import demandimport
    demandimport.ignore.extend(["sqlalchemy"])

to the top of the extension, but that just caused the error to happen every
single time, and the extension wouldn't load.  That in turn caused me to
try the import in a program of its own, and thus the "simple" testcase
above.

Any ideas?  (Other than adding the command to the testsuite and running it
sooner: already on the schedule.)

Thanks,
Danek
Augie Fackler - Feb. 5, 2016, 3:42 p.m.
On Thu, Feb 04, 2016 at 01:34:27PM -0800, Danek Duvall wrote:
> Pierre-Yves David wrote:
>
> > On 11/08/2015 11:05 AM, Gregory Szorc wrote:
> > >On Sat, Nov 7, 2015 at 11:39 AM, Pierre-Yves David
> > ><pierre-yves.david@ens-lyon.org <mailto:pierre-yves.david@ens-lyon.org>>
> > >wrote:
> > >
> > >
> > >
> > >    On 11/07/2015 10:49 AM, Gregory Szorc wrote:
> > >
> > >        On Mon, Nov 2, 2015 at 6:10 AM, Yuya Nishihara <yuya@tcha.org
> > >        <mailto:yuya@tcha.org>
> > >        <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>> wrote:
> > >
> > >             # HG changeset patch
> > >             # User Yuya Nishihara <yuya@tcha.org <mailto:yuya@tcha.org>
> > >        <mailto:yuya@tcha.org <mailto:yuya@tcha.org>>>
> > >             # Date 1446380349 -32400
> > >             #      Sun Nov 01 21:19:09 2015 +0900
> > >             # Node ID d779ddd9edd51273de2754748dfcc0db1e1e0f72
> > >             # Parent  58b7f3e93bbab749ab16c09df12aae5ba7880708
> > >             demandimport: fix level passed to loader of sub-modules
> > >
> > >
> > >    I've pushed that one to the clowncopter.
> > >
> > >    Thanks goes to Greg for the review.
> > >
> > >
> > >Come to think of it, this patch should probably be for stable because
> > >without it, "from ..X import Y" will be broken. This could prevent 3rd
> > >party extensions which load 3rd party Python packages from working
> > >correctly. I /think/ this is a regression in 3.6 from my demandimport
> > >refactoring.
> >
> > I've move the fix to stable. Thanks for your input.
>
> My apologies for the late testing; I was running the internal testsuite,
> but hadn't tried yet to update our internal extension, which uses
> SQLAlchemy, to work with the latest bits.  With 3.7.1:
>
>     $ python
>     >>> from mercurial import demandimport
>     >>> demandimport.enable()
>     >>> from sqlalchemy import create_engine
>     Traceback (most recent call last):
>       File "<stdin>", line 1, in <module>
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 141, in _demandimport
>       return _hgextimport(_import, name, globals, locals, fromlist, level)
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
>       return importfunc(name, globals, *args, **kwargs)
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/__init__.py", line 9, in <module>
>       from .sql import (
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 192, in _demandimport
>       fromlist, level)
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
>       return importfunc(name, globals, *args, **kwargs)
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/__init__.py", line 91, in <module>
>       __go(locals())
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/__init__.py", line 89, in __go
>       from . import naming
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 141, in _demandimport
>       return _hgextimport(_import, name, globals, locals, fromlist, level)
>       File "/usr/lib/python2.7/vendor-packages/mercurial/demandimport.py", line 53, in _hgextimport
>       return importfunc(name, globals, *args, **kwargs)
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/sql/naming.py", line 129, in <module>
>       @event.listens_for(Index, "after_parent_attach")
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 94, in decorate
>       listen(target, identifier, fn, *args, **kw)
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 63, in listen
>       _event_key(target, identifier, fn).listen(*args, **kw)
>       File "/usr/lib/python2.7/vendor-packages/sqlalchemy/event/api.py", line 28, in _event_key
>       (identifier, target))
>     sqlalchemy.exc.InvalidRequestError: No such event 'after_parent_attach' for target '<class 'sqlalchemy.sql.schema.Index'>'
>
> Backing out this changeset (78d05778907b) fixes this particular problem.
>
> I tried adding
>
>     >>> demandimport.ignore.append("sqlalchemy")
>
> before the import, but it doesn't seem to make any difference.  I also
> tried "sqlalchemy.sql.naming" as suggested by the stack trace, and
> "sqlalchemy.sql" for good measure, but none of that helped.
>
> SQLAlchemy is version 0.9.8.
>
> What's worrying me right now is that while I've been testing and updating
> our extension for a few days now, I just started seeing this last night.  I
> noticed it for the first time when running a command that isn't part of the
> testsuite, and is where the sqlalchemy bits are imported.  It seemed like
> something to track down, so I started poking.
>
> My first thought was to add
>
>     from mercurial import demandimport
>     demandimport.ignore.extend(["sqlalchemy"])
>
> to the top of the extension, but that just caused the error to happen every
> single time, and the extension wouldn't load.  That in turn caused me to
> try the import in a program of its own, and thus the "simple" testcase
> above.
>
> Any ideas?  (Other than adding the command to the testsuite and running it
> sooner: already on the schedule.)

Please file a bug about this, and mark it as a regression. Note that
if you would have reported this a week ago (before the release), you
might have been eligible for a prize.

>
> Thanks,
> Danek
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Danek Duvall - Feb. 5, 2016, 4:38 p.m.
Augie Fackler wrote:

> Please file a bug about this, and mark it as a regression.

Bug 5805.  https://bz.mercurial-scm.org/show_bug.cgi?id=5085

> Note that if you would have reported this a week ago (before the
> release), you might have been eligible for a prize.

Yeah, I know.  I'm working on the infrastructure that'll let me test both
mercurial and the extension on a regular basis, without having to spend
much time on it.  Just didn't get to it soon enough.

I'll just have to deal with Matt being annoyed with me for a while.

Thanks,
Danek
Pierre-Yves David - Feb. 11, 2016, 9:21 p.m.
On 02/05/2016 04:38 PM, Danek Duvall wrote:
> Augie Fackler wrote:
>
>> Please file a bug about this, and mark it as a regression.
>
> Bug 5805.  https://bz.mercurial-scm.org/show_bug.cgi?id=5085
>
>> Note that if you would have reported this a week ago (before the
>> release), you might have been eligible for a prize.
>
> Yeah, I know.  I'm working on the infrastructure that'll let me test both
> mercurial and the extension on a regular basis, without having to spend
> much time on it.  Just didn't get to it soon enough.
>
> I'll just have to deal with Matt being annoyed with me for a while.

Matt have most probably passed the point where he is still getting 
annoyed by this ;-)

It might still make him sad however (but I'm not him)

Cheers,

Patch

diff --git a/mercurial/demandimport.py b/mercurial/demandimport.py
--- a/mercurial/demandimport.py
+++ b/mercurial/demandimport.py
@@ -164,7 +164,7 @@  def _demandimport(name, globals=None, lo
         # The name of the module the import statement is located in.
         globalname = globals.get('__name__')
 
-        def processfromitem(mod, attr, **kwargs):
+        def processfromitem(mod, attr):
             """Process an imported symbol in the import statement.
 
             If the symbol doesn't exist in the parent module, it must be a
@@ -172,7 +172,7 @@  def _demandimport(name, globals=None, lo
             """
             symbol = getattr(mod, attr, nothing)
             if symbol is nothing:
-                symbol = _demandmod(attr, mod.__dict__, locals, **kwargs)
+                symbol = _demandmod(attr, mod.__dict__, locals, level=1)
                 setattr(mod, attr, symbol)
 
             # Record the importing module references this symbol so we can
@@ -194,7 +194,7 @@  def _demandimport(name, globals=None, lo
             mod = _hgextimport(_origimport, name, globals, locals, level=level)
 
             for x in fromlist:
-                processfromitem(mod, x, level=level)
+                processfromitem(mod, x)
 
             return mod