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
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.
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.
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.
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.
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
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
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
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