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

# HG changeset patch
# User Yuya Nishihara <yuya@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?

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

> # HG changeset patch
> # User Yuya Nishihara <yuya@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@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On Sun, 14 Aug 2016 09:52:19 -0700, Gregory Szorc wrote:
> On Sat, Aug 13, 2016 at 3:15 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > # HG changeset patch
> > # User Yuya Nishihara <yuya@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.

Yeah, I know that would be the most controversial part of this series.
Good news is you can bisect without "make clean" as long as you have
mercurial/osutil.so compiled by old revisions. .so appears to precede .py.

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

Another idea I came up with is to add a helper to load cffi functions into
pure. This works only if cffi functions don't depend on pure functions and
classes.

  # mercurial/pure/osutil.py
  ...
  globals().update(policy.importcffioverrides(u'osutil'))

What I want to address right now are:

 a) move cffi codes and compiled objects out of the root directory
 b) centralize the import rule of cffi, instead of try-except in pure modules

On Sun, Aug 14, 2016 at 6:12 PM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Sun, 14 Aug 2016 09:52:19 -0700, Gregory Szorc wrote:
> > On Sat, Aug 13, 2016 at 3:15 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> > > # HG changeset patch
> > > # User Yuya Nishihara <yuya@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.
>
> Yeah, I know that would be the most controversial part of this series.
> Good news is you can bisect without "make clean" as long as you have
> mercurial/osutil.so compiled by old revisions. .so appears to precede .py.
>
> > 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.
>
> Another idea I came up with is to add a helper to load cffi functions into
> pure. This works only if cffi functions don't depend on pure functions and
> classes.
>
>   # mercurial/pure/osutil.py
>   ...
>   globals().update(policy.importcffioverrides(u'osutil'))
>
> What I want to address right now are:
>
>  a) move cffi codes and compiled objects out of the root directory
>  b) centralize the import rule of cffi, instead of try-except in pure
> modules
>

I think that this series is a stop in the right direction towards making
the cffi integration nicer. We can always work on "unifying" the modules
later.