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

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

[James Hilliard]

We need host-python-cython for python-msgpack to build correctly.
This is a hard error when using a pep517 frontend.

Fixes:
* Getting build dependencies for wheel...
running egg_info
writing msgpack.egg-info/PKG-INFO
writing dependency_links to msgpack.egg-info/dependency_links.txt
writing top-level names to msgpack.egg-info/top_level.txt
reading manifest file 'msgpack.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching '*.c' under directory 'msgpack'
adding license file 'COPYING'
writing manifest file 'msgpack.egg-info/SOURCES.txt'

ERROR Missing dependencies:
	Cython~=0.29.30

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 package/python-msgpack/python-msgpack.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/python-msgpack/python-msgpack.mk b/package/python-msgpack/python-msgpack.mk
index d9304e4def..5241416fb1 100644
--- a/package/python-msgpack/python-msgpack.mk
+++ b/package/python-msgpack/python-msgpack.mk
@@ -10,5 +10,6 @@ PYTHON_MSGPACK_SITE = https://files.pythonhosted.org/packages/22/44/0829b19ac243
 PYTHON_MSGPACK_LICENSE = Apache-2.0
 PYTHON_MSGPACK_LICENSE_FILES = COPYING
 PYTHON_MSGPACK_SETUP_TYPE = setuptools
+PYTHON_MSGPACK_DEPENDENCIES = host-python-cython
 
 $(eval $(python-package))
-- 
2.34.1

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

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

[Thomas Petazzoni via buildroot]

On Mon, 26 Jun 2023 02:00:16 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> We need host-python-cython for python-msgpack to build correctly.
> This is a hard error when using a pep517 frontend.
> 
> Fixes:
> * Getting build dependencies for wheel...
> running egg_info
> writing msgpack.egg-info/PKG-INFO
> writing dependency_links to msgpack.egg-info/dependency_links.txt
> writing top-level names to msgpack.egg-info/top_level.txt
> reading manifest file 'msgpack.egg-info/SOURCES.txt'
> reading manifest template 'MANIFEST.in'
> warning: no files found matching '*.c' under directory 'msgpack'
> adding license file 'COPYING'
> writing manifest file 'msgpack.egg-info/SOURCES.txt'
> 
> ERROR Missing dependencies:
> 	Cython~=0.29.30

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?

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.

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 1/1] package/python-msgpack: add host cython dependency

[James Hilliard]

On Mon, Jul 10, 2023 at 12:03 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 26 Jun 2023 02:00:16 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > We need host-python-cython for python-msgpack to build correctly.
> > This is a hard error when using a pep517 frontend.
> >
> > Fixes:
> > * Getting build dependencies for wheel...
> > running egg_info
> > writing msgpack.egg-info/PKG-INFO
> > writing dependency_links to msgpack.egg-info/dependency_links.txt
> > writing top-level names to msgpack.egg-info/top_level.txt
> > reading manifest file 'msgpack.egg-info/SOURCES.txt'
> > reading manifest template 'MANIFEST.in'
> > warning: no files found matching '*.c' under directory 'msgpack'
> > adding license file 'COPYING'
> > writing manifest file 'msgpack.egg-info/SOURCES.txt'
> >
> > ERROR Missing dependencies:
> >       Cython~=0.29.30
>
> 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.

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

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

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.

>
> 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 1/1] package/python-msgpack: add host cython dependency

[Thomas Petazzoni via buildroot]

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

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

[James Hilliard]

On Tue, Jul 11, 2023 at 1:38 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> 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?

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
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

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

[Thomas Petazzoni via buildroot]

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

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

[James Hilliard]

On Sat, Aug 26, 2023 at 6:17 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> 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.

Sure, it's not strictly needed but it's AFAIU intended as a build dependency
by the maintainers of msgpack-python.

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

Using pre-cythonized sources usually works but can result in suboptimal/broken
builds in some cases due to generated sources having been built with an old
version of cython, we've had issues in the past with pre-cythonized sources
that break across python version updates.

For example:
https://github.com/buildroot/buildroot/commit/6833dd5079b8017bde37b7d3ef201d35450a12e6

I'm not entirely sure the fallback logic forces updated sources to be
generated if
prebuilt sources are present as the fallback logic is a bit complex
here but it's
probably an upstream bug if that's not happening as one typically does want
updated cythonized sources where possible.

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

Sure, I'll try and add more details when I'm aware of them.

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

Keep in mind that in this case I wasn't even aware that this was a redundant
dependency as I hadn't dug that deeply into the cythonize fallback logic and
was merely adding the dependency based on the upstream build dependency
configurations. I hadn't really questioned the accuracy of this dependency being
included as a build dependency by upstream.

I generally just assume what the upstream packages specify is correct as
far as specified build dependencies goes as it's typically non-trivial to double
check this sort of thing(due to having to examine in detail the fallback logic).

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

This is a bit of an unusual case IMO and I'm still not 100% sure that this
wouldn't make sense to apply to non-pep517 builds(due to having potential
benefits from using cythonized sources generated by a newer version of cython).

In any case once we've migrated setuptools to pep517 it will be more clear
what build dependencies are actually required as they will be enforced by the
pep517 frontend more strictly.

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

I'll follow up there in more detail but I don't think it makes sense
to try to avoid
a flag day in this case.

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