[Buildroot] [PATCH v2 1/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[Buildroot] [PATCH v2 1/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[James Hilliard]

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 utils/scanpypi | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/utils/scanpypi b/utils/scanpypi
index 17d8a0017a..724e59f759 100755
--- a/utils/scanpypi
+++ b/utils/scanpypi
@@ -296,23 +296,25 @@ class BuildrootPackage():
         current_dir = os.getcwd()
         os.chdir(self.tmp_extract)
         sys.path.insert(0, self.tmp_extract)
-        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('-', '_')
         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
-        os.chdir(current_dir)
-        sys.path.remove(self.tmp_extract)
+            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('-', '_')
+            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
+        finally:
+            os.chdir(current_dir)
+            sys.path.remove(self.tmp_extract)
 
     def get_requirements(self, pkg_folder):
         """
-- 
2.25.1

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

[Buildroot] [PATCH v2 2/2] utils/scanpypi: add flit package support

[James Hilliard]

These packages don't have a setup.py so we instead need to parse their
pyproject.toml file.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
Changes v1 -> v2:
  - remove homepage format fixes(sent as separate patch)
  - remove load_setup fixes
  - move load_pyproject cleanup to finally block
---
 utils/scanpypi | 80 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/utils/scanpypi b/utils/scanpypi
index 724e59f759..f501b36232 100755
--- a/utils/scanpypi
+++ b/utils/scanpypi
@@ -42,6 +42,48 @@ except ImportError:
           'pip install spdx_lookup')
     liclookup = None
 
+def toml_load(f):
+    with open(f, 'rb') as fh:
+        ex = None
+
+        # Try regular tomli first
+        try:
+            from tomli import load
+            return load(fh)
+        except ImportError as e:
+            ex = e
+
+        # Try pip's vendored tomli
+        try:
+            from pip._vendor.tomli import load
+            try:
+                return load(fh)
+            except TypeError:
+                # Fallback to handle older version
+                try:
+                    fh.seek(0)
+                    w = io.TextIOWrapper(fh, encoding="utf8", newline="")
+                    return load(w)
+                finally:
+                    w.detach()
+        except ImportError as e:
+            pass
+
+        # Try regular toml last
+        try:
+            from toml import load
+            fh.seek(0)
+            w = io.TextIOWrapper(fh, encoding="utf8", newline="")
+            try:
+                return load(w)
+            finally:
+                w.detach()
+        except ImportError:
+            pass
+
+        print('This package needs tomli')
+        raise ex
+
 
 def setup_decorator(func, method):
     """
@@ -316,6 +358,37 @@ class BuildrootPackage():
             os.chdir(current_dir)
             sys.path.remove(self.tmp_extract)
 
+    def load_pyproject(self):
+        """
+        Loads the corresponding pyproject.toml and store its metadata
+        """
+        current_dir = os.getcwd()
+        os.chdir(self.tmp_extract)
+        sys.path.insert(0, self.tmp_extract)
+        try:
+            pyproject_data = toml_load('pyproject.toml')
+            try:
+                self.setup_metadata = pyproject_data.get('project', {})
+                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 is not None and build_backend == 'flit_core.buildapi':
+                    self.setup_metadata['method'] = 'flit'
+                elif build_system.get('backend-path', None) is not None:
+                    self.setup_metadata['method'] = 'pep517'
+                else:
+                    self.setup_metadata['method'] = 'unknown'
+            except KeyError:
+                # This means setup was not called
+                print('ERROR: Could not determine package metadata for {pkg}.\n'
+                      .format(pkg=self.real_name))
+                raise
+        except FileNotFoundError:
+            raise
+        finally:
+            os.chdir(current_dir)
+            sys.path.remove(self.tmp_extract)
+
     def get_requirements(self, pkg_folder):
         """
         Retrieve dependencies from the metadata found in the setup.py script of
@@ -694,9 +767,12 @@ def main():
             except ImportError as err:
                 if 'buildutils' in str(err):
                     print('This package needs buildutils')
+                    continue
                 else:
-                    raise
-                continue
+                    try:
+                        package.load_pyproject()
+                    except Exception as e:
+                        raise
             except (AttributeError, KeyError) as error:
                 print('Error: Could not install package {pkg}: {error}'.format(
                     pkg=package.real_name, error=error))
-- 
2.25.1

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

Re: [Buildroot] [PATCH v2 1/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[Thomas Petazzoni via buildroot]

On Sun, 13 Mar 2022 11:13:54 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> +            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('-', '_')
> +            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
> +        finally:
> +            os.chdir(current_dir)
> +            sys.path.remove(self.tmp_extract)
>  
>      def get_requirements(self, pkg_folder):
>          """

Are you sure this work correctly?

If I understand correctly self.tmp_extract is where the Python package
source code is extracted. So if you remove it at the end of
load_setup(), how will your new load_pyproject() added in PATCH 2/2 be
able to use the package source code?

Also, why is this removal needed, as anyway the full tmp_path gets
removed at the end of the main() function?

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 2/2] utils/scanpypi: add flit package support

[Thomas Petazzoni via buildroot]

On Sun, 13 Mar 2022 11:13:55 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> +def toml_load(f):
> +    with open(f, 'rb') as fh:
> +        ex = None
> +
> +        # Try regular tomli first
> +        try:
> +            from tomli import load
> +            return load(fh)
> +        except ImportError as e:
> +            ex = e
> +
> +        # Try pip's vendored tomli
> +        try:
> +            from pip._vendor.tomli import load
> +            try:
> +                return load(fh)
> +            except TypeError:
> +                # Fallback to handle older version
> +                try:
> +                    fh.seek(0)
> +                    w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> +                    return load(w)
> +                finally:
> +                    w.detach()
> +        except ImportError as e:
> +            pass
> +
> +        # Try regular toml last
> +        try:
> +            from toml import load
> +            fh.seek(0)
> +            w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> +            try:
> +                return load(w)
> +            finally:
> +                w.detach()
> +        except ImportError:
> +            pass
> +
> +        print('This package needs tomli')
> +        raise ex

I'm confused by how "ex" gets used here. It's initially none, and it's
set to the ImportError exception if the tomli module couldn't be used.

>      def get_requirements(self, pkg_folder):
>          """
>          Retrieve dependencies from the metadata found in the setup.py script of
> @@ -694,9 +767,12 @@ def main():
>              except ImportError as err:
>                  if 'buildutils' in str(err):
>                      print('This package needs buildutils')
> +                    continue
>                  else:
> -                    raise
> -                continue
> +                    try:
> +                        package.load_pyproject()
> +                    except Exception as e:
> +                        raise

I really don't like the construction here. We're doing this:

	try:
		... do an attempt with traditional setup.py ...
	expect ImportError as err:
		...
		try:
			... do an attempt with pyproject.toml ...
		expect Exception as e:
			...

So, if get a third and then a fourth method of Python module packaging,
we will end up with:

	try:
		... do an attempt with traditional setup.py ...
	expect ImportError as err:
		...
		try:
			... do an attempt with pyproject.toml ...
		expect Exception as e:
			...
			try:
				... do an attempt with another method
			except:
				try:
					... do an attempt with another method
				except:

Clearly ugly. Can we have a better construct? I would know how to do
that in C, but I'm not very good at Python-ic constructs.
				
Thanks!

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/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[James Hilliard]

On Wed, Aug 3, 2022 at 3:38 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Sun, 13 Mar 2022 11:13:54 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > +            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('-', '_')
> > +            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
> > +        finally:
> > +            os.chdir(current_dir)
> > +            sys.path.remove(self.tmp_extract)
> >
> >      def get_requirements(self, pkg_folder):
> >          """
>
> Are you sure this work correctly?

Looks right to me.

>
> If I understand correctly self.tmp_extract is where the Python package
> source code is extracted. So if you remove it at the end of
> load_setup(), how will your new load_pyproject() added in PATCH 2/2 be
> able to use the package source code?

We're not deleting self.tmp_extract, we're just ensuring we current dir and
sys.path states are cleaned up properly on failure, load_pyproject() does the
same thing. Always restoring initial state at the end means load_pyproject()
doesn't need to deal with current dir and sys.path global state changes induced
by load_setup().

>
> Also, why is this removal needed, as anyway the full tmp_path gets
> removed at the end of the main() function?

sys.path.remove(self.tmp_extract) is not the same as shutil.rmtree(tmp_path)

We're restoring the module search path at the end to avoid leaking global state
changes when there's an exception.

>
> 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 2/2] utils/scanpypi: add flit package support

