* [Buildroot] [PATCH 1/1] package/pkg-python: use a shell expansion for sysconfigdata_name
2020-06-05 20:59 [Buildroot] [PATCH 1/1] package/pkg-python: use a shell expansion for sysconfigdata_name aduskett at gmail.com
@ 2020-06-05 21:58 ` Yann E. MORIN
2020-06-15 21:22 ` Yann E. MORIN
2020-07-15 19:36 ` Peter Korsgaard
2 siblings, 0 replies; 4+ messages in thread
From: Yann E. MORIN @ 2020-06-05 21:58 UTC (permalink / raw)
To: buildroot
Adam, All,
On 2020-06-05 13:59 -0700, aduskett at gmail.com spake thusly:
> From: Adam Duskett <Aduskett@gmail.com>
>
> Currently, GNU Make expands the Python SYSCONFIGDATA_NAME variable; however,
> when building with per-package directories, this variable is not set because
> the evaluation of this variable occurs before buildroot creates the
> per-package directories of a given package.
This can be easily demonstrated with that trivial Makefile:
$ cat Makefile
BLA = $(wildcard bla)
all:
@echo 'BLA=$(BLA)'
@touch bla
@echo 'BLA=$(BLA)'
$ make
BLA=
BLA=
$ make
BLA=bla
BLA=bla
I.e. the variables are evaluated at the beginning of a recipe, not for
each line of the recipe.
> There are two solutions to fix this problem:
> - Add a step between "patch" and "configure," which would evaluate all of the
> variables after creating the per-package directories.
>
> - Evaluate SYSCONFIGDATA_NAME via a shell expansion instead of GNU Make.
>
> As the second option is more expedient, the second option is what this commit
> impliments.
>
> Remove the current PKG_PYTHON_SYSCONFIGDATA_NAME definition and replace it
> with the following:
>
> PKG_PYTHON_SYSCONFIGDATA_PATH:
> - This variable is used to make the next line easier to read.
>
> PKG_PYTHON_SYSCONFIGDATA_NAME = `{ [ -e $(PKG_PYTHON_SYSCONFIGDATA_PATH) ] &&
> basename $(PKG_PYTHON_SYSCONFIGDATA_PATH) .py; } || true`
>
> - The "-e" check ensures the path exists, as the basename command only
> evaluates strings.
>
> - The "|| true" is added to ensure the old behavior of returning an empty
> string if the file does not exist still works.
That's not correct. The real reason for the || true is to cover cases
where the expansion would be in a context where 'set -e' is in effect,
and the when the file actually does not exist (i.e. for python2 for
example), where we do not want the shell to fail in that case (to mimick
the existing behaviour).
But I'd also like some more knowledgeable people to look at it too...
Regards,
Yann E. MORIN.
> Fixes: https://bugs.busybox.net/show_bug.cgi?id=12941
> Signed-off-by: Adam Duskett <Aduskett@gmail.com>
> ---
> package/pkg-python.mk | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> index 4bf762e662..73fe9954e7 100644
> --- a/package/pkg-python.mk
> +++ b/package/pkg-python.mk
> @@ -20,9 +20,11 @@
> #
> ################################################################################
>
> -define PKG_PYTHON_SYSCONFIGDATA_NAME
> -$(basename $(notdir $(wildcard $(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/_sysconfigdata__linux_*.py)))
> -endef
> +# basename does not evaluate if a file exists, so we must check to ensure
> +# the _sysconfigdata__linux_*.py file exists. The "|| true" is added to return
> +# an empty string if the file does not exist.
> +PKG_PYTHON_SYSCONFIGDATA_PATH = $(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/_sysconfigdata__linux_*.py
> +PKG_PYTHON_SYSCONFIGDATA_NAME = `{ [ -e $(PKG_PYTHON_SYSCONFIGDATA_PATH) ] && basename $(PKG_PYTHON_SYSCONFIGDATA_PATH) .py; } || true`
>
> # Target distutils-based packages
> PKG_PYTHON_DISTUTILS_ENV = \
> --
> 2.26.2
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/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. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 4+ messages in thread* [Buildroot] [PATCH 1/1] package/pkg-python: use a shell expansion for sysconfigdata_name
2020-06-05 20:59 [Buildroot] [PATCH 1/1] package/pkg-python: use a shell expansion for sysconfigdata_name aduskett at gmail.com
2020-06-05 21:58 ` Yann E. MORIN
@ 2020-06-15 21:22 ` Yann E. MORIN
2020-07-15 19:36 ` Peter Korsgaard
2 siblings, 0 replies; 4+ messages in thread
From: Yann E. MORIN @ 2020-06-15 21:22 UTC (permalink / raw)
To: buildroot
Adam, All,
On 2020-06-05 13:59 -0700, aduskett at gmail.com spake thusly:
> From: Adam Duskett <Aduskett@gmail.com>
>
> Currently, GNU Make expands the Python SYSCONFIGDATA_NAME variable; however,
> when building with per-package directories, this variable is not set because
> the evaluation of this variable occurs before buildroot creates the
> per-package directories of a given package.
>
> There are two solutions to fix this problem:
> - Add a step between "patch" and "configure," which would evaluate all of the
> variables after creating the per-package directories.
>
> - Evaluate SYSCONFIGDATA_NAME via a shell expansion instead of GNU Make.
>
> As the second option is more expedient, the second option is what this commit
> impliments.
>
> Remove the current PKG_PYTHON_SYSCONFIGDATA_NAME definition and replace it
> with the following:
>
> PKG_PYTHON_SYSCONFIGDATA_PATH:
> - This variable is used to make the next line easier to read.
>
> PKG_PYTHON_SYSCONFIGDATA_NAME = `{ [ -e $(PKG_PYTHON_SYSCONFIGDATA_PATH) ] &&
> basename $(PKG_PYTHON_SYSCONFIGDATA_PATH) .py; } || true`
>
> - The "-e" check ensures the path exists, as the basename command only
> evaluates strings.
>
> - The "|| true" is added to ensure the old behavior of returning an empty
> string if the file does not exist still works.
>
> Fixes: https://bugs.busybox.net/show_bug.cgi?id=12941
> Signed-off-by: Adam Duskett <Aduskett@gmail.com>
We had quite some back-n-forth with Thomas on IRC, but we did not came
to a strong conclusion in favour of a better solution than this patch of
yours for now.
For reference, the discussion was centred on whether we actually needed
to have a "dynamic" variable that got expanded (wildcard, etc..), or if
we could not do without and use a static value. There were two options
considered for that value:
- something completely made up by us Buildroot, passed to python at
build time, so it knows about it and uses it at runtime; howwever,
this last point is not possible: python computes the value even at
runtime, by combining a few internl values (that are however already
known at build time, sigh)so we'd need to patch it. Or we'd need to
export _PYTHON_SYSCONFIGDATA_NAME on the target, which is not very
convenient either
- _PYTHON_SYSCONFIGDATA_NAME is _sysconfigdata_{abi}_{platform}_{multiarch}
where (for Buildroot) {abi} is always empty, {platform} is always
'linux', and {multiarch} is the output of `target-gcc --print-multiarch`
So this last case would still require that we have an expansion of
some sorts, because we can't easily know what gcc thinks its
multiarch is.
Of course, we also discussed the best solution, to introduce an internal
step between 'patch' and 'configure', but there again we did not get to
a final conclusion either...
So, I've applied your patch to master, after rewording the commit log
slightly. Thanks!
Regards,
Yann E. MORIN.
> ---
> package/pkg-python.mk | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> index 4bf762e662..73fe9954e7 100644
> --- a/package/pkg-python.mk
> +++ b/package/pkg-python.mk
> @@ -20,9 +20,11 @@
> #
> ################################################################################
>
> -define PKG_PYTHON_SYSCONFIGDATA_NAME
> -$(basename $(notdir $(wildcard $(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/_sysconfigdata__linux_*.py)))
> -endef
> +# basename does not evaluate if a file exists, so we must check to ensure
> +# the _sysconfigdata__linux_*.py file exists. The "|| true" is added to return
> +# an empty string if the file does not exist.
> +PKG_PYTHON_SYSCONFIGDATA_PATH = $(STAGING_DIR)/usr/lib/python$(PYTHON3_VERSION_MAJOR)/_sysconfigdata__linux_*.py
> +PKG_PYTHON_SYSCONFIGDATA_NAME = `{ [ -e $(PKG_PYTHON_SYSCONFIGDATA_PATH) ] && basename $(PKG_PYTHON_SYSCONFIGDATA_PATH) .py; } || true`
>
> # Target distutils-based packages
> PKG_PYTHON_DISTUTILS_ENV = \
> --
> 2.26.2
>
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/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. |
'------------------------------^-------^------------------^--------------------'
^ permalink raw reply [flat|nested] 4+ messages in thread