[Buildroot] [PATCH 1/2] package/python-poetry-core: new host package

[Buildroot] [PATCH 1/2] package/python-poetry-core: new host package

[James Hilliard]

We need to add a patch to prevent poetry core from matching a bogus
git directory when determining which files should be ignored.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 ...atching-bogus-parent-git-directories.patch | 49 +++++++++++++++++++
 .../python-poetry-core.hash                   |  5 ++
 .../python-poetry-core/python-poetry-core.mk  | 14 ++++++
 3 files changed, 68 insertions(+)
 create mode 100644 package/python-poetry-core/0001-Prevent-matching-bogus-parent-git-directories.patch
 create mode 100644 package/python-poetry-core/python-poetry-core.hash
 create mode 100644 package/python-poetry-core/python-poetry-core.mk

diff --git a/package/python-poetry-core/0001-Prevent-matching-bogus-parent-git-directories.patch b/package/python-poetry-core/0001-Prevent-matching-bogus-parent-git-directories.patch
new file mode 100644
index 0000000000..6b52ff9612
--- /dev/null
+++ b/package/python-poetry-core/0001-Prevent-matching-bogus-parent-git-directories.patch
@@ -0,0 +1,49 @@
+From 77924f1347d773bb3d60f5351045527377489984 Mon Sep 17 00:00:00 2001
+From: James Hilliard <james.hilliard1@gmail.com>
+Date: Mon, 26 Jun 2023 23:45:45 -0600
+Subject: [PATCH] Prevent matching bogus parent git directories
+
+Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
+[Upstream status:
+https://github.com/python-poetry/poetry-core/pull/611]
+---
+ src/poetry/core/vcs/__init__.py | 21 ++++++++++++++-------
+ 1 file changed, 14 insertions(+), 7 deletions(-)
+
+diff --git a/src/poetry/core/vcs/__init__.py b/src/poetry/core/vcs/__init__.py
+index f4096ec..b4899b3 100644
+--- a/src/poetry/core/vcs/__init__.py
++++ b/src/poetry/core/vcs/__init__.py
+@@ -17,15 +17,22 @@ def get_vcs(directory: Path) -> Git | None:
+     try:
+         from poetry.core.vcs.git import executable
+ 
+-        git_dir = (
+-            subprocess.check_output(
+-                [executable(), "rev-parse", "--show-toplevel"], stderr=subprocess.STDOUT
++        check_ignore = subprocess.run(
++            [executable(), "check-ignore", "."],
++        ).returncode
++
++        if check_ignore == 0:
++            vcs = None
++        else:
++            git_dir = (
++                subprocess.check_output(
++                    [executable(), "rev-parse", "--show-toplevel"], stderr=subprocess.STDOUT
++                )
++                .decode()
++                .strip()
+             )
+-            .decode()
+-            .strip()
+-        )
+ 
+-        vcs = Git(Path(git_dir))
++            vcs = Git(Path(git_dir))
+ 
+     except (subprocess.CalledProcessError, OSError, RuntimeError):
+         vcs = None
+-- 
+2.41.0
+
diff --git a/package/python-poetry-core/python-poetry-core.hash b/package/python-poetry-core/python-poetry-core.hash
new file mode 100644
index 0000000000..5e62cb1aaf
--- /dev/null
+++ b/package/python-poetry-core/python-poetry-core.hash
@@ -0,0 +1,5 @@
+# md5, sha256 from https://pypi.org/pypi/poetry-core/json
+md5  37e1a9d3d3c97c9670aed62108f2d5cb  poetry_core-1.6.1.tar.gz
+sha256  0f9b0de39665f36d6594657e7d57b6f463cc10f30c28e6d1c3b9ff54c26c9ac3  poetry_core-1.6.1.tar.gz
+# Locally computed sha256 checksums
+sha256  f1978133782b90f4733bc308ddb19267c3fe04797c88d9ed3bc219032495a982  LICENSE
diff --git a/package/python-poetry-core/python-poetry-core.mk b/package/python-poetry-core/python-poetry-core.mk
new file mode 100644
index 0000000000..05d321a54d
--- /dev/null
+++ b/package/python-poetry-core/python-poetry-core.mk
@@ -0,0 +1,14 @@
+################################################################################
+#
+# python-poetry-core
+#
+################################################################################
+
+PYTHON_POETRY_CORE_VERSION = 1.6.1
+PYTHON_POETRY_CORE_SOURCE = poetry_core-$(PYTHON_POETRY_CORE_VERSION).tar.gz
+PYTHON_POETRY_CORE_SITE = https://files.pythonhosted.org/packages/20/e8/e0a80cc355bc207fb1760160344e978f39d683c35e1230f71b8916bf3a50
+PYTHON_POETRY_CORE_SETUP_TYPE = pep517
+PYTHON_POETRY_CORE_LICENSE = MIT
+PYTHON_POETRY_CORE_LICENSE_FILES = LICENSE
+
+$(eval $(host-python-package))
-- 
2.34.1

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

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

[James Hilliard]

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

Upstream has merged a patch replacing poetry with poetry-core, however
we can not backport this using a patch file due to CRLF line ending
issues so we will have to apply the change in the patch using sed
instead.

See upstream commit:
https://github.com/matthewdeanmartin/terminaltables/commit/9e3dda0efb54fee6934c744a13a7336d24c6e9e9

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 package/python-terminaltables/python-terminaltables.mk | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/package/python-terminaltables/python-terminaltables.mk b/package/python-terminaltables/python-terminaltables.mk
index b31ed332b6..385c71ae2d 100644
--- a/package/python-terminaltables/python-terminaltables.mk
+++ b/package/python-terminaltables/python-terminaltables.mk
@@ -7,8 +7,16 @@
 PYTHON_TERMINALTABLES_VERSION = 3.1.10
 PYTHON_TERMINALTABLES_SOURCE = terminaltables-$(PYTHON_TERMINALTABLES_VERSION).tar.gz
 PYTHON_TERMINALTABLES_SITE = https://files.pythonhosted.org/packages/f5/fc/0b73d782f5ab7feba8d007573a3773c58255f223c5940a7b7085f02153c3
-PYTHON_TERMINALTABLES_SETUP_TYPE = setuptools
+PYTHON_TERMINALTABLES_SETUP_TYPE = pep517
 PYTHON_TERMINALTABLES_LICENSE = MIT
 PYTHON_TERMINALTABLES_LICENSE_FILES = LICENSE
+PYTHON_TERMINALTABLES_DEPENDENCIES = host-python-poetry-core
+
+# we can't use a normal patch file due to different line endings
+define PYTHON_TERMINALTABLES_USE_POETRY_CORE
+	$(SED) 's/requires = \["poetry>=0.12"\]/requires = \["poetry-core>=1.0.0"\]/' $(@D)/pyproject.toml
+	$(SED) 's/build-backend = "poetry.masonry.api"/build-backend = "poetry.core.masonry.api"/' $(@D)/pyproject.toml
+endef
+PYTHON_TERMINALTABLES_POST_PATCH_HOOKS += PYTHON_TERMINALTABLES_USE_POETRY_CORE
 
 $(eval $(python-package))
-- 
2.34.1

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

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

[Thomas Petazzoni via buildroot]

Hello James,

On Tue, 27 Jun 2023 01:26:40 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> We need to migrate python-terminaltables to the pep517 poetry-core
> backend as setuptools is not supported.
> 
> Upstream has merged a patch replacing poetry with poetry-core, however
> we can not backport this using a patch file due to CRLF line ending
> issues so we will have to apply the change in the patch using sed
> instead.
> 
> See upstream commit:
> https://github.com/matthewdeanmartin/terminaltables/commit/9e3dda0efb54fee6934c744a13a7336d24c6e9e9
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>

What is the motivation to do this, if the current version of
python-terminaltables builds fine with setuptools?

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 2/2] package/python-terminaltables: fix build backend

[James Hilliard]

On Mon, Jul 10, 2023 at 12:01 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello James,
>
> On Tue, 27 Jun 2023 01:26:40 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > We need to migrate python-terminaltables to the pep517 poetry-core
> > backend as setuptools is not supported.
> >
> > Upstream has merged a patch replacing poetry with poetry-core, however
> > we can not backport this using a patch file due to CRLF line ending
> > issues so we will have to apply the change in the patch using sed
> > instead.
> >
> > See upstream commit:
> > https://github.com/matthewdeanmartin/terminaltables/commit/9e3dda0efb54fee6934c744a13a7336d24c6e9e9
> >
> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
>
> What is the motivation to do this, if the current version of
> python-terminaltables builds fine with setuptools?

It doesn't build in combination with pep517 setuptools since the pyproject.toml
specifies poetry as the build backend and not setuptools.

>
> 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 2/2] package/python-terminaltables: fix build backend

