* [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
@ 2021-11-15 23:53 Ricardo Martincoski
2021-12-10 19:09 ` Arnout Vandecappelle
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ricardo Martincoski @ 2021-11-15 23:53 UTC (permalink / raw)
To: buildroot; +Cc: Thomas Petazzoni, Ricardo Martincoski, Titouan Christophe
Currently this .mk snippet results in unexpected behavior from
check-package:
|VAR_1 = VALUE1
|ifeq (condition)
|VAR_1 := $(VAR_1), VALUE2
|endif
Fix commit "163f160a8e utils/{check-package, checkpackagelib}:
consistently use raw strings for re.compile" that ended up doing this:
- CONCATENATING = re.compile("^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
+ CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
But raw strings do not expect escaping when referencing \1 and the
pattern ends up searching for a raw '\\1' instead of an occurrence of
the first pattern inside parenthesis.
|$ python3
|Python 3.8.10 (default, Sep 28 2021, 16:10:42)
|[GCC 9.3.0] on linux
|Type "help", "copyright", "credits" or "license" for more information.
|>>> import re
|>>> p1 = re.compile('(foo)bar\\1')
|>>> p2 = re.compile(r'(foo)bar\\1')
|>>> p3 = re.compile(r'(foo)bar\1')
|>>> s1 = 'foobarfoo'
|>>> s2 = 'foobar\\1'
|>>> print(p1.search(s1))
|<re.Match object; span=(0, 9), match='foobarfoo'>
|>>> print(p2.search(s1))
|None
|>>> print(p3.search(s1))
|<re.Match object; span=(0, 9), match='foobarfoo'>
|>>> print(p1.search(s2))
|None
|>>> print(p2.search(s2))
|<re.Match object; span=(0, 8), match='foobar\\1'>
|>>> print(p3.search(s2))
|None
|>>>
So use '\1' instead of '\\1' in the raw string.
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Titouan Christophe <titouan.christophe@railnova.eu>
---
Caught this regression using unit tests.
Before:
https://gitlab.com/RicardoMartincoski/check-package/-/jobs/1785343039
After:
https://gitlab.com/RicardoMartincoski/check-package/-/jobs/1785364353
---
utils/checkpackagelib/lib_mk.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 0278354434..572fe75990 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -77,7 +77,7 @@ class Indent(_CheckFunction):
class OverriddenVariable(_CheckFunction):
- CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
+ CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\1\)")
END_CONDITIONAL = re.compile(r"^\s*({})".format("|".join(end_conditional)))
OVERRIDING_ASSIGNMENTS = [':=', "="]
START_CONDITIONAL = re.compile(r"^\s*({})".format("|".join(start_conditional)))
--
2.25.1
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
2021-11-15 23:53 [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable Ricardo Martincoski
@ 2021-12-10 19:09 ` Arnout Vandecappelle
2021-12-26 22:39 ` Thomas Petazzoni
2022-01-14 16:05 ` Peter Korsgaard
2 siblings, 0 replies; 6+ messages in thread
From: Arnout Vandecappelle @ 2021-12-10 19:09 UTC (permalink / raw)
To: Ricardo Martincoski, buildroot; +Cc: Thomas Petazzoni, Titouan Christophe
On 16/11/2021 00:53, Ricardo Martincoski wrote:
> Currently this .mk snippet results in unexpected behavior from
> check-package:
> |VAR_1 = VALUE1
> |ifeq (condition)
> |VAR_1 := $(VAR_1), VALUE2
> |endif
>
> Fix commit "163f160a8e utils/{check-package, checkpackagelib}:
> consistently use raw strings for re.compile" that ended up doing this:
> - CONCATENATING = re.compile("^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
> + CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
>
> But raw strings do not expect escaping when referencing \1 and the
> pattern ends up searching for a raw '\\1' instead of an occurrence of
> the first pattern inside parenthesis.
>
> |$ python3
> |Python 3.8.10 (default, Sep 28 2021, 16:10:42)
> |[GCC 9.3.0] on linux
> |Type "help", "copyright", "credits" or "license" for more information.
> |>>> import re
> |>>> p1 = re.compile('(foo)bar\\1')
> |>>> p2 = re.compile(r'(foo)bar\\1')
> |>>> p3 = re.compile(r'(foo)bar\1')
> |>>> s1 = 'foobarfoo'
> |>>> s2 = 'foobar\\1'
> |>>> print(p1.search(s1))
> |<re.Match object; span=(0, 9), match='foobarfoo'>
> |>>> print(p2.search(s1))
> |None
> |>>> print(p3.search(s1))
> |<re.Match object; span=(0, 9), match='foobarfoo'>
> |>>> print(p1.search(s2))
> |None
> |>>> print(p2.search(s2))
> |<re.Match object; span=(0, 8), match='foobar\\1'>
> |>>> print(p3.search(s2))
> |None
> |>>>
>
> So use '\1' instead of '\\1' in the raw string.
>
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Titouan Christophe <titouan.christophe@railnova.eu>
Wow, great explanation!
Applied to master, thanks.
Regards,
Arnout
> ---
> Caught this regression using unit tests.
> Before:
> https://gitlab.com/RicardoMartincoski/check-package/-/jobs/1785343039
> After:
> https://gitlab.com/RicardoMartincoski/check-package/-/jobs/1785364353
> ---
> utils/checkpackagelib/lib_mk.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
> index 0278354434..572fe75990 100644
> --- a/utils/checkpackagelib/lib_mk.py
> +++ b/utils/checkpackagelib/lib_mk.py
> @@ -77,7 +77,7 @@ class Indent(_CheckFunction):
>
>
> class OverriddenVariable(_CheckFunction):
> - CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
> + CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\1\)")
> END_CONDITIONAL = re.compile(r"^\s*({})".format("|".join(end_conditional)))
> OVERRIDING_ASSIGNMENTS = [':=', "="]
> START_CONDITIONAL = re.compile(r"^\s*({})".format("|".join(start_conditional)))
>
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
2021-11-15 23:53 [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable Ricardo Martincoski
2021-12-10 19:09 ` Arnout Vandecappelle
@ 2021-12-26 22:39 ` Thomas Petazzoni
2021-12-27 10:45 ` ricardo.martincoski
2022-01-14 16:05 ` Peter Korsgaard
2 siblings, 1 reply; 6+ messages in thread
From: Thomas Petazzoni @ 2021-12-26 22:39 UTC (permalink / raw)
To: Ricardo Martincoski; +Cc: Titouan Christophe, Fabrice Fontaine, buildroot
Hello Ricardo,
On Mon, 15 Nov 2021 20:53:36 -0300
Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:
> Currently this .mk snippet results in unexpected behavior from
> check-package:
> |VAR_1 = VALUE1
> |ifeq (condition)
> |VAR_1 := $(VAR_1), VALUE2
> |endif
There is apparently still a problem with this check. Indeed, I just
committed 1118f2c51c357d968e2d08e31ad3c741f5fa7df8 which adds an
unconditional:
+# https://www.mail-archive.com/lttng-dev@lists.lttng.org/msg12950.html
+LTTNG_LIBUST_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -DUATOMIC_NO_LINK_ERROR"
but even though it is outside of any condition, check-package complains:
package/lttng-libust/lttng-libust.mk:30: conditional override of variable LTTNG_LIBUST_CONF_ENV
I have not investigated further this issue for now.
Best regards,
Thomas
--
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
2021-12-26 22:39 ` Thomas Petazzoni
@ 2021-12-27 10:45 ` ricardo.martincoski
2021-12-27 11:05 ` Thomas Petazzoni
0 siblings, 1 reply; 6+ messages in thread
From: ricardo.martincoski @ 2021-12-27 10:45 UTC (permalink / raw)
To: thomas.petazzoni; +Cc: titouan.christophe, fontaine.fabrice, buildroot
[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]
Hello Thomas,
On Sun, Dec 26, 2021 at 07:39 PM, Thomas Petazzoni wrote:
> On Mon, 15 Nov 2021 20:53:36 -0300
> Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote:
>
>> Currently this .mk snippet results in unexpected behavior from
>> check-package:
>> |VAR_1 = VALUE1
>> |ifeq (condition)
>> |VAR_1 := $(VAR_1), VALUE2
>> |endif
>
> There is apparently still a problem with this check. Indeed, I just
> committed 1118f2c51c357d968e2d08e31ad3c741f5fa7df8 which adds an
> unconditional:
>
> +# https://www.mail-archive.com/lttng-dev@lists.lttng.org/msg12950.html
> +LTTNG_LIBUST_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -DUATOMIC_NO_LINK_ERROR"
>
> but even though it is outside of any condition, check-package complains:
>
> package/lttng-libust/lttng-libust.mk:30: conditional override of variable LTTNG_LIBUST_CONF_ENV
20 |# https://www.mail-archive.com/lttng-dev@lists.lttng.org/msg12950.html
21 |LTTNG_LIBUST_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -DUATOMIC_NO_LINK_ERROR"
22 |
23 |ifeq ($(BR2_PACKAGE_PYTHON),y)
24 |LTTNG_LIBUST_DEPENDENCIES += python
25 |LTTNG_LIBUST_CONF_OPTS += --enable-python-agent
26 |else ifeq ($(BR2_PACKAGE_PYTHON3),y)
27 |LTTNG_LIBUST_DEPENDENCIES += python3
28 |LTTNG_LIBUST_CONF_OPTS += --enable-python-agent
29 |else
30 |LTTNG_LIBUST_CONF_ENV = am_cv_pathless_PYTHON="none"
31 |LTTNG_LIBUST_CONF_OPTS += --disable-python-agent
32 |endif
Well, it seems that line 30 is indeed overriding line 21 when python is not
enabled.
As check-package is today, except for _DEPENDENCIES, all other variables can
have either:
|ifeq (condition)
|VAR_1 = value
|endif
or:
|ifeq (condition)
|VAR_1 += value
|endif
check-package will only complain when there is an actual override:
|VAR_1 = value
|ifeq (condition)
|VAR_1 = value
|endif
We could, for instance, change it to always expect:
|ifeq (condition)
|VAR_1 += value
|endif
... catching potential overrides.
Regards,
Ricardo
[-- Attachment #2: Type: text/plain, Size: 150 bytes --]
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
2021-12-27 10:45 ` ricardo.martincoski
@ 2021-12-27 11:05 ` Thomas Petazzoni
0 siblings, 0 replies; 6+ messages in thread
From: Thomas Petazzoni @ 2021-12-27 11:05 UTC (permalink / raw)
To: ricardo.martincoski; +Cc: titouan.christophe, fontaine.fabrice, buildroot
Hello Ricardo,
On Mon, 27 Dec 2021 07:45:00 -0300
ricardo.martincoski@gmail.com wrote:
> 20 |# https://www.mail-archive.com/lttng-dev@lists.lttng.org/msg12950.html
> 21 |LTTNG_LIBUST_CONF_ENV = CFLAGS="$(TARGET_CFLAGS) -DUATOMIC_NO_LINK_ERROR"
> 22 |
> 23 |ifeq ($(BR2_PACKAGE_PYTHON),y)
> 24 |LTTNG_LIBUST_DEPENDENCIES += python
> 25 |LTTNG_LIBUST_CONF_OPTS += --enable-python-agent
> 26 |else ifeq ($(BR2_PACKAGE_PYTHON3),y)
> 27 |LTTNG_LIBUST_DEPENDENCIES += python3
> 28 |LTTNG_LIBUST_CONF_OPTS += --enable-python-agent
> 29 |else
> 30 |LTTNG_LIBUST_CONF_ENV = am_cv_pathless_PYTHON="none"
> 31 |LTTNG_LIBUST_CONF_OPTS += --disable-python-agent
> 32 |endif
>
> Well, it seems that line 30 is indeed overriding line 21 when python is not
> enabled.
>
> As check-package is today, except for _DEPENDENCIES, all other variables can
> have either:
>
> |ifeq (condition)
> |VAR_1 = value
> |endif
>
> or:
>
> |ifeq (condition)
> |VAR_1 += value
> |endif
>
> check-package will only complain when there is an actual override:
>
> |VAR_1 = value
> |ifeq (condition)
> |VAR_1 = value
> |endif
Aah, that is the part that I missed!
> We could, for instance, change it to always expect:
>
> |ifeq (condition)
> |VAR_1 += value
> |endif
>
> ... catching potential overrides.
Well, I just merged code that does this:
ifeq ($(BR2_ENDIAN),"BIG")
BENTO4_BYTE_ORDER = 0
else
BENTO4_BYTE_ORDER = 1
endif
and here we don't want to force a +=.
So I guess the current behavior of check-package is correct!
Thanks for the clarification!
Thomas
--
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable
2021-11-15 23:53 [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable Ricardo Martincoski
2021-12-10 19:09 ` Arnout Vandecappelle
2021-12-26 22:39 ` Thomas Petazzoni
@ 2022-01-14 16:05 ` Peter Korsgaard
2 siblings, 0 replies; 6+ messages in thread
From: Peter Korsgaard @ 2022-01-14 16:05 UTC (permalink / raw)
To: Ricardo Martincoski; +Cc: Titouan Christophe, Thomas Petazzoni, buildroot
>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:
> Currently this .mk snippet results in unexpected behavior from
> check-package:
> |VAR_1 = VALUE1
> |ifeq (condition)
> |VAR_1 := $(VAR_1), VALUE2
> |endif
> Fix commit "163f160a8e utils/{check-package, checkpackagelib}:
> consistently use raw strings for re.compile" that ended up doing this:
> - CONCATENATING = re.compile("^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
> + CONCATENATING = re.compile(r"^([A-Z0-9_]+)\s*(\+|:|)=\s*\$\(\\1\)")
> But raw strings do not expect escaping when referencing \1 and the
> pattern ends up searching for a raw '\\1' instead of an occurrence of
> the first pattern inside parenthesis.
> |$ python3
> |Python 3.8.10 (default, Sep 28 2021, 16:10:42)
> |[GCC 9.3.0] on linux
> |Type "help", "copyright", "credits" or "license" for more information.
> |>>> import re
> |>>> p1 = re.compile('(foo)bar\\1')
> |>>> p2 = re.compile(r'(foo)bar\\1')
> |>>> p3 = re.compile(r'(foo)bar\1')
> |>>> s1 = 'foobarfoo'
> |>>> s2 = 'foobar\\1'
> |>>> print(p1.search(s1))
> |<re.Match object; span=(0, 9), match='foobarfoo'>
> |>>> print(p2.search(s1))
> |None
> |>>> print(p3.search(s1))
> |<re.Match object; span=(0, 9), match='foobarfoo'>
> |>>> print(p1.search(s2))
> |None
> |>>> print(p2.search(s2))
> |<re.Match object; span=(0, 8), match='foobar\\1'>
> |>>> print(p3.search(s2))
> |None
> |>>>
> So use '\1' instead of '\\1' in the raw string.
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Titouan Christophe <titouan.christophe@railnova.eu>
Committed to 2021.02.x and 2021.11.x, thanks.
--
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-01-14 16:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-15 23:53 [Buildroot] [PATCH] utils/checkpackagelib/lib_mk.py: fix check for overridden variable Ricardo Martincoski
2021-12-10 19:09 ` Arnout Vandecappelle
2021-12-26 22:39 ` Thomas Petazzoni
2021-12-27 10:45 ` ricardo.martincoski
2021-12-27 11:05 ` Thomas Petazzoni
2022-01-14 16:05 ` Peter Korsgaard
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.