|
Hi All,
I shared a patch on the bitbake mailing list to make some syntax generate
warnings, specifically things of the form:
XXX_append += "YYY"
XXX_prepend += "YYY"
XXX_remove += "YYY"
and the ?=, =+, .= and =. variants. This also covers variants with overrides in
there too. It would basically mean that you need to use "=" with the append
operator and friends.
We found one reference of that use in OE-Core and one in meta-intel. There are
others in other layers but the usage isn't that widespread.
Why should we warn against this? The += usage in particular is sometimes used to
add a space so:
XXX_append += "YYY"
and
XXX_append = " YYY"
are equivalent but this is more of an accident rather than a specific bitbake
design. It is extremely confusing to new users and has been complained about
before as an issue effecting usability. New users often think that += will stack
the append strings much like += does on a normal variable but that is not the
case and can lead to confusion.
I'm been looking at ways we can simplify without hurting functionality and
having this slightly obscure usage doesn't really seem to help much. If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
Cheers,
Richard
|
|
|
If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
Agreed, this combination isn't really needed and it's easy to fix relatively few cases which slipped into various layers.
LGTM
|
|
|
Khem Raj
On 11/9/21 7:15 AM, Richard Purdie wrote: Hi All,
I shared a patch on the bitbake mailing list to make some syntax generate
warnings, specifically things of the form:
XXX_append += "YYY"
XXX_prepend += "YYY"
XXX_remove += "YYY"
and the ?=, =+, .= and =. variants. This also covers variants with overrides in
there too. It would basically mean that you need to use "=" with the append
operator and friends.
We found one reference of that use in OE-Core and one in meta-intel. There are
others in other layers but the usage isn't that widespread.
Why should we warn against this? The += usage in particular is sometimes used to
add a space so:
XXX_append += "YYY"
and
XXX_append = " YYY"
are equivalent but this is more of an accident rather than a specific bitbake
design. It is extremely confusing to new users and has been complained about
before as an issue effecting usability. New users often think that += will stack
the append strings much like += does on a normal variable but that is not the
case and can lead to confusion.
I think this is a good change, since it clarifies the functionality and removes undefined behavior that sort of seemed to achieve a valid operation. I'm been looking at ways we can simplify without hurting functionality and
having this slightly obscure usage doesn't really seem to help much. If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
Cheers,
Richard
|
|
|
Marco Cavallini
On 09/11/21 16:15, Richard Purdie wrote: Hi All,
I shared a patch on the bitbake mailing list to make some syntax generate
warnings, specifically things of the form:
XXX_append += "YYY"
XXX_prepend += "YYY"
XXX_remove += "YYY"
and the ?=, =+, .= and =. variants. This also covers variants with overrides in
there too. It would basically mean that you need to use "=" with the append
operator and friends.
We found one reference of that use in OE-Core and one in meta-intel. There are
others in other layers but the usage isn't that widespread.
Why should we warn against this? The += usage in particular is sometimes used to
add a space so:
XXX_append += "YYY"
and
XXX_append = " YYY"
are equivalent but this is more of an accident rather than a specific bitbake
design. It is extremely confusing to new users and has been complained about
before as an issue effecting usability. New users often think that += will stack
the append strings much like += does on a normal variable but that is not the
case and can lead to confusion.
I'm been looking at ways we can simplify without hurting functionality and
having this slightly obscure usage doesn't really seem to help much. If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
Cheers,
Richard
Lovely! -- Marco
|
|
|
Hi Richard, On November 9, 2021 4:15:16 PM GMT+01:00, Richard Purdie <richard.purdie@...> wrote: Hi All,
I shared a patch on the bitbake mailing list to make some syntax generate
warnings, specifically things of the form:
XXX_append += "YYY"
XXX_prepend += "YYY"
XXX_remove += "YYY"
Using the old syntax :D? and the ?=, =+, .= and =. variants. This also covers variants with overrides in
there too. It would basically mean that you need to use "=" with the append
operator and friends.
We found one reference of that use in OE-Core and one in meta-intel. There are
others in other layers but the usage isn't that widespread.
Why should we warn against this? The += usage in particular is sometimes used to
add a space so:
XXX_append += "YYY"
and
XXX_append = " YYY"
are equivalent but this is more of an accident rather than a specific bitbake
design. It is extremely confusing to new users and has been complained about
before as an issue effecting usability. New users often think that += will stack
the append strings much like += does on a normal variable but that is not the
case and can lead to confusion.
I'm been looking at ways we can simplify without hurting functionality and
having this slightly obscure usage doesn't really seem to help much. If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
I'm all for removing potential areas of confusion, so +1. On that topic, if I'm not mistaken, the same applies to: FOO:.*{append, prepend, remove}:.*{append,prepend,remove} ? Basically it does not make much sense to have (any) two of append, prepend, remove as overrides of the same variable (on the same line). Cheers, Quentin Cheers,
Richard
|
|
|
Hi All,
I would also emit a (switchable on / off) warning for VAR_append and VAR:append without a space when VAR contains spaces already i.e.:
VAR = "opt1 opt2"
VAR:append = "opt3" --> warning
VAR2 = "--I-have-super-long-custom-option-flag"
VAR2:append = "-and-short-also" --> NO warning
This should cause very little false warnings.
On Tue, Nov 9, 2021 at 8:15 PM Quentin Schulz < foss@...> wrote: Hi Richard,
On November 9, 2021 4:15:16 PM GMT+01:00, Richard Purdie <richard.purdie@...> wrote:
>Hi All,
>
>I shared a patch on the bitbake mailing list to make some syntax generate
>warnings, specifically things of the form:
>
>XXX_append += "YYY"
>XXX_prepend += "YYY"
>XXX_remove += "YYY"
>
Using the old syntax :D?
>and the ?=, =+, .= and =. variants. This also covers variants with overrides in
>there too. It would basically mean that you need to use "=" with the append
>operator and friends.
>
>We found one reference of that use in OE-Core and one in meta-intel. There are
>others in other layers but the usage isn't that widespread.
>
>Why should we warn against this? The += usage in particular is sometimes used to
>add a space so:
>
>XXX_append += "YYY"
>
>and
>
>XXX_append = " YYY"
>
>are equivalent but this is more of an accident rather than a specific bitbake
>design. It is extremely confusing to new users and has been complained about
>before as an issue effecting usability. New users often think that += will stack
>the append strings much like += does on a normal variable but that is not the
>case and can lead to confusion.
>
>I'm been looking at ways we can simplify without hurting functionality and
>having this slightly obscure usage doesn't really seem to help much. If we agree
>to do this, I would make it an error in due course after users have seen the
>warnings for a while. I'd probably want to do that before the LTS next year.
>
I'm all for removing potential areas of confusion, so +1.
On that topic, if I'm not mistaken, the same applies to:
FOO:.*{append, prepend, remove}:.*{append,prepend,remove}
? Basically it does not make much sense to have (any) two of append, prepend, remove as overrides of the same variable (on the same line).
Cheers,
Quentin
>Cheers,
>
>Richard
>
>
>
>
|
|
|
Tim Orling
Hi All,
I would also emit a (switchable on / off) warning for VAR_append and VAR:append without a space when VAR contains spaces already i.e.:
VAR = "opt1 opt2"
VAR:append = "opt3" --> warning
I was just going to comment with the same idea. Much earlier in my OE journey, this bit me often and was a source of confusion.
VAR2 = "--I-have-super-long-custom-option-flag"
VAR2:append = "-and-short-also" --> NO warning
This should cause very little false warnings.
On Tue, Nov 9, 2021 at 8:15 PM Quentin Schulz < foss@...> wrote: Hi Richard,
On November 9, 2021 4:15:16 PM GMT+01:00, Richard Purdie <richard.purdie@...> wrote:
>Hi All,
>
>I shared a patch on the bitbake mailing list to make some syntax generate
>warnings, specifically things of the form:
>
>XXX_append += "YYY"
>XXX_prepend += "YYY"
>XXX_remove += "YYY"
>
Using the old syntax :D?
>and the ?=, =+, .= and =. variants. This also covers variants with overrides in
>there too. It would basically mean that you need to use "=" with the append
>operator and friends.
>
>We found one reference of that use in OE-Core and one in meta-intel. There are
>others in other layers but the usage isn't that widespread.
>
>Why should we warn against this? The += usage in particular is sometimes used to
>add a space so:
>
>XXX_append += "YYY"
>
>and
>
>XXX_append = " YYY"
>
>are equivalent but this is more of an accident rather than a specific bitbake
>design. It is extremely confusing to new users and has been complained about
>before as an issue effecting usability. New users often think that += will stack
>the append strings much like += does on a normal variable but that is not the
>case and can lead to confusion.
>
>I'm been looking at ways we can simplify without hurting functionality and
>having this slightly obscure usage doesn't really seem to help much. If we agree
>to do this, I would make it an error in due course after users have seen the
>warnings for a while. I'd probably want to do that before the LTS next year.
>
I'm all for removing potential areas of confusion, so +1.
On that topic, if I'm not mistaken, the same applies to:
FOO:.*{append, prepend, remove}:.*{append,prepend,remove}
? Basically it does not make much sense to have (any) two of append, prepend, remove as overrides of the same variable (on the same line).
Cheers,
Quentin
>Cheers,
>
>Richard
>
>
>
>
|
|
|
Hi All,
I would also emit a (switchable on / off) warning for VAR_append and VAR:append without a space when VAR contains spaces already i.e.:
VAR = "opt1 opt2"
VAR:append = "opt3" --> warning
VAR2 = "--I-have-super-long-custom-option-flag"
VAR2:append = "-and-short-also" --> NO warning
But this 2nd example is what might be difficult to spot and most likely needs the leading space as well, right?
Especially when there is some override like VAR2:append:machine-foo:distro-foo = "-and-short-also" in some .bbappend somewhere else.
and you spend a lot of time trying to find out why --I-have-super-long-custom-option-flag isn't respected in this one build for some specific machine and specific distro, until you check with bitbake-getvar to find "--I-have-super-long-custom-option-flag-and-short-also" being used.
So this warning can cause bunch of false positives, won't be able to catch other true positives and probably just adds false sense of security - instead of people learning to use bitbake-getvar early to notice possible issues.
|
|
|
On Tue, 2021-11-09 at 18:14 +0100, Quentin Schulz wrote: Hi Richard,
On November 9, 2021 4:15:16 PM GMT+01:00, Richard Purdie <richard.purdie@...> wrote:
Hi All,
I shared a patch on the bitbake mailing list to make some syntax generate
warnings, specifically things of the form:
XXX_append += "YYY"
XXX_prepend += "YYY"
XXX_remove += "YYY"
Using the old syntax :D? Er, yes. What was I thinking exactly? :) Anyway, you all knew what I meant!
and the ?=, =+, .= and =. variants. This also covers variants with overrides in
there too. It would basically mean that you need to use "=" with the append
operator and friends.
We found one reference of that use in OE-Core and one in meta-intel. There are
others in other layers but the usage isn't that widespread.
Why should we warn against this? The += usage in particular is sometimes used to
add a space so:
XXX_append += "YYY"
and
XXX_append = " YYY"
are equivalent but this is more of an accident rather than a specific bitbake
design. It is extremely confusing to new users and has been complained about
before as an issue effecting usability. New users often think that += will stack
the append strings much like += does on a normal variable but that is not the
case and can lead to confusion.
I'm been looking at ways we can simplify without hurting functionality and
having this slightly obscure usage doesn't really seem to help much. If we agree
to do this, I would make it an error in due course after users have seen the
warnings for a while. I'd probably want to do that before the LTS next year.
I'm all for removing potential areas of confusion, so +1.
On that topic, if I'm not mistaken, the same applies to:
FOO:.*{append, prepend, remove}:.*{append,prepend,remove}
? Basically it does not make much sense to have (any) two of append, prepend,
remove as overrides of the same variable (on the same line).
I think anything with two or more such operators should just error as that has never been supported and shouldn't/couldn't do much that was useful. I'd just make that a hard error now. Cheers, Richard
|
|
|