[Thomas Petazzoni via buildroot]

On Mon, 10 Jul 2023 13:58:38 -0600
James Hilliard <james.hilliard1@gmail.com> wrote:

> > > We need to migrate python-terminaltables to the pep517 poetry-core
> > > backend as setuptools is not supported.
> > >
> > > Upstream has merged a patch replacing poetry with poetry-core, however
> > > we can not backport this using a patch file due to CRLF line ending
> > > issues so we will have to apply the change in the patch using sed
> > > instead.
> > >
> > > See upstream commit:
> > > https://github.com/matthewdeanmartin/terminaltables/commit/9e3dda0efb54fee6934c744a13a7336d24c6e9e9
> > >
> > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>  
> >
> > What is the motivation to do this, if the current version of
> > python-terminaltables builds fine with setuptools?  
> 
> It doesn't build in combination with pep517 setuptools since the pyproject.toml
> specifies poetry as the build backend and not setuptools.

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

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 2/2] package/python-terminaltables: fix build backend

[James Hilliard]

On Tue, Jul 11, 2023 at 1:32 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Mon, 10 Jul 2023 13:58:38 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > > > We need to migrate python-terminaltables to the pep517 poetry-core
> > > > backend as setuptools is not supported.
> > > >
> > > > Upstream has merged a patch replacing poetry with poetry-core, however
> > > > we can not backport this using a patch file due to CRLF line ending
> > > > issues so we will have to apply the change in the patch using sed
> > > > instead.
> > > >
> > > > See upstream commit:
> > > > https://github.com/matthewdeanmartin/terminaltables/commit/9e3dda0efb54fee6934c744a13a7336d24c6e9e9
> > > >
> > > > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > >
> > > What is the motivation to do this, if the current version of
> > > python-terminaltables builds fine with setuptools?
> >
> > It doesn't build in combination with pep517 setuptools since the pyproject.toml
> > specifies poetry as the build backend and not setuptools.
>
> So is this patch series modifying python-terminaltables a prerequisite
> to applying the patch at
> https://patchwork.ozlabs.org/project/buildroot/patch/20230626181531.2312002-5-james.hilliard1@gmail.com/ ?

