Patchwork [5,of,6,import-refactor,V2] mercurial: be more strict about loading dual implemented modules

login
register
mail settings
Submitter Gregory Szorc
Date Nov. 25, 2015, 7:31 a.m.
Message ID <45043395515abca0dc72.1448436709@ubuntu-main>
Download mbox | patch
Permalink /patch/11655/
State Superseded
Commit a40c84defd76412445ecb4c29e729bd510584bb9
Delegated to: Yuya Nishihara
Headers show

Comments

Gregory Szorc - Nov. 25, 2015, 7:31 a.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1448434204 28800
#      Tue Nov 24 22:50:04 2015 -0800
# Node ID 45043395515abca0dc7290128b16752258be868b
# Parent  f6f18cd052e20dc517a4bce358e3d633897bc584
mercurial: be more strict about loading dual implemented modules

With this change in place, we should have slightly stronger guarantees
about how modules with both Python and C implementations are loaded.
Before, our module loader's default policy looked under both mercurial/*
and mercurial/pure/* and imported whatever it found, C or pure. The fact
it looked in both locations by default was a temporary regression from
the beginning of this series.

This patch does 2 things:

1) Changes the default module load policy to only load C modules
2) Verifies that files loaded from mercurial/* are actually C modules

This 2nd behavior change makes our new module loading mechanism
stricter than from before this series. Before, it was possible to load
a .py-based module from mercurial/*. This could happen if an old
installation orphaned a file and then somehow didn't install the C
version for the new install. We now detect this odd configuration
and fall back to loading the pure Python module, assuming it is
allowed. In the case of a busted installation, we fail fast. While
we could fall back, we explicitly decide not to do this because
we don't want people accidentally not running the C modules and having
slow performance as a result.

Patch

diff --git a/mercurial/__init__.py b/mercurial/__init__.py
--- a/mercurial/__init__.py
+++ b/mercurial/__init__.py
@@ -15,18 +15,17 @@  import sys
 #
 #    c - require C extensions
 #    allow - allow pure Python implementation when C loading fails
 #    py - only load pure Python modules
 modulepolicy = '@MODULELOADPOLICY@'
 
 # By default, require the C extensions for performance reasons.
 if modulepolicy == '@' 'MODULELOADPOLICY' '@':
-    # TODO change to 'c' once installer is changed.
-    modulepolicy = 'allow'
+    modulepolicy = 'c'
 
 # Environment variable can always force settings.
 modulepolicy = os.environ.get('HGMODULEPOLICY', modulepolicy)
 
 # Modules that have both Python and C implementations. See also the
 # set of .py files under mercurial/pure/.
 _dualmodules = set([
     'mercurial.base85',
@@ -72,21 +71,19 @@  class hgimporter(object):
             # for some installations to have .py files under mercurial/*.
             # Loading Python modules when we expected C versions could result
             # in a) poor performance b) loading a version from a previous
             # Mercurial version, potentially leading to incompatibility. Either
             # scenario is bad. So we verify that modules loaded from
             # mercurial/* are C extensions. If the current policy allows the
             # loading of .py modules, the module will be re-imported from
             # mercurial/pure/* below.
-            # TODO uncomment once setup.py is updated to actually install
-            # into mercurial/pure.
-            #if modinfo[2][2] != imp.C_EXTENSION:
-            #    raise ImportError('.py version of %s found where C '
-            #                      'version should exist' % name)
+            if modinfo[2][2] != imp.C_EXTENSION:
+                raise ImportError('.py version of %s found where C '
+                                  'version should exist' % name)
 
         except ImportError:
             if modulepolicy == 'c':
                 raise
 
             # Could not load the C extension and pure Python is allowed. So
             # try to load them.
             from . import pure