[James Hilliard]

On Wed, Aug 3, 2022 at 3:46 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Sun, 13 Mar 2022 11:13:55 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > +def toml_load(f):
> > +    with open(f, 'rb') as fh:
> > +        ex = None
> > +
> > +        # Try regular tomli first
> > +        try:
> > +            from tomli import load
> > +            return load(fh)
> > +        except ImportError as e:
> > +            ex = e
> > +
> > +        # Try pip's vendored tomli
> > +        try:
> > +            from pip._vendor.tomli import load
> > +            try:
> > +                return load(fh)
> > +            except TypeError:
> > +                # Fallback to handle older version
> > +                try:
> > +                    fh.seek(0)
> > +                    w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> > +                    return load(w)
> > +                finally:
> > +                    w.detach()
> > +        except ImportError as e:
> > +            pass
> > +
> > +        # Try regular toml last
> > +        try:
> > +            from toml import load
> > +            fh.seek(0)
> > +            w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> > +            try:
> > +                return load(w)
> > +            finally:
> > +                w.detach()
> > +        except ImportError:
> > +            pass
> > +
> > +        print('This package needs tomli')
> > +        raise ex
>
> I'm confused by how "ex" gets used here. It's initially none, and it's
> set to the ImportError exception if the tomli module couldn't be used.
>
> >      def get_requirements(self, pkg_folder):
> >          """
> >          Retrieve dependencies from the metadata found in the setup.py script of
> > @@ -694,9 +767,12 @@ def main():
> >              except ImportError as err:
> >                  if 'buildutils' in str(err):
> >                      print('This package needs buildutils')
> > +                    continue
> >                  else:
> > -                    raise
> > -                continue
> > +                    try:
> > +                        package.load_pyproject()
> > +                    except Exception as e:
> > +                        raise
>
> I really don't like the construction here. We're doing this:
>
>         try:
>                 ... do an attempt with traditional setup.py ...
>         expect ImportError as err:
>                 ...
>                 try:
>                         ... do an attempt with pyproject.toml ...
>                 expect Exception as e:
>                         ...
>
> So, if get a third and then a fourth method of Python module packaging,
> we will end up with:

All new python packaging methods are supposed to contain a pyproject.toml
file per pep517 so I think we can probably defer refactoring this for now.

The only supported packaging method in the absence of a pyproject.toml
will be setuptools(distutils is effectively being moved inside of setuptools
to avoid breaking older packages that are not migrated to setuptools, so
once distutils is removed from python our current distutils packages will
need to be moved to setuptools infrastructure, but that should more or
less just work automatically).

>
>         try:
>                 ... do an attempt with traditional setup.py ...
>         expect ImportError as err:
>                 ...
>                 try:
>                         ... do an attempt with pyproject.toml ...
>                 expect Exception as e:
>                         ...
>                         try:
>                                 ... do an attempt with another method
>                         except:
>                                 try:
>                                         ... do an attempt with another method
>                                 except:
>
> Clearly ugly. Can we have a better construct? I would know how to do
> that in C, but I'm not very good at Python-ic constructs.
>
> Thanks!
>
> 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/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[Thomas Petazzoni via buildroot]

Hello,

On Wed, 3 Aug 2022 16:02:43 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> > If I understand correctly self.tmp_extract is where the Python package
> > source code is extracted. So if you remove it at the end of
> > load_setup(), how will your new load_pyproject() added in PATCH 2/2 be
> > able to use the package source code?  
> 
> We're not deleting self.tmp_extract, we're just ensuring we current dir and
> sys.path states are cleaned up properly on failure, load_pyproject() does the
> same thing. Always restoring initial state at the end means load_pyproject()
> doesn't need to deal with current dir and sys.path global state changes induced
> by load_setup().
> 
> >
> > Also, why is this removal needed, as anyway the full tmp_path gets
> > removed at the end of the main() function?  
> 
> sys.path.remove(self.tmp_extract) is not the same as shutil.rmtree(tmp_path)
> 
> We're restoring the module search path at the end to avoid leaking global state
> changes when there's an exception.

Aaah, yes, my bad! I guess I shouldn't have reviewed this past midnight.

I'll go ahead and apply PATCH 1/2.

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 2/2] utils/scanpypi: add flit package support

[Marcus Hoffmann]

Hi James,

I needed to package quite a lot of python dependencies, so I tried out 
this patch. There's a number of things missing still here, plus I ended 
up being a bit confused with the pep517 vs flit setup types... maybe you 
could help clarifying this?


On 13.03.22 18:13, James Hilliard wrote:
> These packages don't have a setup.py so we instead need to parse their
> pyproject.toml file.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
> Changes v1 -> v2:
>    - remove homepage format fixes(sent as separate patch)
>    - remove load_setup fixes
>    - move load_pyproject cleanup to finally block
> ---
>   utils/scanpypi | 80 ++++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 78 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/scanpypi b/utils/scanpypi
> index 724e59f759..f501b36232 100755
> --- a/utils/scanpypi
> +++ b/utils/scanpypi
> @@ -42,6 +42,48 @@ except ImportError:
>             'pip install spdx_lookup')
>       liclookup = None
>   
> +def toml_load(f):
> +    with open(f, 'rb') as fh:
> +        ex = None
> +
> +        # Try regular tomli first
> +        try:
> +            from tomli import load
> +            return load(fh)
> +        except ImportError as e:
> +            ex = e
> +
> +        # Try pip's vendored tomli
> +        try:
> +            from pip._vendor.tomli import load
> +            try:
> +                return load(fh)
> +            except TypeError:
> +                # Fallback to handle older version
> +                try:
> +                    fh.seek(0)
> +                    w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> +                    return load(w)
> +                finally:
> +                    w.detach()
> +        except ImportError as e:
> +            pass
> +
> +        # Try regular toml last
> +        try:
> +            from toml import load
> +            fh.seek(0)
> +            w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> +            try:
> +                return load(w)
> +            finally:
> +                w.detach()
> +        except ImportError:
> +            pass
> +
> +        print('This package needs tomli')
> +        raise ex

Couldn't we just try to use tomli or error out with this message and be 
done with it? Surely users of this script would know how to get tomli 
for their python install?

(For what it's worth python 3.11 will ship with a copy of tomli under 
the tomllib module, so we could probably try using that and otherwise 
fall back to requiring tomli)

> +
>   
>   def setup_decorator(func, method):
>       """
> @@ -316,6 +358,37 @@ class BuildrootPackage():
>               os.chdir(current_dir)
>               sys.path.remove(self.tmp_extract)
>   
> +    def load_pyproject(self):
> +        """
> +        Loads the corresponding pyproject.toml and store its metadata
> +        """
> +        current_dir = os.getcwd()
> +        os.chdir(self.tmp_extract)
> +        sys.path.insert(0, self.tmp_extract)
> +        try:
> +            pyproject_data = toml_load('pyproject.toml')
> +            try:
> +                self.setup_metadata = pyproject_data.get('project', {})
> +                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 is not None and build_backend == 'flit_core.buildapi':

