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

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

[James Hilliard]

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.

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

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes v1 -> v2:
  - split out set comprehension changes
---
 utils/scanpypi | 88 +++++++++++++++++---------------------------------
 1 file changed, 29 insertions(+), 59 deletions(-)

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
 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')
-
-
 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)
         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)
         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'
                 elif build_system.get('backend-path', None):
                     self.setup_metadata['method'] = 'pep517'
                 else:
-                    self.setup_metadata['method'] = 'unknown'
+                    raise Exception("handle setuptools")
             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)
+        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:
                         raise
             except (AttributeError, KeyError) as error:
                 print('Error: Could not install package {pkg}: {error}'.format(
-- 
2.34.1

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Thomas Petazzoni via buildroot]

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

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

[James Hilliard]

On Wed, Feb 7, 2024 at 12:22 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> 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?

This is due to pep517 having a standardized way of retrieving metadata,
the monkeypatching was a hack to deal with setuptools/distutils not providing
a proper method for getting metadata.

>
> > 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)?

We want this to follow the same order of preference as our build infrastructure,
which now uses pep517 for setuptools based packages and thus pyproject.toml
is checked first.

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

It's all kind of intertwined and hard to split out.

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

Yes, this is related to replacing the monkeypatching.

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

Yes.

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

Yeah, so this is being removed due to it being used by the monkeypatching
hack.

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

Yep, this is also part of replacing the monkeypatching.

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

Yes, but it's related to removing the monkeypatching still in that otherwise
stuff will break due to incorrect ordering.

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

This is just to force the codepath to use the fallback for setuptools which
was previously used preferentially with the monkeypatching.

I'm thinking I should probably refactor this to handle pyproject.toml
setuptools within load_pyproject as it's a bit confusing.

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

Removal of the monkeypatching hack.

>
> > +        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().

This is actually related as the new pep517 based setuptools method does
also look at pyproject.toml and will in some cases cause the wrong setup
method to be used if we don't change the ordering.

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

It's a bit tricky in this case due to all the side effects of migrating away
from the monkeypatching which makes testing stuff in isolation hard,
the existing code here is also broken in a bunch of random ways to
to begin with.

My development process here was to just hack on stuff until things work
properly. I'll try and clean some stuff up more and see if I can split out
or minimize the reordering related changes but it's a bit tricky to test
things incrementally.

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