Pull requests

#32 Merged
Repository
Deleted repository
Branch
default (f95bf0e841b4)
Repository
tarek/distribute distribute
Branch
default

Minimize impact of namespace package support for CPython 3.3.

Author
  1. Dirkjan Ochtman
Reviewers
Description

See changeset message for a more complete description. I should note that I also have a patch out to Mercurial to fix the relevant issue on their end.

Comments (12)

  1. Jason R. Coombs

    Dirkjan, thanks for the patch.

    I would prefer if the commit message were a little more descriptive. As written, the message indicates that it solves a regression, but it's not clear which systems are impacted, why they're impacted, when the regression occurred, or how the fix works to correct it. A good way to do this is to write a descriptive bug report for distribute (or Mercurial if appropriate), and then reference that bug report in the commit.

    Additionally, the patch doesn't provide any protection against the regression occurring again. It would be worthwhile to include at least a note indicating why that import technique (test-then-import vs. import-but-suppress-errors) is used to prevent a subsequent commit from re-writing the code back to the regressive state.

    Again thanks.

  2. Dirkjan Ochtman author

    The issue is fairly obscure. I'm happy to spell it out in a bug report if that helps, but it would be pretty hard to recreate it in a test suite.

    Mercurial has a module called demandimport (http://hg.intevation.org/mercurial/crew/file/tip/mercurial/demandimport.py) that makes importing lazy: the actual import is deferred until the module is actually accessed (by accessing an attribute). This is an optimization mostly to help Mercurial start up quickly (importing lots of modules can be pretty slow) and to break circular dependencies.

    This breaks the except ImportError pattern, because importing doesn't actually import; the ImportError will be thrown once the module is accessed. This is pretty bad, but Mercurial code generally only imports its own code and the stdlib; the stdlib doesn't use the pattern much, and Mercurial's code can generally be rewritten to access something in the module inside the try block so the import gets triggered.

    Now, I ran into this today because I call a Python hook from a Mercurial repository. However, that hook happens to be in namespace package, accessed via an egg-link file. Apparently, pkg_resources gets used to do something at import time for this. Changeset 77c8b15 added another instance of the except ImportError to do something that Python 3.3 needs, which made Mercurial report the ImportError incurred when trying to access _frozen_importlib.SourceFileLoader instead of running my hook.

    Now, I think the pattern Arfrever employed in 77c8b15 is pretty non-standard for this kind of thing; it seems neater (and more compact) to just test for the Python version we're running on. That happens to also solve my problem.

    Note that I also have a patch out with Mercurial to disable demandimport while importing the hook code, see http://selenic.com/pipermail/mercurial-devel/2012-December/046886.html.

  3. Erik Bray

    I don't know if it's a permissions issue or what, but bitbucket is not showing the full changeset for me then. All it shows is the change in pkg_resource.py. Not really your problem, but this is bizarre.

  4. Jason R. Coombs

    Erik, I don't see any comments in the diff or activity tab, but when I click on the 'commits' tab, I see the list of commits, and when I click on a particular commit, I see the full commit message such ash Dirkjan referenced.

  5. Erik Bray

    Nevermind; I'm tired--eyes hazy. I read "changeset message" as "changelog message" and expected to find some more details in the changelog.

  6. Dirkjan Ochtman author

    Not sure why I missed those. Thanks for fixing them up!

    I was thinking that you could also revert to doing something like

    try:
        import _frozen_importlib; _frozen_importlib.SourceFileLoader
    except ImportError:
        pass