This doesn't catch the very common case of having a pyproject.toml which 
requests the setuptools build backend via build-backend = 
"setuptools.build_meta"

These projects could alternatively be built via the setuptools frontend 
iff they ship with a (stub) setup.py file but it seems better to just 
build them via pep517 anyway, right?

Mh, why do we even have flit and pep517 build systems as separate 
options? flit is (at least nowadays) only a pep517 backend (plus a 
frontend which we don't use, I think). (I tried making sense of 
package/pkg-python.mk but I couldn't really see the differerence between 
the pep517 and flit methods)

So shouldn't anything that has a pyproject.toml with a [build-system] 
section be pep517?

> +                    self.setup_metadata['method'] = 'flit'
> +                elif build_system.get('backend-path', None) is not None:
> +                    self.setup_metadata['method'] = 'pep517' > +                else:
> +                    self.setup_metadata['method'] = 'unknown'

We are still missing reading out the dependencies for the pyproject.toml 
case, right? Dependencies are currently expected to be in the 
"install_requires" key in the metadata, but in the new world they are 
under 'dependencies'. Easiest would be copying them over right here. The 
format of those dependency specifications seems to be the same. (There's 
also the old-style flit metadata[1] that i.e. fastapi[2] uses for 
dependency specifications, but I don't think that's worth supporting; We 
also don't handle requirements.txt or setup.cfg *shrug*)

> +            except KeyError:
> +                # This means setup was not called

This comment is wrong here, I believe.

> +                print('ERROR: Could not determine package metadata for {pkg}.\n'
> +                      .format(pkg=self.real_name))
> +                raise
> +        except FileNotFoundError:
> +            raise
> +        finally:
> +            os.chdir(current_dir)
> +            sys.path.remove(self.tmp_extract)
> +
>       def get_requirements(self, pkg_folder):
>           """
>           Retrieve dependencies from the metadata found in the setup.py script of
> @@ -694,9 +767,12 @@ def main():
>               except ImportError as err:
>                   if 'buildutils' in str(err):
>                       print('This package needs buildutils')
> +                    continue
>                   else:
> -                    raise
> -                continue
> +                    try:
> +                        package.load_pyproject()
> +                    except Exception as e:
> +                        raise
>               except (AttributeError, KeyError) as error:
>                   print('Error: Could not install package {pkg}: {error}'.format(
>                       pkg=package.real_name, error=error))

One other thing we'd need to handle here is packages which now ship with 
just a stub setup.py file, i.e. [3]. Those will trigger one 
AttributeError, KeyError and we should in that case just continue on 
with parsing the pyproject.toml.


[1]: https://flit.pypa.io/en/latest/pyproject_toml.html#old-style-metadata
[2]: https://github.com/tiangolo/fastapi/blob/0.79.0/pyproject.toml
[3]: https://github.com/Pylons/waitress/blob/v2.1.2/setup.py
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 2/2] utils/scanpypi: add flit package support

[James Hilliard]

On Tue, Aug 16, 2022 at 6:48 PM Marcus Hoffmann
<marcus.hoffmann@othermo.de> wrote:
>
> Hi James,
>
> I needed to package quite a lot of python dependencies, so I tried out
> this patch. There's a number of things missing still here, plus I ended
> up being a bit confused with the pep517 vs flit setup types... maybe you
> could help clarifying this?

Yeah, this is more of an initial flit support patch, not a complete flit support
patch.

>
>
> On 13.03.22 18:13, James Hilliard wrote:
> > These packages don't have a setup.py so we instead need to parse their
> > pyproject.toml file.
> >
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> > Changes v1 -> v2:
> >    - remove homepage format fixes(sent as separate patch)
> >    - remove load_setup fixes
> >    - move load_pyproject cleanup to finally block
> > ---
> >   utils/scanpypi | 80 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 78 insertions(+), 2 deletions(-)
> >
> > diff --git a/utils/scanpypi b/utils/scanpypi
> > index 724e59f759..f501b36232 100755
> > --- a/utils/scanpypi
> > +++ b/utils/scanpypi
> > @@ -42,6 +42,48 @@ except ImportError:
> >             'pip install spdx_lookup')
> >       liclookup = None
> >
> > +def toml_load(f):
> > +    with open(f, 'rb') as fh:
> > +        ex = None
> > +
> > +        # Try regular tomli first
> > +        try:
> > +            from tomli import load
> > +            return load(fh)
> > +        except ImportError as e:
> > +            ex = e
> > +
> > +        # Try pip's vendored tomli
> > +        try:
> > +            from pip._vendor.tomli import load
> > +            try:
> > +                return load(fh)
> > +            except TypeError:
> > +                # Fallback to handle older version
> > +                try:
> > +                    fh.seek(0)
> > +                    w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> > +                    return load(w)
> > +                finally:
> > +                    w.detach()
> > +        except ImportError as e:
> > +            pass
> > +
> > +        # Try regular toml last
> > +        try:
> > +            from toml import load
> > +            fh.seek(0)
> > +            w = io.TextIOWrapper(fh, encoding="utf8", newline="")
> > +            try:
> > +                return load(w)
> > +            finally:
> > +                w.detach()
> > +        except ImportError:
> > +            pass
> > +
> > +        print('This package needs tomli')
> > +        raise ex
>
> Couldn't we just try to use tomli or error out with this message and be
> done with it? Surely users of this script would know how to get tomli
> for their python install?

I mean, we could...but this greatly increases the probability of things
working without having to install additional libraries(as most users will at
least have pip already).

>
> (For what it's worth python 3.11 will ship with a copy of tomli under
> the tomllib module, so we could probably try using that and otherwise
> fall back to requiring tomli)

We'll want to add that once python 3.11 is released, but this should work
reasonably reliably for now.

>
> > +
> >
> >   def setup_decorator(func, method):
> >       """
> > @@ -316,6 +358,37 @@ class BuildrootPackage():
> >               os.chdir(current_dir)
> >               sys.path.remove(self.tmp_extract)
> >
> > +    def load_pyproject(self):
> > +        """
> > +        Loads the corresponding pyproject.toml and store its metadata
> > +        """
> > +        current_dir = os.getcwd()
> > +        os.chdir(self.tmp_extract)
> > +        sys.path.insert(0, self.tmp_extract)
> > +        try:
> > +            pyproject_data = toml_load('pyproject.toml')
> > +            try:
> > +                self.setup_metadata = pyproject_data.get('project', {})
> > +                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 is not None and build_backend == 'flit_core.buildapi':
>
> This doesn't catch the very common case of having a pyproject.toml which
> requests the setuptools build backend via build-backend =
> "setuptools.build_meta"

Yes, this doesn't touch setuptools at the moment, that does need some
cleanup but IMO best to deal with that after this is merged.

>
> These projects could alternatively be built via the setuptools frontend
> iff they ship with a (stub) setup.py file but it seems better to just
> build them via pep517 anyway, right?

Well there are currently blockers in regards to migrating out setuptools
infrastructure to use pep517 builds, see details:
https://github.com/buildroot/buildroot/commit/4686fb32a053753f502e24c5cd980db5b71f6a2c

>
> Mh, why do we even have flit and pep517 build systems as separate
> options? flit is (at least nowadays) only a pep517 backend (plus a
> frontend which we don't use, I think). (I tried making sense of
> package/pkg-python.mk but I couldn't really see the differerence between
> the pep517 and flit methods)

Well, the setup type is essentially the same as the build backend, we have
a base pep517 setup type for handling non-flit based pep517 build backends.

For example the we use it with the maturin build-backend at the moment(which
doesn't yet have full python infrastructure support):
https://github.com/buildroot/buildroot/blob/2022.08-rc1/package/python-orjson/python-orjson.mk#L10

I do have a pending patch adding pyo3(maturin/setuptools-rust) infra support
here:
https://patchwork.ozlabs.org/project/buildroot/patch/20220807003832.1177015-1-james.hilliard1@gmail.com/

>
> So shouldn't anything that has a pyproject.toml with a [build-system]
> section be pep517?
>
> > +                    self.setup_metadata['method'] = 'flit'
> > +                elif build_system.get('backend-path', None) is not None:
> > +                    self.setup_metadata['method'] = 'pep517' > +                else:
> > +                    self.setup_metadata['method'] = 'unknown'
>
> We are still missing reading out the dependencies for the pyproject.toml
> case, right? Dependencies are currently expected to be in the
> "install_requires" key in the metadata, but in the new world they are
> under 'dependencies'. Easiest would be copying them over right here. The
> format of those dependency specifications seems to be the same. (There's
> also the old-style flit metadata[1] that i.e. fastapi[2] uses for
> dependency specifications, but I don't think that's worth supporting; We
> also don't handle requirements.txt or setup.cfg *shrug*)

Yeah, I'm aware this isn't handling a number of things like dependencies
yet.

>
> > +            except KeyError:
> > +                # This means setup was not called
>
> This comment is wrong here, I believe.

Yeah, I'll remove that.

>
> > +                print('ERROR: Could not determine package metadata for {pkg}.\n'
> > +                      .format(pkg=self.real_name))
> > +                raise
> > +        except FileNotFoundError:
> > +            raise
> > +        finally:
> > +            os.chdir(current_dir)
> > +            sys.path.remove(self.tmp_extract)
> > +
> >       def get_requirements(self, pkg_folder):
> >           """
> >           Retrieve dependencies from the metadata found in the setup.py script of
> > @@ -694,9 +767,12 @@ def main():
> >               except ImportError as err:
> >                   if 'buildutils' in str(err):
> >                       print('This package needs buildutils')
> > +                    continue
> >                   else:
> > -                    raise
> > -                continue
> > +                    try:
> > +                        package.load_pyproject()
> > +                    except Exception as e:
> > +                        raise
> >               except (AttributeError, KeyError) as error:
> >                   print('Error: Could not install package {pkg}: {error}'.format(
> >                       pkg=package.real_name, error=error))
>
> One other thing we'd need to handle here is packages which now ship with
> just a stub setup.py file, i.e. [3]. Those will trigger one
> AttributeError, KeyError and we should in that case just continue on
> with parsing the pyproject.toml.

Yeah, setuptools support def needs some cleanup, was waiting for flit support
to be merged before working on that.

>
>
> [1]: https://flit.pypa.io/en/latest/pyproject_toml.html#old-style-metadata
> [2]: https://github.com/tiangolo/fastapi/blob/0.79.0/pyproject.toml
> [3]: https://github.com/Pylons/waitress/blob/v2.1.2/setup.py
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH v2 1/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[James Hilliard]

On Thu, Aug 4, 2022 at 1:59 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Wed, 3 Aug 2022 16:02:43 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > > If I understand correctly self.tmp_extract is where the Python package
> > > source code is extracted. So if you remove it at the end of
> > > load_setup(), how will your new load_pyproject() added in PATCH 2/2 be
> > > able to use the package source code?
> >
> > We're not deleting self.tmp_extract, we're just ensuring we current dir and
> > sys.path states are cleaned up properly on failure, load_pyproject() does the
> > same thing. Always restoring initial state at the end means load_pyproject()
> > doesn't need to deal with current dir and sys.path global state changes induced
> > by load_setup().
> >
> > >
> > > Also, why is this removal needed, as anyway the full tmp_path gets
> > > removed at the end of the main() function?
> >
> > sys.path.remove(self.tmp_extract) is not the same as shutil.rmtree(tmp_path)
> >
> > We're restoring the module search path at the end to avoid leaking global state
> > changes when there's an exception.
>
> Aaah, yes, my bad! I guess I shouldn't have reviewed this past midnight.
>
> I'll go ahead and apply PATCH 1/2.

I'm not seeing this in either master or next yet.

>
> 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/2] utils/scanpypi: ensure tmp_extract is cleaned up on exception

[James Hilliard]

On Tue, Aug 16, 2022 at 11:57 PM James Hilliard
<james.hilliard1@gmail.com> wrote:
>
> On Thu, Aug 4, 2022 at 1:59 AM Thomas Petazzoni
> <thomas.petazzoni@bootlin.com> wrote:
> >
> > Hello,
> >
> > On Wed, 3 Aug 2022 16:02:43 -0600
> > James Hilliard <james.hilliard1@gmail.com> wrote:
> >
> > > > If I understand correctly self.tmp_extract is where the Python package
> > > > source code is extracted. So if you remove it at the end of
> > > > load_setup(), how will your new load_pyproject() added in PATCH 2/2 be
> > > > able to use the package source code?
> > >
> > > We're not deleting self.tmp_extract, we're just ensuring we current dir and
> > > sys.path states are cleaned up properly on failure, load_pyproject() does the
> > > same thing. Always restoring initial state at the end means load_pyproject()
> > > doesn't need to deal with current dir and sys.path global state changes induced
> > > by load_setup().
> > >
> > > >
> > > > Also, why is this removal needed, as anyway the full tmp_path gets
> > > > removed at the end of the main() function?
> > >
> > > sys.path.remove(self.tmp_extract) is not the same as shutil.rmtree(tmp_path)
> > >
> > > We're restoring the module search path at the end to avoid leaking global state
> > > changes when there's an exception.
> >
> > Aaah, yes, my bad! I guess I shouldn't have reviewed this past midnight.
> >
> > I'll go ahead and apply PATCH 1/2.
>
> I'm not seeing this in either master or next yet.

Respin with clearer commit log:
https://patchwork.ozlabs.org/project/buildroot/patch/20220817061157.4120650-1-james.hilliard1@gmail.com/

>
> >
> > 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 2/2] utils/scanpypi: add flit package support

[Marcus Hoffmann]

Hi,

On 17.08.22 07:56, James Hilliard wrote:
> On Tue, Aug 16, 2022 at 6:48 PM Marcus Hoffmann
> <marcus.hoffmann@othermo.de> wrote:
>>

[...]

>> Couldn't we just try to use tomli or error out with this message and be
>> done with it? Surely users of this script would know how to get tomli
>> for their python install?
> 
> I mean, we could...but this greatly increases the probability of things
> working without having to install additional libraries(as most users will at
> least have pip already).
> 
>>
>> (For what it's worth python 3.11 will ship with a copy of tomli under
>> the tomllib module, so we could probably try using that and otherwise
>> fall back to requiring tomli)
> 
> We'll want to add that once python 3.11 is released, but this should work
> reasonably reliably for now.
> 

I'm not a fan of trying to use internal modules of pip, but also not a 
big deal *shrug*.

>>
>>> +
>>>
>>>    def setup_decorator(func, method):
>>>        """
>>> @@ -316,6 +358,37 @@ class BuildrootPackage():
>>>                os.chdir(current_dir)
>>>                sys.path.remove(self.tmp_extract)
>>>
>>> +    def load_pyproject(self):
>>> +        """
>>> +        Loads the corresponding pyproject.toml and store its metadata
>>> +        """
>>> +        current_dir = os.getcwd()
>>> +        os.chdir(self.tmp_extract)
>>> +        sys.path.insert(0, self.tmp_extract)
>>> +        try:
>>> +            pyproject_data = toml_load('pyproject.toml')
>>> +            try:
>>> +                self.setup_metadata = pyproject_data.get('project', {})
>>> +                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 is not None and build_backend == 'flit_core.buildapi':
>>
>> This doesn't catch the very common case of having a pyproject.toml which
>> requests the setuptools build backend via build-backend =
>> "setuptools.build_meta"
> 
> Yes, this doesn't touch setuptools at the moment, that does need some
> cleanup but IMO best to deal with that after this is merged.
> 
>>
>> These projects could alternatively be built via the setuptools frontend
>> iff they ship with a (stub) setup.py file but it seems better to just
>> build them via pep517 anyway, right?
> 
> Well there are currently blockers in regards to migrating out setuptools
> infrastructure to use pep517 builds, see details:
> https://github.com/buildroot/buildroot/commit/4686fb32a053753f502e24c5cd980db5b71f6a2c
> 

Thanks, that greatly helped me understand the status quo around 
setuptools/pep517 support in builroot :).

>>

[...]

>>
>>> +                    self.setup_metadata['method'] = 'flit'
>>> +                elif build_system.get('backend-path', None) is not None:
>>> +                    self.setup_metadata['method'] = 'pep517' > +                else:
>>> +                    self.setup_metadata['method'] = 'unknown'
>>
>> We are still missing reading out the dependencies for the pyproject.toml
>> case, right? Dependencies are currently expected to be in the
>> "install_requires" key in the metadata, but in the new world they are
>> under 'dependencies'. Easiest would be copying them over right here. The
>> format of those dependency specifications seems to be the same. (There's
>> also the old-style flit metadata[1] that i.e. fastapi[2] uses for
>> dependency specifications, but I don't think that's worth supporting; We
>> also don't handle requirements.txt or setup.cfg *shrug*)
> 
> Yeah, I'm aware this isn't handling a number of things like dependencies
> yet.

That should probably be at least noted in the commit message. Or just 
added to the next revision here.

Adding the following line just makes it work for now, even if that maybe 
isn't an ideal solution:

 > self.setup_metadata['install_requires'] = 
self.setup_metadata.get('dependencies', [])


> 
>>
>>> +            except KeyError:
>>> +                # This means setup was not called
>>
>> This comment is wrong here, I believe.
> 
> Yeah, I'll remove that.
> 
>>
>>> +                print('ERROR: Could not determine package metadata for {pkg}.\n'
>>> +                      .format(pkg=self.real_name))
>>> +                raise
>>> +        except FileNotFoundError:
>>> +            raise
>>> +        finally:
>>> +            os.chdir(current_dir)
>>> +            sys.path.remove(self.tmp_extract)
>>> +
>>>        def get_requirements(self, pkg_folder):
>>>            """
>>>            Retrieve dependencies from the metadata found in the setup.py script of
>>> @@ -694,9 +767,12 @@ def main():
>>>                except ImportError as err:
>>>                    if 'buildutils' in str(err):
>>>                        print('This package needs buildutils')
>>> +                    continue
>>>                    else:
>>> -                    raise
>>> -                continue
>>> +                    try:
>>> +                        package.load_pyproject()
>>> +                    except Exception as e:
>>> +                        raise
>>>                except (AttributeError, KeyError) as error:
>>>                    print('Error: Could not install package {pkg}: {error}'.format(
>>>                        pkg=package.real_name, error=error))
>>
>> One other thing we'd need to handle here is packages which now ship with
>> just a stub setup.py file, i.e. [3]. Those will trigger one
>> AttributeError, KeyError and we should in that case just continue on
>> with parsing the pyproject.toml.
> 
> Yeah, setuptools support def needs some cleanup, was waiting for flit support
> to be merged before working on that

Right, that can happen later.

[...]

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

Re: [Buildroot] [PATCH v2 2/2] utils/scanpypi: add flit package support

[James Hilliard]

On Wed, Aug 17, 2022 at 8:24 AM Marcus Hoffmann
<marcus.hoffmann@othermo.de> wrote:
>
> Hi,
>
> On 17.08.22 07:56, James Hilliard wrote:
> > On Tue, Aug 16, 2022 at 6:48 PM Marcus Hoffmann
> > <marcus.hoffmann@othermo.de> wrote:
> >>
>
> [...]
>
> >> Couldn't we just try to use tomli or error out with this message and be
> >> done with it? Surely users of this script would know how to get tomli
> >> for their python install?
> >
> > I mean, we could...but this greatly increases the probability of things
> > working without having to install additional libraries(as most users will at
> > least have pip already).
> >
> >>
> >> (For what it's worth python 3.11 will ship with a copy of tomli under
> >> the tomllib module, so we could probably try using that and otherwise
> >> fall back to requiring tomli)
> >
> > We'll want to add that once python 3.11 is released, but this should work
> > reasonably reliably for now.
> >
>
> I'm not a fan of trying to use internal modules of pip, but also not a
> big deal *shrug*.

Yeah, it's literally just pip's vendored tomli so shouldn't cause issues,
pip is pretty always present in system wide python installs while the
unvendored tomli is not.

>
> >>
> >>> +
> >>>
> >>>    def setup_decorator(func, method):
> >>>        """
> >>> @@ -316,6 +358,37 @@ class BuildrootPackage():
> >>>                os.chdir(current_dir)
> >>>                sys.path.remove(self.tmp_extract)
> >>>
> >>> +    def load_pyproject(self):
> >>> +        """
> >>> +        Loads the corresponding pyproject.toml and store its metadata
> >>> +        """
> >>> +        current_dir = os.getcwd()
> >>> +        os.chdir(self.tmp_extract)
> >>> +        sys.path.insert(0, self.tmp_extract)
> >>> +        try:
> >>> +            pyproject_data = toml_load('pyproject.toml')
> >>> +            try:
> >>> +                self.setup_metadata = pyproject_data.get('project', {})
> >>> +                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 is not None and build_backend == 'flit_core.buildapi':
> >>
> >> This doesn't catch the very common case of having a pyproject.toml which
> >> requests the setuptools build backend via build-backend =
> >> "setuptools.build_meta"
> >
> > Yes, this doesn't touch setuptools at the moment, that does need some
> > cleanup but IMO best to deal with that after this is merged.
> >
> >>
> >> These projects could alternatively be built via the setuptools frontend
> >> iff they ship with a (stub) setup.py file but it seems better to just
> >> build them via pep517 anyway, right?
> >
> > Well there are currently blockers in regards to migrating out setuptools
> > infrastructure to use pep517 builds, see details:
> > https://github.com/buildroot/buildroot/commit/4686fb32a053753f502e24c5cd980db5b71f6a2c
> >
>
> Thanks, that greatly helped me understand the status quo around
> setuptools/pep517 support in builroot :).
>
> >>
>
> [...]
>
> >>
> >>> +                    self.setup_metadata['method'] = 'flit'
> >>> +                elif build_system.get('backend-path', None) is not None:
> >>> +                    self.setup_metadata['method'] = 'pep517' > +                else:
> >>> +                    self.setup_metadata['method'] = 'unknown'
> >>
> >> We are still missing reading out the dependencies for the pyproject.toml
> >> case, right? Dependencies are currently expected to be in the
> >> "install_requires" key in the metadata, but in the new world they are
> >> under 'dependencies'. Easiest would be copying them over right here. The
> >> format of those dependency specifications seems to be the same. (There's
> >> also the old-style flit metadata[1] that i.e. fastapi[2] uses for
> >> dependency specifications, but I don't think that's worth supporting; We
> >> also don't handle requirements.txt or setup.cfg *shrug*)
> >
> > Yeah, I'm aware this isn't handling a number of things like dependencies
> > yet.
>
> That should probably be at least noted in the commit message. Or just
> added to the next revision here.

Added in v3:
https://patchwork.ozlabs.org/project/buildroot/patch/20220818082617.24624-1-james.hilliard1@gmail.com/

>
> Adding the following line just makes it work for now, even if that maybe
> isn't an ideal solution:
>
>  > self.setup_metadata['install_requires'] =
> self.setup_metadata.get('dependencies', [])

I'm thinking it may be possible to hook a pep517 function like:
get_requires_for_build_sdist(config_settings=None)
https://peps.python.org/pep-0517/#get-requires-for-build-sdist

This might also be usable with setuptools dependency resolution as well.

I'll try and look into this in more detail after initial flit support is merged.

>
>
> >
> >>
> >>> +            except KeyError:
> >>> +                # This means setup was not called
> >>
> >> This comment is wrong here, I believe.
> >
> > Yeah, I'll remove that.
> >
> >>
> >>> +                print('ERROR: Could not determine package metadata for {pkg}.\n'
> >>> +                      .format(pkg=self.real_name))
> >>> +                raise
> >>> +        except FileNotFoundError:
> >>> +            raise
> >>> +        finally:
> >>> +            os.chdir(current_dir)
> >>> +            sys.path.remove(self.tmp_extract)
> >>> +
> >>>        def get_requirements(self, pkg_folder):
> >>>            """
> >>>            Retrieve dependencies from the metadata found in the setup.py script of
> >>> @@ -694,9 +767,12 @@ def main():
> >>>                except ImportError as err:
> >>>                    if 'buildutils' in str(err):
> >>>                        print('This package needs buildutils')
> >>> +                    continue
> >>>                    else:
> >>> -                    raise
> >>> -                continue
> >>> +                    try:
> >>> +                        package.load_pyproject()
> >>> +                    except Exception as e:
> >>> +                        raise
> >>>                except (AttributeError, KeyError) as error:
> >>>                    print('Error: Could not install package {pkg}: {error}'.format(
> >>>                        pkg=package.real_name, error=error))
> >>
> >> One other thing we'd need to handle here is packages which now ship with
> >> just a stub setup.py file, i.e. [3]. Those will trigger one
> >> AttributeError, KeyError and we should in that case just continue on
> >> with parsing the pyproject.toml.
> >
> > Yeah, setuptools support def needs some cleanup, was waiting for flit support
> > to be merged before working on that
>
> Right, that can happen later.
>
> [...]
>
> Marcus
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot