[PATCH 5 of 5 RFC] osutil: switch to placeholder module that imports cpy/pure selectively

[Gregory Szorc]

On Sat, Aug 13, 2016 at 3:15 AM, Yuya Nishihara <yuya at tcha.org> wrote:

> # HG changeset patch
> # User Yuya Nishihara <yuya at tcha.org>
> # Date 1470969317 -32400
> #      Fri Aug 12 11:35:17 2016 +0900
> # Node ID e1318b322922baf91a146d92b1ee21cc0e509c04
> # Parent  3d8b3e09190aaacc3c57ae37b265838df4accb1a
> osutil: switch to placeholder module that imports cpy/pure selectively
>
> You have to do "make clean" to test this change. Otherwise, the existing
> osutil.so would be loaded directly. Also, you need "make clean" to go back
> to previous revisions.
>
> Nit: should we move osutil.c to mercurial/cpy?
>

I like the spirit of this series to establish some more order around the
various module implementations.

I'm pretty sure mpm won't like the `make clean` requirement, as he likes
the ability to bisect without having to worry about build system foo.

I haven't been paying close attention to the cpy/pure/cffi discussions. I
know there is a common pattern in other Python projects of having modules
define the pure Python code inline then have code at the bottom of the
module/file that imports C extensions/libraries and overwrites the Python
bits. So e.g. we could move mercurial/pure/osutil.py to mercurial/osutil.py
then at the bottom of the file do something like:

  if modulepolicy != 'py':
      globals().update(policy.uimportvars(u'osutil'))

Or we could potentially inline the cffi bits without having to a) maintain
a separate file/module or b) abstract the import mechanism for modules with
C implementations. In other words, we could add C implementations to any
module without having to tell the module import mechanism about which
modules contain C implementations. We would likely still need this
policy.importvars() trick for C extensions, since C extensions need to live
in their own module. But at least we wouldn't have an extra module for cffi.


>
> diff --git a/contrib/check-py3-compat.py b/contrib/check-py3-compat.py
> --- a/contrib/check-py3-compat.py
> +++ b/contrib/check-py3-compat.py
> @@ -55,7 +55,9 @@ def check_compat_py3(f):
>      # out module paths for things not in a package can be confusing.
>      if f.startswith(('hgext/', 'mercurial/')) and not
> f.endswith('__init__.py'):
>          assert f.endswith('.py')
> -        name = f.replace('/', '.')[:-3].replace('.pure.', '.')
> +        name = f.replace('/', '.')[:-3]
> +        if not f.endswith('osutil.py'):
> +            name = name.replace('.pure.', '.')
>          with open(f, 'r') as fh:
>              try:
>                  imp.load_module(name, fh, '', ('py', 'r', imp.PY_SOURCE))
> diff --git a/contrib/import-checker.py b/contrib/import-checker.py
> --- a/contrib/import-checker.py
> +++ b/contrib/import-checker.py
> @@ -688,7 +688,8 @@ def main(argv):
>      used_imports = {}
>      any_errors = False
>      for source_path in argv[1:]:
> -        modname = dotted_name_of_path(source_path, trimpure=True)
> +        trimpure = not source_path.endswith('osutil.py')
> +        modname = dotted_name_of_path(source_path, trimpure=trimpure)
>          localmods[modname] = source_path
>      for localmodname, source_path in sorted(localmods.items()):
>          for src, modname, name, line in sources(source_path,
> localmodname):
> diff --git a/mercurial/__init__.py b/mercurial/__init__.py
> --- a/mercurial/__init__.py
> +++ b/mercurial/__init__.py
> @@ -27,7 +27,6 @@ modulepolicy = policy.policy
>      'mercurial.bdiff',
>      'mercurial.diffhelpers',
>      'mercurial.mpatch',
> -    'mercurial.osutil',
>      'mercurial.parsers',
>  ])
>
> diff --git a/mercurial/osutil.py b/mercurial/osutil.py
> new file mode 100644
> --- /dev/null
> +++ b/mercurial/osutil.py
> @@ -0,0 +1,11 @@
> +# osutil.py - native operating system services
> +#
> +# Copyright 2007 Matt Mackall and others
> +#
> +# This software may be used and distributed according to the terms of the
> +# GNU General Public License version 2 or any later version.
> +
> +from __future__ import absolute_import
> +
> +from . import policy
> +globals().update(policy.uimportvars(u'osutil'))
> diff --git a/mercurial/pure/osutil.py b/mercurial/pure/osutil.py
> --- a/mercurial/pure/osutil.py
> +++ b/mercurial/pure/osutil.py
> @@ -14,7 +14,7 @@ import socket
>  import stat as statmod
>  import sys
>
> -from . import policy
> +from .. import policy
>  modulepolicy = policy.policy
>  policynocffi = policy.policynocffi
>
> diff --git a/setup.py b/setup.py
> --- a/setup.py
> +++ b/setup.py
> @@ -577,7 +577,7 @@ extmodules = [
>                                      'mercurial/pathencode.c'],
>                include_dirs=common_include_dirs,
>                depends=common_depends),
> -    Extension('mercurial.osutil', ['mercurial/osutil.c'],
> +    Extension('mercurial.cpy.osutil', ['mercurial/osutil.c'],
>                include_dirs=common_include_dirs,
>                extra_link_args=osutil_ldflags,
>                depends=common_depends),
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20160814/68b90cba/attachment.html>