Re: [Buildroot] [PATCH v2 1/1] utils/scanpypi: refactor setuptools handling to not use imp

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello James,

On Thu, 30 Nov 2023 14:21:53 -0700
James Hilliard <james.hilliard1@gmail.com> wrote:

> The imp module is deprecated as of python verison 3.12.
> 
> Refactor setuptools handling to remove monkeypatching hack and
> instead do pep517 metadata generation and dependency resolution.

How does it work without monkeypatching? Why was monkeypatching needed
before and no longer needed now?

> Invert setuptools/pyproject fallback ordering so that we try
> parsing pyproject.toml files first.

What is the motivation for this particular change (inverting the order
between setuptools and pyproject)?

Also, this patch should be split into several ones, as you're really
doing several independent things, as far as I can see.

> diff --git a/utils/scanpypi b/utils/scanpypi
> index 021c99a172..9de1b5d402 100755
> --- a/utils/scanpypi
> +++ b/utils/scanpypi
> @@ -18,8 +18,8 @@ import hashlib
>  import re
>  import textwrap
>  import tempfile
> -import imp
> -from functools import wraps
> +import importlib
> +from setuptools.build_meta import prepare_metadata_for_build_wheel

This is part of the "don't use imp / remove monkeypatching" thing if I
understand correctly.

>  import six.moves.urllib.request
>  import six.moves.urllib.error
>  import six.moves.urllib.parse
> @@ -93,32 +93,6 @@ def toml_load(f):
>          raise ex
>  
>  
> -def setup_decorator(func, method):
> -    """
> -    Decorator for distutils.core.setup and setuptools.setup.
> -    Puts the arguments with which setup is called as a dict
> -    Add key 'method' which should be either 'setuptools' or 'distutils'.
> -
> -    Keyword arguments:
> -    func -- either setuptools.setup or distutils.core.setup
> -    method -- either 'setuptools' or 'distutils'
> -    """
> -
> -    @wraps(func)
> -    def closure(*args, **kwargs):
> -        # Any python packages calls its setup function to be installed.
> -        # Argument 'name' of this setup function is the package's name
> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
> -    return closure
> -
> -# monkey patch
> -import setuptools  # noqa E402
> -setuptools.setup = setup_decorator(setuptools.setup, 'setuptools')
> -import distutils   # noqa E402
> -distutils.core.setup = setup_decorator(setuptools.setup, 'distutils')

This is part of the "don't use imp / remove monkeypatching" thing if I
understand correctly.

>  def find_file_upper_case(filenames, path='./'):
>      """
>      List generator:
> @@ -345,26 +319,18 @@ class BuildrootPackage():
>          """
>          current_dir = os.getcwd()
>          os.chdir(self.tmp_extract)
> -        sys.path.insert(0, self.tmp_extract)

Not sure to what this is related? The monkeypatching as well? The use
of imp?

>          try:
> -            s_file, s_path, s_desc = imp.find_module('setup', [self.tmp_extract])
> -            imp.load_module('__main__', s_file, s_path, s_desc)
> -            if self.metadata_name in self.setup_args:
> -                pass
> -            elif self.metadata_name.replace('_', '-') in self.setup_args:
> -                self.metadata_name = self.metadata_name.replace('_', '-')
> -            elif self.metadata_name.replace('-', '_') in self.setup_args:
> -                self.metadata_name = self.metadata_name.replace('-', '_')
> +            metadata = prepare_metadata_for_build_wheel(self.tmp_extract)
>              try:
> -                self.setup_metadata = self.setup_args[self.metadata_name]
> -            except KeyError:
> -                # This means setup was not called
> -                print('ERROR: Could not determine package metadata for {pkg}.\n'
> -                      .format(pkg=self.real_name))
> -                raise
> +                dist = importlib.metadata.Distribution.at(metadata)
> +                self.metadata_name = dist.name
> +                self.setup_metadata = {'method': 'setuptools'}
> +                if dist.requires:
> +                    self.setup_metadata['install_requires'] = dist.requires
> +            finally:
> +                shutil.rmtree(metadata)

This is part of the "don't use imp / remove monkeypatching" thing if I
understand correctly.

>          finally:
>              os.chdir(current_dir)
> -            sys.path.remove(self.tmp_extract)
>  
>      def load_pyproject(self):
>          """
> @@ -372,7 +338,6 @@ class BuildrootPackage():
>          """
>          current_dir = os.getcwd()
>          os.chdir(self.tmp_extract)
> -        sys.path.insert(0, self.tmp_extract)
>          try:
>              pyproject_data = toml_load('pyproject.toml')
>              try:
> @@ -380,20 +345,25 @@ class BuildrootPackage():
>                  self.metadata_name = self.setup_metadata.get('name', self.real_name)
>                  build_system = pyproject_data.get('build-system', {})
>                  build_backend = build_system.get('build-backend', None)
> -                if build_backend and build_backend == 'flit_core.buildapi':
> -                    self.setup_metadata['method'] = 'flit'
> +                if build_backend:
> +                    if build_backend == 'flit_core.buildapi':
> +                        self.setup_metadata['method'] = 'flit'
> +                    elif build_backend == 'setuptools.build_meta':
> +                        raise Exception("handle setuptools")
> +                    else:
> +                        self.setup_metadata['method'] = 'unknown'

This seems more related to PEP517 handling improvements?

>                  elif build_system.get('backend-path', None):
>                      self.setup_metadata['method'] = 'pep517'
>                  else:
> -                    self.setup_metadata['method'] = 'unknown'
> +                    raise Exception("handle setuptools")

So we don't support setuptools as a PEP517 backend? I guess it was
already the case before your patch?

>              except KeyError:
>                  print('ERROR: Could not determine package metadata for {pkg}.\n'
>                        .format(pkg=self.real_name))
>                  raise
>          except FileNotFoundError:
>              raise
> -        os.chdir(current_dir)
> -        sys.path.remove(self.tmp_extract)

Related to what?

> +        finally:
> +            os.chdir(current_dir)
>  
>      def get_requirements(self, pkg_folder):
>          """
> @@ -778,15 +748,15 @@ def main():
>  
>              # Loading the package install info from the package
>              try:
> -                package.load_setup()
> -            except ImportError as err:
> -                if 'buildutils' in str(err):
> -                    print('This package needs buildutils')
> -                    continue
> -                else:
> -                    try:
> -                        package.load_pyproject()
> -                    except Exception:
> +                package.load_pyproject()
> +            except Exception:
> +                try:
> +                    package.load_setup()
> +                except ImportError as err:
> +                    if 'buildutils' in str(err):
> +                        print('This package needs buildutils')
> +                        continue
> +                    else:

This is a separate change, about the ordering between .load_setup() and
.load_pyproject().

Could you split this useful patch into smaller chunks, with a better
justification for each chunk?

Believe me, if you do this, patches like this will be merged, much,
much, much faster.

Thanks a lot for your contribution!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot