| Message ID | 20230930221727.1458825-1-arnout@mind.be |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | package/pkg-python.mk: refactor setup-type variables | expand |
On Sat, Sep 30, 2023 at 4:17 PM Arnout Vandecappelle <arnout@mind.be> wrote: > > Most of the python-package infrastructure consists of a big conditional > tree that sets various variables based on the package's setup type. > Initially, this was quite OK, but since we have 7 different setup types > now, some of which share some variables with others, it's becoming quite > complicated and hard to read. It might be a good idea to apply my setuptools pep517 patch first which simplifies things a bit by largely combining the setuptools setup type with pep517 builds. https://patchwork.ozlabs.org/project/buildroot/patch/20230930152517.1077576-3-james.hilliard1@gmail.com/ We can probably drop distutils setup type as well as we should be able to instead just use setuptools for any packages using that. > > This patch series refactors it so that the inner-python-package doesn't > dispatch the setup type through a conditional tree, but instead does it > with variable indirection, i.e. by using variables like > $(PKG_PYTHON_$($(PKG)_SETUP_TYPE_UPPER)_ENV) > > The rist two patches are simple cleanups, removing variables that are > not really needed. > > The third patch is a preparatory one that splits the build commands for > target and host builds. This is necessary because we need to use > different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. > > The fourth and fifth patch each replace one variable from the > conditional tree with an indirectly addressed one. > > The following three patches are needed because the ninth patch removes > the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in > fact not strictly needed to remove those variables, but IMHO they don't > add sufficient value to keep them. I think it's better to handle the few > special cases explicitly. > > The ninth patch replaces the remaining variables from the conditional > tree with indirectly addressed ones. The conditional tree is now empty, > and the error handling that was in there is made more explicit. > > The tenth patch edits the documentation to remove the references to the > PKG_PYTHON_*_OPTS variables that were removed in the previous patch. > > This series adds lines rather than removing lines. That is because > things are made more explicit, introducing per-setup-type variables > where previously several setup types were (partially) reusing the pep517 > variables. There are also some additional lines because of more line > splitting. > > To test this series, I built all the python packages with one specific > toolchain configuration, and I ran all runtime tests for python packages. > One package failed to build: host-python-sip. It also fails on master. > For the runtime tests, 14 of them failed, all of them also fail on > master. > > > The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: > > package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) > > are available in the Git repository at: > > git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables > > for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: > > docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) > > ---------------------------------------------------------------- > Arnout Vandecappelle (Essensium/Mind) (10): > package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable > package/pkg-python.mk: remove _BASE_BUILD_OPTS variable > package/pkg-python.mk: split the commands in a target and host section > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) > package/python-flit-core: instantiate _INSTALL_CMDS > package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) > docs/manual: remove references to PKG_PYTHON_*_OPTS > > docs/manual/adding-packages-python.txt | 14 +------- > package/i2c-tools/i2c-tools.mk | 6 ++-- > package/jailhouse/jailhouse.mk | 7 +++- > package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- > package/python-flit-core/python-flit-core.mk | 9 +++-- > 5 files changed, 173 insertions(+), 119 deletions(-) > > >
On 01/10/2023 06:05, James Hilliard wrote: > On Sat, Sep 30, 2023 at 4:17 PM Arnout Vandecappelle <arnout@mind.be> wrote: >> >> Most of the python-package infrastructure consists of a big conditional >> tree that sets various variables based on the package's setup type. >> Initially, this was quite OK, but since we have 7 different setup types >> now, some of which share some variables with others, it's becoming quite >> complicated and hard to read. > > It might be a good idea to apply my setuptools pep517 patch first which > simplifies things a bit by largely combining the setuptools setup type with > pep517 builds. > > https://patchwork.ozlabs.org/project/buildroot/patch/20230930152517.1077576-3-james.hilliard1@gmail.com/ Yeah, my series was already finished when you sent that, but the testing took more than 24 hours... The two will conflict with each other, obviously, but I think they can be applied in either order. > We can probably drop distutils setup type as well as we should be able to > instead just use setuptools for any packages using that. That's a bit a longer-term project. That said, assuming that distutils is gone, do you think the current condition tree would be simpler than the indirect variables that my series ends up with? Regards, Arnout [snip]
On Sun, Oct 1, 2023 at 2:21 AM Arnout Vandecappelle <arnout@mind.be> wrote: > > > > On 01/10/2023 06:05, James Hilliard wrote: > > On Sat, Sep 30, 2023 at 4:17 PM Arnout Vandecappelle <arnout@mind.be> wrote: > >> > >> Most of the python-package infrastructure consists of a big conditional > >> tree that sets various variables based on the package's setup type. > >> Initially, this was quite OK, but since we have 7 different setup types > >> now, some of which share some variables with others, it's becoming quite > >> complicated and hard to read. > > > > It might be a good idea to apply my setuptools pep517 patch first which > > simplifies things a bit by largely combining the setuptools setup type with > > pep517 builds. > > > > https://patchwork.ozlabs.org/project/buildroot/patch/20230930152517.1077576-3-james.hilliard1@gmail.com/ > > Yeah, my series was already finished when you sent that, but the testing took > more than 24 hours... > > The two will conflict with each other, obviously, but I think they can be > applied in either order. True, however the complexity reduction of the pep517 migration should also simplify your series a bit and make it a lot easier to review. > > > > We can probably drop distutils setup type as well as we should be able to > > instead just use setuptools for any packages using that. > > That's a bit a longer-term project. I only see 4 packages still using distutils still so assuming they work ok under setuptools this may actually be quite simple. I'll try and test if those build under setuptools and migrate them if they do. > > That said, assuming that distutils is gone, do you think the current condition > tree would be simpler than the indirect variables that my series ends up with? It's a little hard to tell, ultimately we're only going to have pep517 style builds for everything plus flit-core-boostrap for bootstrapping our pep517 toolchain since the entire python ecosystem appears to have moved to pep517 packaging. > > Regards, > Arnout > > [snip]
On Sun, Oct 1, 2023 at 2:41 AM James Hilliard <james.hilliard1@gmail.com> wrote: > > On Sun, Oct 1, 2023 at 2:21 AM Arnout Vandecappelle <arnout@mind.be> wrote: > > > > > We can probably drop distutils setup type as well as we should be able to > > > instead just use setuptools for any packages using that. > > > > That's a bit a longer-term project. > > I only see 4 packages still using distutils still so assuming they work ok under > setuptools this may actually be quite simple. > > I'll try and test if those build under setuptools and migrate them if they do. > Yeah, they all seem to build fine under setuptools, I sent patches migrating those so we should be good to remove our distutils infrastructure any time after those are merged I think. https://patchwork.ozlabs.org/project/buildroot/patch/20231001091616.1771725-1-james.hilliard1@gmail.com/ https://patchwork.ozlabs.org/project/buildroot/patch/20231001091802.1771968-1-james.hilliard1@gmail.com/ https://patchwork.ozlabs.org/project/buildroot/patch/20231001091943.1772441-1-james.hilliard1@gmail.com/ https://patchwork.ozlabs.org/project/buildroot/patch/20231001092109.1772658-1-james.hilliard1@gmail.com/
Arnout, All, On 2023-10-01 00:16 +0200, Arnout Vandecappelle via buildroot spake thusly: > Most of the python-package infrastructure consists of a big conditional > tree that sets various variables based on the package's setup type. > Initially, this was quite OK, but since we have 7 different setup types > now, some of which share some variables with others, it's becoming quite > complicated and hard to read. > > This patch series refactors it so that the inner-python-package doesn't > dispatch the setup type through a conditional tree, but instead does it > with variable indirection, i.e. by using variables like > $(PKG_PYTHON_$($(PKG)_SETUP_TYPE_UPPER)_ENV) I've applied the whole series to next, now, thanks! When I started looking at: package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) I was not very happy with it, because it moves the variable part of the variable name in the middle, while I believe it is more customary to have it at the end. Also, I did not initially see why we needed to go with UPPERCASE, especially as dot and dash are perfectly legit in variable names. So I started changing your patch to use: FOO_ENV_$($(PKG)_SETUP_TYPE) And then I did the same to the other patches. Until I realised that those variables were used in specific packages, and there it looked misplaced to use such a naming. Also, this would have also meant we'd have to renamed the remaining variables, like _OPTS and co for consistency, so I backpedaled on it and restored your patches as you initially sent them. Still, I reworked the jailhouse package slightly, to move the odd condition-in-_CMDS to conditionally defined macros. So, bottom of it: whole series applied to next. Thanks a lot for this immensely valuable rework! I think we could even go a bit further and drop the conditional block that defines the dependencies... Any taker? ;-) Regards, Yann E. MORIN. > The rist two patches are simple cleanups, removing variables that are > not really needed. > > The third patch is a preparatory one that splits the build commands for > target and host builds. This is necessary because we need to use > different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. > > The fourth and fifth patch each replace one variable from the > conditional tree with an indirectly addressed one. > > The following three patches are needed because the ninth patch removes > the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in > fact not strictly needed to remove those variables, but IMHO they don't > add sufficient value to keep them. I think it's better to handle the few > special cases explicitly. > > The ninth patch replaces the remaining variables from the conditional > tree with indirectly addressed ones. The conditional tree is now empty, > and the error handling that was in there is made more explicit. > > The tenth patch edits the documentation to remove the references to the > PKG_PYTHON_*_OPTS variables that were removed in the previous patch. > > This series adds lines rather than removing lines. That is because > things are made more explicit, introducing per-setup-type variables > where previously several setup types were (partially) reusing the pep517 > variables. There are also some additional lines because of more line > splitting. > > To test this series, I built all the python packages with one specific > toolchain configuration, and I ran all runtime tests for python packages. > One package failed to build: host-python-sip. It also fails on master. > For the runtime tests, 14 of them failed, all of them also fail on > master. > > > The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: > > package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) > > are available in the Git repository at: > > git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables > > for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: > > docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) > > ---------------------------------------------------------------- > Arnout Vandecappelle (Essensium/Mind) (10): > package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable > package/pkg-python.mk: remove _BASE_BUILD_OPTS variable > package/pkg-python.mk: split the commands in a target and host section > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) > package/python-flit-core: instantiate _INSTALL_CMDS > package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) > docs/manual: remove references to PKG_PYTHON_*_OPTS > > docs/manual/adding-packages-python.txt | 14 +------- > package/i2c-tools/i2c-tools.mk | 6 ++-- > package/jailhouse/jailhouse.mk | 7 +++- > package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- > package/python-flit-core/python-flit-core.mk | 9 +++-- > 5 files changed, 173 insertions(+), 119 deletions(-) > > > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
On Sat, Nov 25, 2023 at 2:22 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > Arnout, All, > > On 2023-10-01 00:16 +0200, Arnout Vandecappelle via buildroot spake thusly: > > Most of the python-package infrastructure consists of a big conditional > > tree that sets various variables based on the package's setup type. > > Initially, this was quite OK, but since we have 7 different setup types > > now, some of which share some variables with others, it's becoming quite > > complicated and hard to read. > > > > This patch series refactors it so that the inner-python-package doesn't > > dispatch the setup type through a conditional tree, but instead does it > > with variable indirection, i.e. by using variables like > > $(PKG_PYTHON_$($(PKG)_SETUP_TYPE_UPPER)_ENV) > > I've applied the whole series to next, now, thanks! > > When I started looking at: > > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > > I was not very happy with it, because it moves the variable part of the > variable name in the middle, while I believe it is more customary to > have it at the end. > > Also, I did not initially see why we needed to go with UPPERCASE, > especially as dot and dash are perfectly legit in variable names. > > So I started changing your patch to use: > > FOO_ENV_$($(PKG)_SETUP_TYPE) > > And then I did the same to the other patches. > > Until I realised that those variables were used in specific packages, > and there it looked misplaced to use such a naming. > > Also, this would have also meant we'd have to renamed the remaining > variables, like _OPTS and co for consistency, so I backpedaled on it and > restored your patches as you initially sent them. > > Still, I reworked the jailhouse package slightly, to move the odd > condition-in-_CMDS to conditionally defined macros. > > So, bottom of it: whole series applied to next. > > Thanks a lot for this immensely valuable rework! > > I think we could even go a bit further and drop the conditional block > that defines the dependencies... > > Any taker? ;-) Which conditional dependencies block are you referring to? I rebased my setuptools pep517 migration, once that's merged and we've removed distutils support there's a good bit of refactoring we can do to simplify things as everything will then use pep517 build/install commands. https://patchwork.ozlabs.org/project/buildroot/patch/20231126011932.1380631-5-james.hilliard1@gmail.com/ > > Regards, > Yann E. MORIN. > > > The rist two patches are simple cleanups, removing variables that are > > not really needed. > > > > The third patch is a preparatory one that splits the build commands for > > target and host builds. This is necessary because we need to use > > different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. > > > > The fourth and fifth patch each replace one variable from the > > conditional tree with an indirectly addressed one. > > > > The following three patches are needed because the ninth patch removes > > the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in > > fact not strictly needed to remove those variables, but IMHO they don't > > add sufficient value to keep them. I think it's better to handle the few > > special cases explicitly. > > > > The ninth patch replaces the remaining variables from the conditional > > tree with indirectly addressed ones. The conditional tree is now empty, > > and the error handling that was in there is made more explicit. > > > > The tenth patch edits the documentation to remove the references to the > > PKG_PYTHON_*_OPTS variables that were removed in the previous patch. > > > > This series adds lines rather than removing lines. That is because > > things are made more explicit, introducing per-setup-type variables > > where previously several setup types were (partially) reusing the pep517 > > variables. There are also some additional lines because of more line > > splitting. > > > > To test this series, I built all the python packages with one specific > > toolchain configuration, and I ran all runtime tests for python packages. > > One package failed to build: host-python-sip. It also fails on master. > > For the runtime tests, 14 of them failed, all of them also fail on > > master. > > > > > > The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: > > > > package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) > > > > are available in the Git repository at: > > > > git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables > > > > for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: > > > > docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) > > > > ---------------------------------------------------------------- > > Arnout Vandecappelle (Essensium/Mind) (10): > > package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable > > package/pkg-python.mk: remove _BASE_BUILD_OPTS variable > > package/pkg-python.mk: split the commands in a target and host section > > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > > package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) > > package/python-flit-core: instantiate _INSTALL_CMDS > > package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) > > docs/manual: remove references to PKG_PYTHON_*_OPTS > > > > docs/manual/adding-packages-python.txt | 14 +------- > > package/i2c-tools/i2c-tools.mk | 6 ++-- > > package/jailhouse/jailhouse.mk | 7 +++- > > package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- > > package/python-flit-core/python-flit-core.mk | 9 +++-- > > 5 files changed, 173 insertions(+), 119 deletions(-) > > > > > > > > _______________________________________________ > > buildroot mailing list > > buildroot@buildroot.org > > https://lists.buildroot.org/mailman/listinfo/buildroot > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
James, All, On 2023-11-25 18:40 -0700, James Hilliard spake thusly: > On Sat, Nov 25, 2023 at 2:22 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > I think we could even go a bit further and drop the conditional block > > that defines the dependencies... > Which conditional dependencies block are you referring to? This one: https://gitlab.com/buildroot.org/buildroot/-/blob/next/package/pkg-python.mk#L333 > I rebased my setuptools pep517 migration, once that's merged and we've > removed distutils support there's a good bit of refactoring we can do to > simplify things as everything will then use pep517 build/install commands. > > https://patchwork.ozlabs.org/project/buildroot/patch/20231126011932.1380631-5-james.hilliard1@gmail.com/ As far as I can see, that patch does not refactor the dependencies in a way similar to how the other variables have been refactored, i.e. something like: PKG_PYTHON_SETUPTOOLS_DEPENDENCIES = host-python-setuptools PKG_PYTHON_SETUPTOOLS_RUST_DEPENDENCIES = host-python-setuptools host-python-setuptools-rust and so on (and for the HOST_ variants as well), and then: $(2)_DEPENDENCIES += $$(PKG_PYTHON_$$($$(PKG)_SETUP_TYPE_UPPER)_DEPENDENCIES) which, as I suggested, would do with the dependency comnditional block basically the same as the previous patches have done for the build commands. And if we're smart, we can go even further and handle all the setup-type-based variables similarly: PKG_PYTHON_SETUPTOOLS_RUST_DL_ENV = $$(PKG_CARGO_ENV) PKG_PYTHON_MATURIN_RUST_DL_ENV = $$(PKG_CARGO_ENV) and then: $(2)_DL_ENV += $$(PKG_PYTHON_$$($$(PKG)_SETUP_TYPE_UPPER)_DL_ENV) It is not that the remaining conditional block are as unwieldy as the previos one about build sommands, but it would be goos to have some consistency in the way the various setup-types are handled. Regards, Yann E. MORIN. > > > > Regards, > > Yann E. MORIN. > > > > > The rist two patches are simple cleanups, removing variables that are > > > not really needed. > > > > > > The third patch is a preparatory one that splits the build commands for > > > target and host builds. This is necessary because we need to use > > > different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. > > > > > > The fourth and fifth patch each replace one variable from the > > > conditional tree with an indirectly addressed one. > > > > > > The following three patches are needed because the ninth patch removes > > > the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in > > > fact not strictly needed to remove those variables, but IMHO they don't > > > add sufficient value to keep them. I think it's better to handle the few > > > special cases explicitly. > > > > > > The ninth patch replaces the remaining variables from the conditional > > > tree with indirectly addressed ones. The conditional tree is now empty, > > > and the error handling that was in there is made more explicit. > > > > > > The tenth patch edits the documentation to remove the references to the > > > PKG_PYTHON_*_OPTS variables that were removed in the previous patch. > > > > > > This series adds lines rather than removing lines. That is because > > > things are made more explicit, introducing per-setup-type variables > > > where previously several setup types were (partially) reusing the pep517 > > > variables. There are also some additional lines because of more line > > > splitting. > > > > > > To test this series, I built all the python packages with one specific > > > toolchain configuration, and I ran all runtime tests for python packages. > > > One package failed to build: host-python-sip. It also fails on master. > > > For the runtime tests, 14 of them failed, all of them also fail on > > > master. > > > > > > > > > The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: > > > > > > package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) > > > > > > are available in the Git repository at: > > > > > > git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables > > > > > > for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: > > > > > > docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) > > > > > > ---------------------------------------------------------------- > > > Arnout Vandecappelle (Essensium/Mind) (10): > > > package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable > > > package/pkg-python.mk: remove _BASE_BUILD_OPTS variable > > > package/pkg-python.mk: split the commands in a target and host section > > > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > > > package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) > > > package/python-flit-core: instantiate _INSTALL_CMDS > > > package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > > package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > > package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) > > > docs/manual: remove references to PKG_PYTHON_*_OPTS > > > > > > docs/manual/adding-packages-python.txt | 14 +------- > > > package/i2c-tools/i2c-tools.mk | 6 ++-- > > > package/jailhouse/jailhouse.mk | 7 +++- > > > package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- > > > package/python-flit-core/python-flit-core.mk | 9 +++-- > > > 5 files changed, 173 insertions(+), 119 deletions(-) > > > > > > > > > > > > _______________________________________________ > > > buildroot mailing list > > > buildroot@buildroot.org > > > https://lists.buildroot.org/mailman/listinfo/buildroot > > > > -- > > .-----------------.--------------------.------------------.--------------------. > > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > > '------------------------------^-------^------------------^--------------------'
On Sun, Nov 26, 2023 at 4:21 AM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > James, All, > > On 2023-11-25 18:40 -0700, James Hilliard spake thusly: > > On Sat, Nov 25, 2023 at 2:22 PM Yann E. MORIN <yann.morin.1998@free.fr> wrote: > > > I think we could even go a bit further and drop the conditional block > > > that defines the dependencies... > > Which conditional dependencies block are you referring to? > > This one: > > https://gitlab.com/buildroot.org/buildroot/-/blob/next/package/pkg-python.mk#L333 > > > I rebased my setuptools pep517 migration, once that's merged and we've > > removed distutils support there's a good bit of refactoring we can do to > > simplify things as everything will then use pep517 build/install commands. > > > > https://patchwork.ozlabs.org/project/buildroot/patch/20231126011932.1380631-5-james.hilliard1@gmail.com/ > > As far as I can see, that patch does not refactor the dependencies in a > way similar to how the other variables have been refactored, i.e. > something like: > > PKG_PYTHON_SETUPTOOLS_DEPENDENCIES = host-python-setuptools > > PKG_PYTHON_SETUPTOOLS_RUST_DEPENDENCIES = host-python-setuptools host-python-setuptools-rust > > and so on (and for the HOST_ variants as well), and then: Actually HOST_ variants are not needed here since all setup type specific dependencies are the same for host and target package builds. > > $(2)_DEPENDENCIES += $$(PKG_PYTHON_$$($$(PKG)_SETUP_TYPE_UPPER)_DEPENDENCIES) I had to tweak this a little to get it to work since it seems we can't use $$(PKG) here: $(2)_DEPENDENCIES += $$(PKG_PYTHON_$$($(2)_SETUP_TYPE_UPPER)_DEPENDENCIES) > > which, as I suggested, would do with the dependency comnditional > block basically the same as the previous patches have done for the build > commands. Ok, I refactored my v6 to use this style: https://patchwork.ozlabs.org/project/buildroot/patch/20231126180840.2081945-5-james.hilliard1@gmail.com/ > > And if we're smart, we can go even further and handle all the > setup-type-based variables similarly: > > PKG_PYTHON_SETUPTOOLS_RUST_DL_ENV = $$(PKG_CARGO_ENV) > > PKG_PYTHON_MATURIN_RUST_DL_ENV = $$(PKG_CARGO_ENV) > > and then: > > $(2)_DL_ENV += $$(PKG_PYTHON_$$($$(PKG)_SETUP_TYPE_UPPER)_DL_ENV) > > It is not that the remaining conditional block are as unwieldy as the > previos one about build sommands, but it would be goos to have some > consistency in the way the various setup-types are handled. I'm not sure this makes sense since all the cargo DL_ENV stuff shouldn't change based on a package being setuptools-rust vs maturin. > > Regards, > Yann E. MORIN. > > > > > > > Regards, > > > Yann E. MORIN. > > > > > > > The rist two patches are simple cleanups, removing variables that are > > > > not really needed. > > > > > > > > The third patch is a preparatory one that splits the build commands for > > > > target and host builds. This is necessary because we need to use > > > > different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. > > > > > > > > The fourth and fifth patch each replace one variable from the > > > > conditional tree with an indirectly addressed one. > > > > > > > > The following three patches are needed because the ninth patch removes > > > > the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in > > > > fact not strictly needed to remove those variables, but IMHO they don't > > > > add sufficient value to keep them. I think it's better to handle the few > > > > special cases explicitly. > > > > > > > > The ninth patch replaces the remaining variables from the conditional > > > > tree with indirectly addressed ones. The conditional tree is now empty, > > > > and the error handling that was in there is made more explicit. > > > > > > > > The tenth patch edits the documentation to remove the references to the > > > > PKG_PYTHON_*_OPTS variables that were removed in the previous patch. > > > > > > > > This series adds lines rather than removing lines. That is because > > > > things are made more explicit, introducing per-setup-type variables > > > > where previously several setup types were (partially) reusing the pep517 > > > > variables. There are also some additional lines because of more line > > > > splitting. > > > > > > > > To test this series, I built all the python packages with one specific > > > > toolchain configuration, and I ran all runtime tests for python packages. > > > > One package failed to build: host-python-sip. It also fails on master. > > > > For the runtime tests, 14 of them failed, all of them also fail on > > > > master. > > > > > > > > > > > > The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: > > > > > > > > package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) > > > > > > > > are available in the Git repository at: > > > > > > > > git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables > > > > > > > > for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: > > > > > > > > docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) > > > > > > > > ---------------------------------------------------------------- > > > > Arnout Vandecappelle (Essensium/Mind) (10): > > > > package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable > > > > package/pkg-python.mk: remove _BASE_BUILD_OPTS variable > > > > package/pkg-python.mk: split the commands in a target and host section > > > > package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) > > > > package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) > > > > package/python-flit-core: instantiate _INSTALL_CMDS > > > > package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > > > package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS > > > > package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) > > > > docs/manual: remove references to PKG_PYTHON_*_OPTS > > > > > > > > docs/manual/adding-packages-python.txt | 14 +------- > > > > package/i2c-tools/i2c-tools.mk | 6 ++-- > > > > package/jailhouse/jailhouse.mk | 7 +++- > > > > package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- > > > > package/python-flit-core/python-flit-core.mk | 9 +++-- > > > > 5 files changed, 173 insertions(+), 119 deletions(-) > > > > > > > > > > > > > > > > _______________________________________________ > > > > buildroot mailing list > > > > buildroot@buildroot.org > > > > https://lists.buildroot.org/mailman/listinfo/buildroot > > > > > > -- > > > .-----------------.--------------------.------------------.--------------------. > > > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > > > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > > > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > > > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > > > '------------------------------^-------^------------------^--------------------' > > -- > .-----------------.--------------------.------------------.--------------------. > | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | > | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | > | +33 561 099 427 `------------.-------: X AGAINST | \e/ There is no | > | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | > '------------------------------^-------^------------------^--------------------'
Most of the python-package infrastructure consists of a big conditional tree that sets various variables based on the package's setup type. Initially, this was quite OK, but since we have 7 different setup types now, some of which share some variables with others, it's becoming quite complicated and hard to read. This patch series refactors it so that the inner-python-package doesn't dispatch the setup type through a conditional tree, but instead does it with variable indirection, i.e. by using variables like $(PKG_PYTHON_$($(PKG)_SETUP_TYPE_UPPER)_ENV) The rist two patches are simple cleanups, removing variables that are not really needed. The third patch is a preparatory one that splits the build commands for target and host builds. This is necessary because we need to use different variables for the two, i.e. HOST_PKG_PYTHON_* for host build. The fourth and fifth patch each replace one variable from the conditional tree with an indirectly addressed one. The following three patches are needed because the ninth patch removes the PKG_PYTHON_*_OPTS variables, which were used in 3 packages. It is in fact not strictly needed to remove those variables, but IMHO they don't add sufficient value to keep them. I think it's better to handle the few special cases explicitly. The ninth patch replaces the remaining variables from the conditional tree with indirectly addressed ones. The conditional tree is now empty, and the error handling that was in there is made more explicit. The tenth patch edits the documentation to remove the references to the PKG_PYTHON_*_OPTS variables that were removed in the previous patch. This series adds lines rather than removing lines. That is because things are made more explicit, introducing per-setup-type variables where previously several setup types were (partially) reusing the pep517 variables. There are also some additional lines because of more line splitting. To test this series, I built all the python packages with one specific toolchain configuration, and I ran all runtime tests for python packages. One package failed to build: host-python-sip. It also fails on master. For the runtime tests, 14 of them failed, all of them also fail on master. The following changes since commit 7906272c39744e26ed73028725787aa3a4441c54: package/python-rtoml: migrate to setuptools-rust infrastructure (2023-09-29 22:02:31 +0200) are available in the Git repository at: git@gitlab.com:arnout/buildroot.git pkg-python-refactor-variables for you to fetch changes up to 89b33e004fc13e22a9c0bdcafcee245a97d2dd51: docs/manual: remove references to PKG_PYTHON_*_OPTS (2023-09-30 15:18:48 +0200) ---------------------------------------------------------------- Arnout Vandecappelle (Essensium/Mind) (10): package/pkg-python.mk: remove $(2)_PYTHON_INTERPRETER variable package/pkg-python.mk: remove _BASE_BUILD_OPTS variable package/pkg-python.mk: split the commands in a target and host section package/pkg-python.mk: replace $(_BASE_ENV) with $($(SETUP_TYPE)_ENV) package/pkg-python.mk: replace $(_BASE_BUILD_CMD) with $($(SETUP_TYPE)_BUILD_CMD) package/python-flit-core: instantiate _INSTALL_CMDS package/jailhouse: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS package/i2c-tools: expand PKG_PYTHON_SETUPTOOLS_INSTALL_TARGET_OPTS package/pkg-python.mk: replace $(_BASE_INSTALL*_CMD) with $($(SETUP_TYPE)_INSTALL*_CMD) docs/manual: remove references to PKG_PYTHON_*_OPTS docs/manual/adding-packages-python.txt | 14 +------- package/i2c-tools/i2c-tools.mk | 6 ++-- package/jailhouse/jailhouse.mk | 7 +++- package/pkg-python.mk | 256 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------- package/python-flit-core/python-flit-core.mk | 9 +++-- 5 files changed, 173 insertions(+), 119 deletions(-)