[Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[erichiggins]

Issue description:
The `utils/scanpypi` script makes an erroneous assumption that Python
packages will call `setup()` with the `name` argument. It's not
required and not often used. This causes the script to fail to load
many packages from Pypi.
 For example,  `./utils/scanpypi wheel` returns the following error:
> `Error: Could not install package wheel: 'name'`

Why it happens:
In plain english, the scanpypi` script assumes that a `name` argument
will be supplied to the `setup` call within the `setup.py` of a Python
package. If it's not there, then the script breaks.
Technical details: The `distutils.core.setup` and `setuptools.setup`
calls are wrapped by `setup_decorator` then monkey-patched after
import to set the `BuildrootPackage.setup_args` based on the `args`
and `kwargs` which were supplied by the package's call to `setup`.
This fails with `KeyError` when there are none.

One solution, which I've provided here as a patch, is to define
`BuildrootPackage.setup_args['name']` upon object instantiation using
the provided `real_name` variable, then using that as the fallback
option in the `setup_decorator`.

How I fixed it:
I store the package name as provided from the command line and use it
as a fallback option in case the-way-it-works-now doesn't work.

Signed-off-by: Eric Higgins <erichiggins@gmail.com>
---
 utils/scanpypi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/utils/scanpypi b/utils/scanpypi
index 452b4a3fc3..a5522a879e 100755
--- a/utils/scanpypi
+++ b/utils/scanpypi
@@ -58,8 +58,9 @@ def setup_decorator(func, method):
     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
+        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
+        BuildrootPackage.setup_args[name] = kwargs
+        BuildrootPackage.setup_args[name]['method'] = method
     return closure

 # monkey patch
@@ -147,6 +148,7 @@ class BuildrootPackage():
         self.url = None
         self.version = None
         self.license_files = []
+        self.setup_args['name'] = self.real_name

     def fetch_package_info(self):
         """
--
2.25.1
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[Thomas Petazzoni via buildroot]

Hello Eric,

+James in Cc.

Sorry for the super long lag.

On Sun, 18 Sep 2022 12:48:31 -0700
erichiggins@gmail.com wrote:

> Issue description:
> The `utils/scanpypi` script makes an erroneous assumption that Python
> packages will call `setup()` with the `name` argument. It's not
> required and not often used. This causes the script to fail to load
> many packages from Pypi.
>  For example,  `./utils/scanpypi wheel` returns the following error:
> > `Error: Could not install package wheel: 'name'`  

Do you have a current example that fails due to this? The wheel package
does pass a name= attribute to its setup() function, and it has been
doing this for many years. Looking at the initial commit of
https://github.com/pypa/wheel:

diff --git a/setup.py b/setup.py
new file mode 100644
index 0000000..09a138e
--- /dev/null
+++ b/setup.py
@@ -0,0 +1,33 @@
+import os
+
+from setuptools import setup
+
+here = os.path.abspath(os.path.dirname(__file__))
+README = open(os.path.join(here, 'README.txt')).read()
+CHANGES = open(os.path.join(here, 'CHANGES.txt')).read()
+
+setup(name='wheel',
+      version='0.1',
+      description='A built-package installer for Python.',
+      long_description=README + '\n\n' +  CHANGES,
+      classifiers=[
+        "Development Status :: 1 - ",
+        "Intended Audience :: Developers",

So I'm a bit puzzled at how you found a wheel package that doesn't pass
"name" in its setup() arguments.

> diff --git a/utils/scanpypi b/utils/scanpypi
> index 452b4a3fc3..a5522a879e 100755
> --- a/utils/scanpypi
> +++ b/utils/scanpypi
> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
>      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
> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> +        BuildrootPackage.setup_args[name] = kwargs
> +        BuildrootPackage.setup_args[name]['method'] = method
>      return closure
> 
>  # monkey patch
> @@ -147,6 +148,7 @@ class BuildrootPackage():
>          self.url = None
>          self.version = None
>          self.license_files = []
> +        self.setup_args['name'] = self.real_name

I'm not a big fan of shoehorning 'name' in setup_args, which as I
understand it is a dict that should contain a single key named after
the package. Perhaps it would be better to make "self.real_name" a
class variable rather than an instance variable, just like setup_args
is already, so that setup_decorator can access it. If I understand the
current implementation correctly, it anyway expects the
BuildrootPackage() class to have only one instance.

What do you think?

Best regards,

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 1/1] utils/scanpypi: supply package name to setup() when not provided

[James Hilliard]

On Sat, Aug 26, 2023 at 5:39 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Eric,
>
> +James in Cc.
>
> Sorry for the super long lag.
>
> On Sun, 18 Sep 2022 12:48:31 -0700
> erichiggins@gmail.com wrote:
>
> > Issue description:
> > The `utils/scanpypi` script makes an erroneous assumption that Python
> > packages will call `setup()` with the `name` argument. It's not
> > required and not often used. This causes the script to fail to load
> > many packages from Pypi.
> >  For example,  `./utils/scanpypi wheel` returns the following error:
> > > `Error: Could not install package wheel: 'name'`
>
> Do you have a current example that fails due to this? The wheel package
> does pass a name= attribute to its setup() function, and it has been
> doing this for many years. Looking at the initial commit of
> https://github.com/pypa/wheel:
>
> diff --git a/setup.py b/setup.py
> new file mode 100644
> index 0000000..09a138e
> --- /dev/null
> +++ b/setup.py
> @@ -0,0 +1,33 @@
> +import os
> +
> +from setuptools import setup
> +
> +here = os.path.abspath(os.path.dirname(__file__))
> +README = open(os.path.join(here, 'README.txt')).read()
> +CHANGES = open(os.path.join(here, 'CHANGES.txt')).read()
> +
> +setup(name='wheel',
> +      version='0.1',
> +      description='A built-package installer for Python.',
> +      long_description=README + '\n\n' +  CHANGES,
> +      classifiers=[
> +        "Development Status :: 1 - ",
> +        "Intended Audience :: Developers",
>
> So I'm a bit puzzled at how you found a wheel package that doesn't pass
> "name" in its setup() arguments.

There's been a lot of brokenness with how setuptools is handled in general
by scanpypi for a while here, I was planning to do some refactoring
after initial
pep517/flit support was merged by attempting to make use of pep517 hooks:
https://patchwork.ozlabs.org/project/buildroot/patch/20221128183230.1915592-1-james.hilliard1@gmail.com/

>
> > diff --git a/utils/scanpypi b/utils/scanpypi
> > index 452b4a3fc3..a5522a879e 100755
> > --- a/utils/scanpypi
> > +++ b/utils/scanpypi
> > @@ -58,8 +58,9 @@ def setup_decorator(func, method):
> >      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
> > +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> > +        BuildrootPackage.setup_args[name] = kwargs
> > +        BuildrootPackage.setup_args[name]['method'] = method
> >      return closure
> >
> >  # monkey patch
> > @@ -147,6 +148,7 @@ class BuildrootPackage():
> >          self.url = None
> >          self.version = None
> >          self.license_files = []
> > +        self.setup_args['name'] = self.real_name
>
> I'm not a big fan of shoehorning 'name' in setup_args, which as I
> understand it is a dict that should contain a single key named after
> the package. Perhaps it would be better to make "self.real_name" a
> class variable rather than an instance variable, just like setup_args
> is already, so that setup_decorator can access it. If I understand the
> current implementation correctly, it anyway expects the
> BuildrootPackage() class to have only one instance.
>
> What do you think?
>
> Best regards,
>
> 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 1/1] utils/scanpypi: supply package name to setup() when not provided

[erichiggins]

[-- Attachment #1.1: Type: text/plain, Size: 3752 bytes --]

Hi Thomas,

Responses inline below.



> > Issue description:
> > The `utils/scanpypi` script makes an erroneous assumption that Python
> > packages will call `setup()` with the `name` argument. It's not
> > required and not often used. This causes the script to fail to load
> > many packages from Pypi.
> >  For example,  `./utils/scanpypi wheel` returns the following error:
> > > `Error: Could not install package wheel: 'name'`
>
> Do you have a current example that fails due to this? The wheel package
> does pass a name= attribute to its setup() function, and it has been
> doing this for many years. Looking at the initial commit of
> https://github.com/pypa/wheel:
>
> diff --git a/setup.py b/setup.py
> new file mode 100644
> index 0000000..09a138e
> --- /dev/null
> +++ b/setup.py
> @@ -0,0 +1,33 @@
> +import os
> +
> +from setuptools import setup
> +
> +here = os.path.abspath(os.path.dirname(__file__))
> +README = open(os.path.join(here, 'README.txt')).read()
> +CHANGES = open(os.path.join(here, 'CHANGES.txt')).read()
> +
> +setup(name='wheel',
> +      version='0.1',
> +      description='A built-package installer for Python.',
> +      long_description=README + '\n\n' +  CHANGES,
> +      classifiers=[
> +        "Development Status :: 1 - ",
> +        "Intended Audience :: Developers",
>
> So I'm a bit puzzled at how you found a wheel package that doesn't pass
> "name" in its setup() arguments.
>


Perhaps now it does, but as of version 0.38.4 (released Nov 2022) it did
not.
https://github.com/pypa/wheel/blob/0.38.4/setup.py
The wheel package appears to have switched to use project.toml instead of
setup.py as of version 0.40.0

Regardless, the larger point remains. The `name` attribute is not required
and therefore should not be expected/enforced by buildroot's utilities.



> > diff --git a/utils/scanpypi b/utils/scanpypi
> > index 452b4a3fc3..a5522a879e 100755
> > --- a/utils/scanpypi
> > +++ b/utils/scanpypi
> > @@ -58,8 +58,9 @@ def setup_decorator(func, method):
> >      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
> > +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> > +        BuildrootPackage.setup_args[name] = kwargs
> > +        BuildrootPackage.setup_args[name]['method'] = method
> >      return closure
> >
> >  # monkey patch
> > @@ -147,6 +148,7 @@ class BuildrootPackage():
> >          self.url = None
> >          self.version = None
> >          self.license_files = []
> > +        self.setup_args['name'] = self.real_name
>
> I'm not a big fan of shoehorning 'name' in setup_args, which as I
> understand it is a dict that should contain a single key named after
> the package. Perhaps it would be better to make "self.real_name" a
> class variable rather than an instance variable, just like setup_args
> is already, so that setup_decorator can access it. If I understand the
> current implementation correctly, it anyway expects the
> BuildrootPackage() class to have only one instance.
>
> What do you think?
>

I'm agnostic about the implementation. I took the approach of making the
minimal viable change that resolved the issue given the existing framework
without introducing additional complexity. It sounds like James is planning
a refactor -- perhaps my patch will suffice to resolve the issue until that
happens?


>
> Best regards,
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com
>

[-- Attachment #1.2: Type: text/html, Size: 5276 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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

[Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[erichiggins]

Signed-off-by: Eric Higgins <erichiggins@gmail.com>
---
 utils/scanpypi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/utils/scanpypi b/utils/scanpypi
index 452b4a3fc3..a5522a879e 100755
--- a/utils/scanpypi
+++ b/utils/scanpypi
@@ -58,8 +58,9 @@ def setup_decorator(func, method):
     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
+        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
+        BuildrootPackage.setup_args[name] = kwargs
+        BuildrootPackage.setup_args[name]['method'] = method
     return closure

 # monkey patch
@@ -147,6 +148,7 @@ class BuildrootPackage():
         self.url = None
         self.version = None
         self.license_files = []
+        self.setup_args['name'] = self.real_name

     def fetch_package_info(self):
         """
-- 
2.25.1
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[Yann E. MORIN]

Eric, All,

+James for his expertise in that file

On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly:
> Signed-off-by: Eric Higgins <erichiggins@gmail.com>

Thanks for this patch.

However, this will need a bit more explanations in the commit log. Start
by describing the issue, explain why that happens, and how it is fixed.

You can get an idea of how to structure that by looking at existing
commit logs: git log utils/scanpypi

> ---
>  utils/scanpypi | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/scanpypi b/utils/scanpypi
> index 452b4a3fc3..a5522a879e 100755
> --- a/utils/scanpypi
> +++ b/utils/scanpypi
> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
>      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

So, this comment states that setup() is called with 'name' argument, but
what your commit title implies is that it is not always true. So, this
comment is now incorrect, and must be amended apropriately.

Could it be that sometimes, 'name' is a keyword argument, and in some
other case, it is just a positional argument?

> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> +        BuildrootPackage.setup_args[name] = kwargs
> +        BuildrootPackage.setup_args[name]['method'] = method
>      return closure
> 
>  # monkey patch
> @@ -147,6 +148,7 @@ class BuildrootPackage():
>          self.url = None
>          self.version = None
>          self.license_files = []
> +        self.setup_args['name'] = self.real_name

Otherwise, I do understand what the code does, and I think this is the
correct solution. James, your opinion?.

Still, what is missing is an explanation on why this change is needed.

Regards,
Yann E. MORIN.

>      def fetch_package_info(self):
>          """
> -- 
> 2.25.1
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[Marcus Hoffmann]

Yann, Eric,

On 12.09.22 22:34, Yann E. MORIN wrote:
> Eric, All,
> 
> +James for his expertise in that file
> 
> On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly:
>> Signed-off-by: Eric Higgins <erichiggins@gmail.com>
> 
> Thanks for this patch.
> 
> However, this will need a bit more explanations in the commit log. Start
> by describing the issue, explain why that happens, and how it is fixed.
> 
> You can get an idea of how to structure that by looking at existing
> commit logs: git log utils/scanpypi
> 
>> ---
>>   utils/scanpypi | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/utils/scanpypi b/utils/scanpypi
>> index 452b4a3fc3..a5522a879e 100755
>> --- a/utils/scanpypi
>> +++ b/utils/scanpypi
>> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
>>       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
> 
> So, this comment states that setup() is called with 'name' argument, but
> what your commit title implies is that it is not always true. So, this
> comment is now incorrect, and must be amended apropriately.
> 
> Could it be that sometimes, 'name' is a keyword argument, and in some
> other case, it is just a positional argument?

I can offer an example of where the existing script goes wrong:

https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py

No arguments passed at all. `name` (like anything else) in this case is 
read from the accompanying setup.cfg file (in medium-modern python 
packaging world):

https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2

This is explained for example here: 
https://towardsdatascience.com/setuptools-python-571e7d5500f2

In the even more modern world the same info is specified in 
pyproject.toml instead:

https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9

But I think it's easiest and correct to use the name specified on the 
cli instead for us.

> 
>> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
>> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
>> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
>> +        BuildrootPackage.setup_args[name] = kwargs
>> +        BuildrootPackage.setup_args[name]['method'] = method
>>       return closure
>>
>>   # monkey patch
>> @@ -147,6 +148,7 @@ class BuildrootPackage():
>>           self.url = None
>>           self.version = None
>>           self.license_files = []
>> +        self.setup_args['name'] = self.real_name
> 
> Otherwise, I do understand what the code does, and I think this is the
> correct solution. James, your opinion?.
> 
> Still, what is missing is an explanation on why this change is needed. >
> Regards,
> Yann E. MORIN.
> 
>>       def fetch_package_info(self):
>>           """
>> -- 
>> 2.25.1
>> _______________________________________________
>> buildroot mailing list
>> buildroot@buildroot.org
>> https://lists.buildroot.org/mailman/listinfo/buildroot
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[erichiggins]

[-- Attachment #1.1: Type: text/plain, Size: 3711 bytes --]

Yann,

I did a write up w/ the justification for this change in this Github gist
https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed

Hopefully that provides the necessary info, but I'm happy to copy/paste it
here if you need it for the mailing list record.

On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann <marcus.hoffmann@othermo.de>
wrote:

> Yann, Eric,
>
> On 12.09.22 22:34, Yann E. MORIN wrote:
> > Eric, All,
> >
> > +James for his expertise in that file
> >
> > On 2022-09-12 09:28 -0700, erichiggins@gmail.com spake thusly:
> >> Signed-off-by: Eric Higgins <erichiggins@gmail.com>
> >
> > Thanks for this patch.
> >
> > However, this will need a bit more explanations in the commit log. Start
> > by describing the issue, explain why that happens, and how it is fixed.
> >
> > You can get an idea of how to structure that by looking at existing
> > commit logs: git log utils/scanpypi
> >
> >> ---
> >>   utils/scanpypi | 6 ++++--
> >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/utils/scanpypi b/utils/scanpypi
> >> index 452b4a3fc3..a5522a879e 100755
> >> --- a/utils/scanpypi
> >> +++ b/utils/scanpypi
> >> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
> >>       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
> >
> > So, this comment states that setup() is called with 'name' argument, but
> > what your commit title implies is that it is not always true. So, this
> > comment is now incorrect, and must be amended apropriately.
> >
> > Could it be that sometimes, 'name' is a keyword argument, and in some
> > other case, it is just a positional argument?
>
> I can offer an example of where the existing script goes wrong:
>
>
> https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py
>
> No arguments passed at all. `name` (like anything else) in this case is
> read from the accompanying setup.cfg file (in medium-modern python
> packaging world):
>
>
> https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2
>
> This is explained for example here:
> https://towardsdatascience.com/setuptools-python-571e7d5500f2
>
> In the even more modern world the same info is specified in
> pyproject.toml instead:
>
>
> https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9
>
> But I think it's easiest and correct to use the name specified on the
> cli instead for us.
>
> >
> >> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
> >> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
> >> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> >> +        BuildrootPackage.setup_args[name] = kwargs
> >> +        BuildrootPackage.setup_args[name]['method'] = method
> >>       return closure
> >>
> >>   # monkey patch
> >> @@ -147,6 +148,7 @@ class BuildrootPackage():
> >>           self.url = None
> >>           self.version = None
> >>           self.license_files = []
> >> +        self.setup_args['name'] = self.real_name
> >
> > Otherwise, I do understand what the code does, and I think this is the
> > correct solution. James, your opinion?.
> >
> > Still, what is missing is an explanation on why this change is needed. >
> > Regards,
> > Yann E. MORIN.
> >
> >>       def fetch_package_info(self):
> >>           """
> >> --
> >> 2.25.1
> >> _______________________________________________
> >> buildroot mailing list
> >> buildroot@buildroot.org
> >> https://lists.buildroot.org/mailman/listinfo/buildroot
> >
>

[-- Attachment #1.2: Type: text/html, Size: 5762 bytes --]

[-- Attachment #2: Type: text/plain, Size: 150 bytes --]

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

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[Yann E. MORIN]

Eric, All,

On 2022-09-12 14:34 -0700, erichiggins@gmail.com spake thusly:
> Yann,
> I did a write up w/ the justification for this change in this Github gist
> [1]https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed
> Hopefully that provides the necessary info, but I'm happy to copy/paste it here if you need it for the mailing list record.

Please, resubmit this change with the appropriate explanations from your
gist if that makes sense, reformatted as a proper commit log;

 1. describe the issue
 2. explain why it happens
 3. explain how you fixed it (don't describe the code, explain it).

Regards,
Yann E. MORIN.

> On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann < [2]marcus.hoffmann@othermo.de> wrote:
> 
>   Yann, Eric,
> 
>   On 12.09.22 22:34, Yann E. MORIN wrote:
>   > Eric, All,
>   >
>   > +James for his expertise in that file
>   >
>   > On 2022-09-12 09:28 -0700, [3]erichiggins@gmail.com spake thusly:
>   >> Signed-off-by: Eric Higgins < [4]erichiggins@gmail.com>
>   >
>   > Thanks for this patch.
>   >
>   > However, this will need a bit more explanations in the commit log. Start
>   > by describing the issue, explain why that happens, and how it is fixed.
>   >
>   > You can get an idea of how to structure that by looking at existing
>   > commit logs: git log utils/scanpypi
>   >
>   >> ---
>   >>   utils/scanpypi | 6 ++++--
>   >>   1 file changed, 4 insertions(+), 2 deletions(-)
>   >>
>   >> diff --git a/utils/scanpypi b/utils/scanpypi
>   >> index 452b4a3fc3..a5522a879e 100755
>   >> --- a/utils/scanpypi
>   >> +++ b/utils/scanpypi
>   >> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
>   >>       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
>   >
>   > So, this comment states that setup() is called with 'name' argument, but
>   > what your commit title implies is that it is not always true. So, this
>   > comment is now incorrect, and must be amended apropriately.
>   >
>   > Could it be that sometimes, 'name' is a keyword argument, and in some
>   > other case, it is just a positional argument?
> 
>   I can offer an example of where the existing script goes wrong:
> 
>   [5]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py
> 
>   No arguments passed at all. `name` (like anything else) in this case is
>   read from the accompanying setup.cfg file (in medium-modern python
>   packaging world):
> 
>   [6]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2
> 
>   This is explained for example here:
>   [7]https://towardsdatascience.com/setuptools-python-571e7d5500f2
> 
>   In the even more modern world the same info is specified in
>   pyproject.toml instead:
> 
>   [8]https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9
> 
>   But I think it's easiest and correct to use the name specified on the
>   cli instead for us.
> 
>   >
>   >> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
>   >> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
>   >> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
>   >> +        BuildrootPackage.setup_args[name] = kwargs
>   >> +        BuildrootPackage.setup_args[name]['method'] = method
>   >>       return closure
>   >>
>   >>   # monkey patch
>   >> @@ -147,6 +148,7 @@ class BuildrootPackage():
>   >>           self.url = None
>   >>           self.version = None
>   >>           self.license_files = []
>   >> +        self.setup_args['name'] = self.real_name
>   >
>   > Otherwise, I do understand what the code does, and I think this is the
>   > correct solution. James, your opinion?.
>   >
>   > Still, what is missing is an explanation on why this change is needed. >
>   > Regards,
>   > Yann E. MORIN.
>   >
>   >>       def fetch_package_info(self):
>   >>           """
>   >> --
>   >> 2.25.1
>   >> _______________________________________________
>   >> buildroot mailing list
>   >> [9]buildroot@buildroot.org
>   >> [10]https://lists.buildroot.org/mailman/listinfo/buildroot
>   >
> 
> Links:
> 1. https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed
> 2. mailto:marcus.hoffmann@othermo.de
> 3. mailto:erichiggins@gmail.com
> 4. mailto:erichiggins@gmail.com
> 5. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py
> 6. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2
> 7. https://towardsdatascience.com/setuptools-python-571e7d5500f2
> 8. https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9
> 9. mailto:buildroot@buildroot.org
> 10. https://lists.buildroot.org/mailman/listinfo/buildroot

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


-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH 1/1] utils/scanpypi: supply package name to setup() when not provided

[erichiggins]

🙄 fine.

On Sun, Sep 18, 2022 at 9:00 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote:
>
> Eric, All,
>
> On 2022-09-12 14:34 -0700, erichiggins@gmail.com spake thusly:
> > Yann,
> > I did a write up w/ the justification for this change in this Github gist
> > [1]https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed
> > Hopefully that provides the necessary info, but I'm happy to copy/paste it here if you need it for the mailing list record.
>
> Please, resubmit this change with the appropriate explanations from your
> gist if that makes sense, reformatted as a proper commit log;
>
>  1. describe the issue
>  2. explain why it happens
>  3. explain how you fixed it (don't describe the code, explain it).
>
> Regards,
> Yann E. MORIN.
>
> > On Mon, Sep 12, 2022 at 2:05 PM Marcus Hoffmann < [2]marcus.hoffmann@othermo.de> wrote:
> >
> >   Yann, Eric,
> >
> >   On 12.09.22 22:34, Yann E. MORIN wrote:
> >   > Eric, All,
> >   >
> >   > +James for his expertise in that file
> >   >
> >   > On 2022-09-12 09:28 -0700, [3]erichiggins@gmail.com spake thusly:
> >   >> Signed-off-by: Eric Higgins < [4]erichiggins@gmail.com>
> >   >
> >   > Thanks for this patch.
> >   >
> >   > However, this will need a bit more explanations in the commit log. Start
> >   > by describing the issue, explain why that happens, and how it is fixed.
> >   >
> >   > You can get an idea of how to structure that by looking at existing
> >   > commit logs: git log utils/scanpypi
> >   >
> >   >> ---
> >   >>   utils/scanpypi | 6 ++++--
> >   >>   1 file changed, 4 insertions(+), 2 deletions(-)
> >   >>
> >   >> diff --git a/utils/scanpypi b/utils/scanpypi
> >   >> index 452b4a3fc3..a5522a879e 100755
> >   >> --- a/utils/scanpypi
> >   >> +++ b/utils/scanpypi
> >   >> @@ -58,8 +58,9 @@ def setup_decorator(func, method):
> >   >>       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
> >   >
> >   > So, this comment states that setup() is called with 'name' argument, but
> >   > what your commit title implies is that it is not always true. So, this
> >   > comment is now incorrect, and must be amended apropriately.
> >   >
> >   > Could it be that sometimes, 'name' is a keyword argument, and in some
> >   > other case, it is just a positional argument?
> >
> >   I can offer an example of where the existing script goes wrong:
> >
> >   [5]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py
> >
> >   No arguments passed at all. `name` (like anything else) in this case is
> >   read from the accompanying setup.cfg file (in medium-modern python
> >   packaging world):
> >
> >   [6]https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2
> >
> >   This is explained for example here:
> >   [7]https://towardsdatascience.com/setuptools-python-571e7d5500f2
> >
> >   In the even more modern world the same info is specified in
> >   pyproject.toml instead:
> >
> >   [8]https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9
> >
> >   But I think it's easiest and correct to use the name specified on the
> >   cli instead for us.
> >
> >   >
> >   >> -        BuildrootPackage.setup_args[kwargs['name']] = kwargs
> >   >> -        BuildrootPackage.setup_args[kwargs['name']]['method'] = method
> >   >> +        name = kwargs.get('name', BuildrootPackage.setup_args['name'])
> >   >> +        BuildrootPackage.setup_args[name] = kwargs
> >   >> +        BuildrootPackage.setup_args[name]['method'] = method
> >   >>       return closure
> >   >>
> >   >>   # monkey patch
> >   >> @@ -147,6 +148,7 @@ class BuildrootPackage():
> >   >>           self.url = None
> >   >>           self.version = None
> >   >>           self.license_files = []
> >   >> +        self.setup_args['name'] = self.real_name
> >   >
> >   > Otherwise, I do understand what the code does, and I think this is the
> >   > correct solution. James, your opinion?.
> >   >
> >   > Still, what is missing is an explanation on why this change is needed. >
> >   > Regards,
> >   > Yann E. MORIN.
> >   >
> >   >>       def fetch_package_info(self):
> >   >>           """
> >   >> --
> >   >> 2.25.1
> >   >> _______________________________________________
> >   >> buildroot mailing list
> >   >> [9]buildroot@buildroot.org
> >   >> [10]https://lists.buildroot.org/mailman/listinfo/buildroot
> >   >
> >
> > Links:
> > 1. https://gist.github.com/erichiggins/223b2495ada64001c44deedd3a3df6ed
> > 2. mailto:marcus.hoffmann@othermo.de
> > 3. mailto:erichiggins@gmail.com
> > 4. mailto:erichiggins@gmail.com
> > 5. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.py
> > 6. https://github.com/Pylons/waitress/blob/73fe701cba0b37dc6099858b064cb1e755e83e9e/setup.cfg#L2
> > 7. https://towardsdatascience.com/setuptools-python-571e7d5500f2
> > 8. https://github.com/agronholm/apscheduler/blob/49344e6954559259beb336436a45698d62eed5b4/pyproject.toml#L2-L9
> > 9. mailto:buildroot@buildroot.org
> > 10. https://lists.buildroot.org/mailman/listinfo/buildroot
>
> > _______________________________________________
> > buildroot mailing list
> > buildroot@buildroot.org
> > https://lists.buildroot.org/mailman/listinfo/buildroot
>
>
> --
> .-----------------.--------------------.------------------.--------------------.
> |  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
> | +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
> | +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
> | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
> '------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot