* [Buildroot] [PATCH 1/2] package/python-poetry-core: new host package @ 2023-06-27 7:26 James Hilliard 2023-06-27 7:26 ` [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend James Hilliard 0 siblings, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-06-27 7:26 UTC (permalink / raw) To: buildroot; +Cc: James Hilliard, Asaf Kahlon, Thomas Petazzoni 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-06-27 7:26 [Buildroot] [PATCH 1/2] package/python-poetry-core: new host package James Hilliard @ 2023-06-27 7:26 ` James Hilliard 2023-07-10 18:01 ` Thomas Petazzoni via buildroot 0 siblings, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-06-27 7:26 UTC (permalink / raw) To: buildroot; +Cc: James Hilliard, Asaf Kahlon, Thomas Petazzoni 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-06-27 7:26 ` [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend James Hilliard @ 2023-07-10 18:01 ` Thomas Petazzoni via buildroot 2023-07-10 19:58 ` James Hilliard 0 siblings, 1 reply; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-10 18:01 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-10 18:01 ` Thomas Petazzoni via buildroot @ 2023-07-10 19:58 ` James Hilliard 2023-07-11 7:32 ` Thomas Petazzoni via buildroot 0 siblings, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-07-10 19:58 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Asaf Kahlon, buildroot 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-10 19:58 ` James Hilliard @ 2023-07-11 7:32 ` Thomas Petazzoni via buildroot 2023-07-11 7:35 ` James Hilliard 0 siblings, 1 reply; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-11 7:32 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 7:32 ` Thomas Petazzoni via buildroot @ 2023-07-11 7:35 ` James Hilliard 2023-07-11 8:00 ` Thomas Petazzoni via buildroot 0 siblings, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-07-11 7:35 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Asaf Kahlon, buildroot 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 7:35 ` James Hilliard @ 2023-07-11 8:00 ` Thomas Petazzoni via buildroot 2023-07-11 9:43 ` James Hilliard 0 siblings, 1 reply; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-11 8:00 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 8:00 ` Thomas Petazzoni via buildroot @ 2023-07-11 9:43 ` James Hilliard 2023-07-11 9:54 ` Thomas Petazzoni via buildroot 0 siblings, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-07-11 9:43 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Asaf Kahlon, buildroot 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 9:43 ` James Hilliard @ 2023-07-11 9:54 ` Thomas Petazzoni via buildroot 2023-07-11 9:57 ` Thomas Petazzoni via buildroot 2023-07-11 10:50 ` James Hilliard 0 siblings, 2 replies; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-11 9:54 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 9:54 ` Thomas Petazzoni via buildroot @ 2023-07-11 9:57 ` Thomas Petazzoni via buildroot 2023-07-11 10:50 ` James Hilliard 1 sibling, 0 replies; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-11 9:57 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 9:54 ` Thomas Petazzoni via buildroot 2023-07-11 9:57 ` Thomas Petazzoni via buildroot @ 2023-07-11 10:50 ` James Hilliard 2023-07-11 12:29 ` Thomas Petazzoni via buildroot 1 sibling, 1 reply; 13+ messages in thread From: James Hilliard @ 2023-07-11 10:50 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Asaf Kahlon, buildroot 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 10:50 ` James Hilliard @ 2023-07-11 12:29 ` Thomas Petazzoni via buildroot 2023-07-11 14:42 ` James Hilliard 0 siblings, 1 reply; 13+ messages in thread From: Thomas Petazzoni via buildroot @ 2023-07-11 12:29 UTC (permalink / raw) To: James Hilliard; +Cc: Asaf Kahlon, 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend 2023-07-11 12:29 ` Thomas Petazzoni via buildroot @ 2023-07-11 14:42 ` James Hilliard 0 siblings, 0 replies; 13+ messages in thread From: James Hilliard @ 2023-07-11 14:42 UTC (permalink / raw) To: Thomas Petazzoni; +Cc: Asaf Kahlon, buildroot 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-07-11 14:42 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-06-27 7:26 [Buildroot] [PATCH 1/2] package/python-poetry-core: new host package James Hilliard 2023-06-27 7:26 ` [Buildroot] [PATCH 2/2] package/python-terminaltables: fix build backend James Hilliard 2023-07-10 18:01 ` Thomas Petazzoni via buildroot 2023-07-10 19:58 ` James Hilliard 2023-07-11 7:32 ` Thomas Petazzoni via buildroot 2023-07-11 7:35 ` James Hilliard 2023-07-11 8:00 ` Thomas Petazzoni via buildroot 2023-07-11 9:43 ` James Hilliard 2023-07-11 9:54 ` Thomas Petazzoni via buildroot 2023-07-11 9:57 ` Thomas Petazzoni via buildroot 2023-07-11 10:50 ` James Hilliard 2023-07-11 12:29 ` Thomas Petazzoni via buildroot 2023-07-11 14:42 ` James Hilliard
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.