Yes, it should be applied before that one.

>
> 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 2/2] package/python-terminaltables: fix build backend

[Thomas Petazzoni via buildroot]

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

> > So is this patch series modifying python-terminaltables a prerequisite
> > to applying the patch at
> > https://patchwork.ozlabs.org/project/buildroot/patch/20230626181531.2312002-5-james.hilliard1@gmail.com/ ?  
> 
> Yes, it should be applied before that one.

Then, as explained in another e-mail, it should be part of the same
series. Yes, I understand that you sent the series before noticing that
python-terminaltables needed to be changed, and that's fine: send a new
complete iteration of the series with the extra patches.

And for such complicated series, please *always* add a cover letter
that gives some background: what you're trying to do, and what is the
path taken to achieve this.

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 2/2] package/python-terminaltables: fix build backend

[James Hilliard]

On Tue, Jul 11, 2023 at 2:00 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Tue, 11 Jul 2023 01:35:54 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > > So is this patch series modifying python-terminaltables a prerequisite
> > > to applying the patch at
> > > https://patchwork.ozlabs.org/project/buildroot/patch/20230626181531.2312002-5-james.hilliard1@gmail.com/ ?
> >
> > Yes, it should be applied before that one.
>
> Then, as explained in another e-mail, it should be part of the same
> series. Yes, I understand that you sent the series before noticing that
> python-terminaltables needed to be changed, and that's fine: send a new
> complete iteration of the series with the extra patches.
>
> And for such complicated series, please *always* add a cover letter
> that gives some background: what you're trying to do, and what is the
> path taken to achieve this.

This series can be reviewed entirely independently of the other one, I
went ahead and marked the final pep517 setuptools migration patch as
deferred for now so that there aren't dependency ordering issues or
confusion regarding the relation of this series to the other one.

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

>
> 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 2/2] package/python-terminaltables: fix build backend

[Thomas Petazzoni via buildroot]

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

> This series can be reviewed entirely independently of the other one, I
> went ahead and marked the final pep517 setuptools migration patch as
> deferred for now so that there aren't dependency ordering issues or
> confusion regarding the relation of this series to the other one.

This is a bad idea, which causes even more confusion, as we don't
understand *why* you're making those changes.

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've already asked this in 3 separate e-mails, but you insist on not
doing what is requested to get your changes integrated :-/

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 2/2] package/python-terminaltables: fix build backend

[Thomas Petazzoni via buildroot]

On Tue, 11 Jul 2023 11:54:58 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> This is a bad idea, which causes even more confusion, as we don't
> understand *why* you're making those changes.
> 
> 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've already asked this in 3 separate e-mails, but you insist on not
> doing what is requested to get your changes integrated :-/

