Re: [Buildroot] [PATCH 1/1] package/python-msgpack: add host cython dependency

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

Hello James,

On Mon, 10 Jul 2023 14:17:55 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> > Could you clarify if this is a current issue in the python-msgpack
> > package that causes build failures, or if this is in preparation to
> > move from setuptools to pep517 to build python-msgpack?  
> 
> It's a hard error when moving setuptools to pep517 but should be applied
> regardless as it's needed for optimized extensions to build properly AFAIU
> even when using legacy setup.py builds.

So this patch is a prerequisite to applying
https://patchwork.ozlabs.org/project/buildroot/patch/20230626181531.2312002-5-james.hilliard1@gmail.com/ ?

Why isn't it part of the same series, then?

What is an "optimized extension" ? Could you clarify in which case
python-msgpack wouldn't build/work, ourside of the PEP517 migration?

> > If you want us to merge your patches faster, please help us: explain in
> > the commit log the "WHY" you are doing the change. Your commit log
> > fails to explain it, and therefore I don't have the context to
> > understand the motivation of the change.  
> 
> I did mention in the commit log that this fixes an error which occurs when
> using a pep517 frontend, which is what the patch doing the setuptools
> pep517 migration changes, I'm not sure what context is missing here.

The context that is missing is that this patch (changing
python-msgpack) comes completely isolated from any other patch. If it
had been in the series that ends with the "package/pkg-python.mk:
migrate setuptools to pep517" patch, then it would have been clear:
it's a pre-requisite to be able to do this move to PEP517.

> We already use a pep517 frontend for other packages(i.e. packages using flit
> infrastructure) so I was assuming familiarity with pep517 build
> frontends/backends
> here. The pep517 documentation has additional background on how this all works.
> 
> https://peps.python.org/pep-0517/
> 
> This should be merged before the pep517 migration, I had only discovered
> this issue after so I had sent it as a follow up instead of within the
> initial pep517 setuptools migration series.

No, what you should have done is resend an updated PEP517 setuptools
migration series. This is what makes things clear for the
maintainers/reviewers. It's totally fine to miss things in the first
iteration of the series, but if you discover missing things, you should
NOT send separate standalone patches. Instead you should send a new
iteration of the series that includes the additional changes.

Could you have a look at resending a complete series that include all
your changes related to PEP517 setuptools migration, with a proper
cover letter that describes the goal and the path taken to reach this
goal?

This would *tremendously* help the work of the maintainers/reviewers.

Thanks a lot for all your work on the Python integration, it is really
much appreciated!

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