[Buildroot] package/pkg-python.mk: refactor setup-type variables

Sun Nov 26 01:40:05 UTC 2023

On Sat, Nov 25, 2023 at 2:22 PM Yann E. MORIN <yann.morin.1998 at 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 at 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 at 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.  |
> '------------------------------^-------^------------------^--------------------'