Another problem of marking PATCH 5/5 as deferred is that we now have
PATCH 1/5 to PATCH 4/5 in patchwork, and we wonder where PATCH 5/5 has
gone.

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 2/2] package/python-terminaltables: fix build backend

[James Hilliard]

On Tue, Jul 11, 2023 at 3:55 AM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> On Tue, 11 Jul 2023 03:43:26 -0600
> James Hilliard <james.hilliard1@gmail.com> wrote:
>
> > This series can be reviewed entirely independently of the other one, I
> > went ahead and marked the final pep517 setuptools migration patch as
> > deferred for now so that there aren't dependency ordering issues or
> > confusion regarding the relation of this series to the other one.
>
> 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.

https://github.com/matthewdeanmartin/terminaltables/blob/v3.1.10/pyproject.toml#L64-L66

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

If I had caught these bugs before sending the setuptools pep517 migration
series I would have deferred the final migration patch there to avoid this
sort of confusion.

>
> I've already asked this in 3 separate e-mails, but you insist on not
> doing what is requested to get your changes integrated :-/

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.

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.

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

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.

>
> 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 2/2] package/python-terminaltables: fix build backend

[Thomas Petazzoni via buildroot]

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

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

[James Hilliard]

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

I think I was assuming that since I've sent a bunch of other patches that
were already merged doing similar changes that the purpose of this would
have been more obvious. I can respin this series with a better explanation
for now if that sounds good.

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

Yeah, I'm not that great at predicting how much detail I should go into
I guess.

In regards to this particular change I didn't want to go into too many details
regarding the end motivation for fixing this issue as it's only tangentially
related to the setuptools pep517 migration at best(the bug is just that the
build backend doesn't match the one specified in the pyproject.toml file,
which is currently not enforced when using buildroot's distutils/setuptools
infrastructure).

Too much background context on how pep517 works internally I think can
cause confusion as well as there's all sorts of strange edge case behavior.

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

Yeah, this has been a bit of a long term migration process for our python
infrastructure, there's been a bunch of discussions in the past I think which
had some details around what the mid/end goals are and such, maybe
some of those discussions weren't all that clear?

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

Might be a good idea to backport, although probably mostly so anyone
using a BR2_EXTERNAL has the ability to add additional python
packages that require a host-python-poetry-core dependency along
with an in-tree package using it to ensure that it's properly tested.

If we don't care about that then it's probably not necessary.

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

Yeah, I guess I worded that kind of badly, I probably should have clarified
that setuptools is not supported when enforcing pep517 rules(which is what
effectively triggers the failure when used with the setuptools pep517
migration patch but using setuptools here is a spec violation regardless).

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

One issue here is that if there's discussions around too many different
changes in a single giant patch series thread then you start having
issues with mixing up largely unrelated changes. Also the dependency
ordering requirements become a bit more difficult to determine as you
may have multiple internal dependency trees of fixes with a single parent
that depends on all of them. Resolving the independent dependency
trees separately before dealing with the top level parent change seems
to make the discussion threads less confusing.

20 patches in a series is quite a lot, I find the difficulty in managing patches
also doesn't increase linearly with the number of patches but rather goes up
much faster.

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

I think I tend to be bad enough at guessing what needs to be explained
and the level of required detail that it ends up being a lot easier for me to
just clarify things after the fact if anything isn't clear.

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

Yeah, I just mean justified independently in the discussions, agree the
commit log could use some clarification.

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

It depends, I guess, often I'll use git cherry-pick a bit to rework a stack
of patches using commits from a few issue branches or to step by
step validate each patch doesn't break anything when applied in order.

I sometimes use "git rebase -i" but I seem to be rather error prone with it
so often I will just fall back to using "git cherry-pick" with "git
commit --amend"
instead.

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

I think if I was better at guessing what background information is
missing/unclear then that might be the case, I tend to assume(likely
in a number of cases incorrectly) that if a patch is merged in the past
then the reasons for merging were understood well enough that a similar
patch would only need a similar level of explanation to be merged.

I try to be fairly fast to follow up on review comments since I know that
I don't always anticipate well enough what parts may be confusing.

So I've noticed in general that a large patch series tend to get merged
significantly slower than a bunch of small independent patches. This
is one reason I usually don't like to intertwine stuff into a large series
beyond what is absolutely necessary.

For example this series has had a number of revisions and a few
patches merged but then got stuck in the backlog after a few rounds
of review. It's also a rather complex series to test/rebase/review.

https://patchwork.ozlabs.org/project/buildroot/list/?series=346546&submitter=&state=*&q=&archive=both&delegate=

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