[1/4] pip_install_wheel: don't lazy assign PIPINSTALLARGS

Message ID 20220307194236.3837684-1-ross.burton@arm.com
State Accepted, archived
Commit fdaf5e0027a52e44f2def0ef240134763660aa00
Headers show
Series [1/4] pip_install_wheel: don't lazy assign PIPINSTALLARGS | expand

Commit Message

Ross Burton March 7, 2022, 7:42 p.m. UTC
If we expect users to extend this we should use =, as otherwise a recipe
that does += will replace the default value.

Signed-off-by: Ross Burton <ross.burton@arm.com>
---
 meta/classes/pip_install_wheel.bbclass | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Konrad Weihmann March 7, 2022, 8:06 p.m. UTC | #1
On 07.03.22 20:42, Ross Burton wrote:
> If we expect users to extend this we should use =, as otherwise a recipe
> that does += will replace the default value.
> 
> Signed-off-by: Ross Burton <ross.burton@arm.com>
> ---
>   meta/classes/pip_install_wheel.bbclass | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/classes/pip_install_wheel.bbclass b/meta/classes/pip_install_wheel.bbclass
> index c1680a24ed..febcc8e445 100644
> --- a/meta/classes/pip_install_wheel.bbclass
> +++ b/meta/classes/pip_install_wheel.bbclass
> @@ -10,7 +10,7 @@ PIP_INSTALL_PACKAGE ?= "${@guess_pip_install_package_name(d)}"
>   PIP_INSTALL_DIST_PATH ?= "${@d.getVar('SETUPTOOLS_SETUP_PATH') or d.getVar('B')}/dist"
>   PYPA_WHEEL ??= "${PIP_INSTALL_DIST_PATH}/${PIP_INSTALL_PACKAGE}-*-*.whl"
>   
> -PIP_INSTALL_ARGS ?= "\
> +PIP_INSTALL_ARGS = "\
>       -vvvv \
>       --ignore-installed \
>       --no-cache \
> 
> 

Hmm all of the classes use ?= and I think for a reason - this isn't the 
fault of the bbclass but of the recipe in the end, as += is known to 
cause this depending on where the additions have been added (before the 
inherit or after it) - I think the better way would be to use 
PIP_INSTALL_ARGS:append in the actual recipe - guess the fallout would 
be way less than the one I expect from this change.

The big question is if that should changeable by the user or not - I 
personally think it should be

> 
> -=-=-=-=-=-=-=-=-=-=-=-
> Links: You receive all messages sent to this group.
> View/Reply Online (#162860): https://lists.openembedded.org/g/openembedded-core/message/162860
> Mute This Topic: https://lists.openembedded.org/mt/89621292/3647476
> Group Owner: openembedded-core+owner@lists.openembedded.org
> Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [kweihmann@outlook.com]
> -=-=-=-=-=-=-=-=-=-=-=-
>
Ross Burton March 7, 2022, 9:17 p.m. UTC | #2
Personally I can’t see any reason to extend the pip install arguments. What
option do you think would be used by a recipe?

Ross

On Mon, 7 Mar 2022 at 20:06, Konrad Weihmann <kweihmann@outlook.com> wrote:

>
> On 07.03.22 20:42, Ross Burton wrote:
> > If we expect users to extend this we should use =, as otherwise a recipe
> > that does += will replace the default value.
> >
> > Signed-off-by: Ross Burton <ross.burton@arm.com>
> > ---
> >   meta/classes/pip_install_wheel.bbclass | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/meta/classes/pip_install_wheel.bbclass
> b/meta/classes/pip_install_wheel.bbclass
> > index c1680a24ed..febcc8e445 100644
> > --- a/meta/classes/pip_install_wheel.bbclass
> > +++ b/meta/classes/pip_install_wheel.bbclass
> > @@ -10,7 +10,7 @@ PIP_INSTALL_PACKAGE ?=
> "${@guess_pip_install_package_name(d)}"
> >   PIP_INSTALL_DIST_PATH ?= "${@d.getVar('SETUPTOOLS_SETUP_PATH') or
> d.getVar('B')}/dist"
> >   PYPA_WHEEL ??=
> "${PIP_INSTALL_DIST_PATH}/${PIP_INSTALL_PACKAGE}-*-*.whl"
> >
> > -PIP_INSTALL_ARGS ?= "\
> > +PIP_INSTALL_ARGS = "\
> >       -vvvv \
> >       --ignore-installed \
> >       --no-cache \
> >
> >
>
> Hmm all of the classes use ?= and I think for a reason - this isn't the
> fault of the bbclass but of the recipe in the end, as += is known to
> cause this depending on where the additions have been added (before the
> inherit or after it) - I think the better way would be to use
> PIP_INSTALL_ARGS:append in the actual recipe - guess the fallout would
> be way less than the one I expect from this change.
>
> The big question is if that should changeable by the user or not - I
> personally think it should be
>
> >
> > -=-=-=-=-=-=-=-=-=-=-=-
> > Links: You receive all messages sent to this group.
> > View/Reply Online (#162860):
> https://lists.openembedded.org/g/openembedded-core/message/162860
> > Mute This Topic: https://lists.openembedded.org/mt/89621292/3647476
> > Group Owner: openembedded-core+owner@lists.openembedded.org
> > Unsubscribe: https://lists.openembedded.org/g/openembedded-core/unsub [
> kweihmann@outlook.com]
> > -=-=-=-=-=-=-=-=-=-=-=-
> >
>
Konrad Weihmann March 8, 2022, 7:40 a.m. UTC | #3
On 07.03.22 22:17, Ross Burton wrote:
> Personally I can’t see any reason to extend the pip install arguments. 
> What option do you think would be used by a recipe?

For instance the --user option is something that I used once in a setup.
Plus --global-option for passing additional options into the 
installation scripting.
I give you that, that these are used rarely - and still could be used, 
if the recipes would be tweaked to append after the inherit statement.

And in my mind it was always like a variable with a weak assignment 
could be changed by the user, the ones with a hard assignment shouldn't 
be touched - but that could be just my mind 
Ross Burton March 8, 2022, 11:47 a.m. UTC | #4
On Tue, 8 Mar 2022 at 07:40, Konrad Weihmann <kweihmann@outlook.com> wrote:
> For instance the --user option is something that I used once in a setup.

This is the pip_install_wheel class for installing wheels to the
system library directory, so --user wouldn't be useful surely?

Ross

Patch

diff --git a/meta/classes/pip_install_wheel.bbclass b/meta/classes/pip_install_wheel.bbclass
index c1680a24ed..febcc8e445 100644
--- a/meta/classes/pip_install_wheel.bbclass
+++ b/meta/classes/pip_install_wheel.bbclass
@@ -10,7 +10,7 @@  PIP_INSTALL_PACKAGE ?= "${@guess_pip_install_package_name(d)}"
 PIP_INSTALL_DIST_PATH ?= "${@d.getVar('SETUPTOOLS_SETUP_PATH') or d.getVar('B')}/dist"
 PYPA_WHEEL ??= "${PIP_INSTALL_DIST_PATH}/${PIP_INSTALL_PACKAGE}-*-*.whl"
 
-PIP_INSTALL_ARGS ?= "\
+PIP_INSTALL_ARGS = "\
     -vvvv \
     --ignore-installed \
     --no-cache \