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

Tue Jul 11 09:35:49 UTC 2023

On Tue, Jul 11, 2023 at 1:38 AM Thomas Petazzoni
<thomas.petazzoni at bootlin.com> wrote:
>
> Hello James,
>
> On Mon, 10 Jul 2023 14:17:55 -0600
> James Hilliard <james.hilliard1 at 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?

I had noticed the issue after sending that series and the issue seemed
to be a bug that should be fixed even without the pep517 migration.

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

Well it's using some fallback logic in setup.py:
https://github.com/msgpack/msgpack-python/blob/v1.0.5/setup.py#L20-L25

Although looking at it again it appears the sdist does have pre-cythonized
sources as a fallback which looks to be the reason that not fixing this
previously worked(I presume due to legacy style setuptools not having a
reliable way for specifying build dependencies).

In any case the intention of the maintainers of msgpack-python does appear
to be that one should have cython as a build dependency when possible as
they are specifying it in the pyproject.toml file and requirements.txt.

https://github.com/msgpack/msgpack-python/blob/v1.0.5/pyproject.toml#L5
https://github.com/msgpack/msgpack-python/blob/v1.0.5/requirements.txt#L2

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

I mean, it's just a missing dependency bug that was revealed by the
pep517 migration, it can be reviewed/merged entirely separately from
the actual pep517 migration patch(although it should be merged first).

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

I don't really have a good workflow for managing a large patch series so
I tend to try and keep things more separated out unless I see a good
reason for the changes to be kept together(ie if there is a need for them
to be reviewed at the same time).

I personally find it a lot harder to review changes when they are mixed
into a large series as that often reduces the signal to noise ratio.

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

I'm not really sure what I should add in a cover letter, all the changes
in the series prior to the final pep517 migration patch are just package
version updates. They would be the same whether or not we were
planning on migrating setuptools to pep517.

It's true that they do need to be merged prior to the setuptools pep517
migration but it's not necessary to even be aware of the final setuptools
pep517 migration patch to review all the prior version bump patches.

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

It seems like a lot of extra review/work to track/explain in detail the
motivation for package version bump prerequisites like this. The only
reason for this even being a series at all is simply for dependency ordering.

Maybe I should just send patches like version bumps and such and
wait for them to be merged before sending a dependent change like the
final pep517 migration patch? I already try to do this most of the time to
keep a patch series from getting too large(having a large series IMO makes
review/rebasing a lot harder at least for myself compared to separate
patches). Large patch series also tend to be more likely to get lost in the
backlog compared with more straightforward version bump patches
which further increases the need for rebasing.

In fact this pep517 migration patch depends upon many other version
bump patches and similar changes I made that have already been merged
as largely independent patches.

I just hadn't deffered the final pep517 migration patch this time as there
were only a few version bumps remaining and someone had brought up
the setup.py deprecation warning being emitted so I figured I'd get something
ready to test.

https://lore.kernel.org/buildroot/1a3add30-eff5-9dfa-c4d9-3ca9122a6f08@smile.fr/

IMO it makes more sense here to just defer this final patch and review
all the others as if it was not sent yet as none of the other patches
depend on the pep517 migration patch at all.

https://patchwork.ozlabs.org/project/buildroot/patch/20230626181531.2312002-5-james.hilliard1@gmail.com/

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