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

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

Hello James,

On Tue, 11 Jul 2023 03:35:49 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

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

So upon further investigation, in fact the host-python-cython
dependency is not needed when we stick to legacy style setuptools. So
in other words, this patch we're discussing does not make sense outside
of the PEP517 setuptools migration, which further confirms my initial
request that this patch should be part of the patch series doing the
PEP517 setuptools conversion.

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

See above: it's not a missing dependency bug, and outside of the pep517
migration, the proposed change is not useful/relevant. It just adds
another dependency (increasing the build time) with no
need/justification.

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

The thing is that it's mainly the maintainers duty to review changes,
and so your preference in terms of what is easy/difficult to review
here does not really matter. As maintainers, we tell you that the way
you're submitting those patches make it difficult for us, because the
patches come isolated, with no explanation as to how they fit in the big
picture.

If you insist to send isolated patches, then you need each patch to
have a very extensive and detailed commit log that allows us to
understand how it fits in the big picture.

This whole discussion on the python-msgpack dependency on
host-python-cython is a good illustration of that.

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

That is correct, but not for this python-msgpack patch.

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

See my reply to your PEP517 setuptools infrastructure change, where I
suggest that maybe we need to find a solution that doesn't require a
flag day.

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