Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend

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

Hello James,

On Tue, 11 Jul 2023 04:50:23 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> > This is a bad idea, which causes even more confusion, as we don't
> > understand *why* you're making those changes.  
> 
> The build system is supposed to be poetry/poetry-core, not setuptools, poetry
> is the correct backend specified in the pyproject.toml, the sdist has
> a setuptools
> shim for backwards compatibility apparently, this shim isn't even checked into
> version control for the project so we shouldn't be using it as it's
> not really the
> correct way to be building a poetry based package.

This is the information that I would like to see in the commit log.
Right now this series only looks like useless change: we have a package
that works, you're suggesting to make some changes, but we don't
understand why.

You should probably realize that we are not in your mind, we don't know
what your "agenda" is and why you're proposing certain changes. So try
to put yourself in our position: what are the relevant information that
the maintainers will need to understand the motivation for my change,
and the implementation of it. And while doing it, you should consider
that we (maintainers, or at least, me personally) are relatively
stupid, so adding more context than less context is very relevant.

I think you have an "agenda" of where you want to see Buildroot
Python's support to go, and this is excellent. We really, truly value
this kind of long-term contributions that improve Buildroot. However,
to make this work, you need to communicate this agenda more clearly:
what are the mid/long-term goals, and how the immediate contributions
fit within those mid/long-term goals. This will tremendously help us
understand where you're going and buy your contributions :-)

> > Could you instead do what we suggest, i.e resend one single full series
> > that include all changes, including the final change of the PEP517
> > setuptools migration, together with a cover letter?  
> 
> I mean, this is just a bug that didn't get noticed earlier, I just hadn't caught
> it before I did my pep517 migration, the fix is the same whether or not
> the pep517 patch is merged.

OK. It isn't clear in your commit log which "bug" is that, and what is
the impact. For example, is this a "bug" serious enough that the fix
needs to be backported to our LTS branch?

> I've sent a bunch of patches that effectively do the same thing as this one
> which have been merged independently, so other than the dependency
> ordering I don't see how this is related to the setuptools pep517 migration
> patch. To me it seems that I accidentally created a bunch of unnecessary
> confusion by sending my setuptools pep517 migration patch too early when
> I should have just waited until dependency fixes like this were merged.

Well, your commit log says:

"""
We need to migrate python-terminaltables to the pep517 poetry-core
backend as setuptools is not supported.
"""

You say that setuptools is not supported, but it is clearly incorrect,
as the current package builds fine with setuptools. So my reading of
the commit log was that it was needed for the upcoming PEP517
setuptools migration.

> I'm kind of pushing back a bit here since making a giant patch series would
> just end up making reviewing/rebasing a lot more confusing IMO.

It's really not, as it allows us to see the big picture. And when there
is a series of 20 patches that we're not yet ready to merge in full, we
do merge the first patches to reduce the backlog on the submitter side.
But seeing the full patch series allows us to understand the big
picture.

Alternatively, just make it very very clear in the commit log why this
preliminary change is needed, and how it fits in the big picture of
where you're going.

> If this was a change that couldn't be justified independently I would agree
> that it should be in a series with others.

As it is justified today in its current commit log, it really doesn't
appear like it is justified independently.

> I think there also might be something wrong with how I'm managing my
> patch series with git as they seem to be significantly easier for others to
> manage. Every time I deal with a large series it feels like it's a lot harder
> than it should be.

Managing a large patch series requires a bit of effort indeed, but
shouldn't be that difficult. I suppose you are efficiently/aggressively
using "git rebase -i" to rework/rebase your stack of patches?

As said above, I think it all boils down to providing more context in
the commit logs as to why a change is needed and proposed. This will
help us a lot, and will in the end reduce the workload on your side
because your patches will be merged much faster, and with less back and
forth over e-mail to collect the missing information from you.

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