All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define
@ 2021-11-25 21:56 Fabrice Fontaine
  2021-11-26  7:42 ` Arnout Vandecappelle
  0 siblings, 1 reply; 4+ messages in thread
From: Fabrice Fontaine @ 2021-11-25 21:56 UTC (permalink / raw)
  To: buildroot; +Cc: Fabrice Fontaine, Ricardo Martincoski

Check typo in define to detect SMAKE_LINUX_CONFIG_FIXUPS in smack
(fixed by 41e2132fbe8a8fc237ca4a2cd2eff9bd9ced09a6)

Add GCC_TARGET in ALLOWED variable to avoid the following warnings:

arch/arch.mk:12: possible typo: GCC_TARGET_ARCH -> *ARCH*
arch/arch.mk:13: possible typo: GCC_TARGET_ABI -> *ARCH*
arch/arch.mk:14: possible typo: GCC_TARGET_NAN -> *ARCH*
arch/arch.mk:15: possible typo: GCC_TARGET_FP32_MODE -> *ARCH*
arch/arch.mk:16: possible typo: GCC_TARGET_CPU -> *ARCH*
arch/arch.mk:17: possible typo: GCC_TARGET_FPU -> *ARCH*
arch/arch.mk:18: possible typo: GCC_TARGET_FLOAT_ABI -> *ARCH*
arch/arch.mk:19: possible typo: GCC_TARGET_MODE -> *ARCH*

Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
---
 utils/checkpackagelib/lib_mk.py | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index d09922f724..9178931276 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -236,6 +236,7 @@ class TypoInPackageVariable(_CheckFunction):
         "BR_CCACHE_INITIAL_SETUP",
         "BR_LIBC",
         "BR_NO_CHECK_HASH_FOR",
+        "GCC_TARGET",
         "LINUX_EXTENSIONS",
         "LINUX_POST_PATCH_HOOKS",
         "LINUX_TOOLS",
@@ -248,7 +249,7 @@ class TypoInPackageVariable(_CheckFunction):
         "TARGET_FINALIZE_HOOKS",
         "TARGETS_ROOTFS",
         "XTENSA_CORE_NAME"]))
-    VARIABLE = re.compile(r"^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
+    VARIABLE = re.compile(r"^(define )?([A-Z0-9_]+_[A-Z0-9_]+)")
 
     def before(self):
         package, _ = os.path.splitext(os.path.basename(self.filename))
@@ -258,7 +259,7 @@ class TypoInPackageVariable(_CheckFunction):
         # linux extensions do not use LINUX_EXT_ prefix for variables
         package = package.replace("LINUX_EXT_", "")
         self.package = package
-        self.REGEX = re.compile(r"^(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
+        self.REGEX = re.compile(r"(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
         self.FIND_VIRTUAL = re.compile(
             r"^{}_PROVIDES\s*(\+|)=\s*(.*)".format(package))
         self.virtual = []
@@ -268,7 +269,7 @@ class TypoInPackageVariable(_CheckFunction):
         if m is None:
             return
 
-        variable = m.group(1)
+        variable = m.group(2)
 
         # allow to set variables for virtual package this package provides
         v = self.FIND_VIRTUAL.search(text)
-- 
2.33.0

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define
  2021-11-25 21:56 [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define Fabrice Fontaine
@ 2021-11-26  7:42 ` Arnout Vandecappelle
  2021-11-26 17:26   ` Fabrice Fontaine
  0 siblings, 1 reply; 4+ messages in thread
From: Arnout Vandecappelle @ 2021-11-26  7:42 UTC (permalink / raw)
  To: Fabrice Fontaine, buildroot; +Cc: Ricardo Martincoski



On 25/11/2021 22:56, Fabrice Fontaine wrote:
> Check typo in define to detect SMAKE_LINUX_CONFIG_FIXUPS in smack
> (fixed by 41e2132fbe8a8fc237ca4a2cd2eff9bd9ced09a6)
> 
> Add GCC_TARGET in ALLOWED variable to avoid the following warnings:
> 
> arch/arch.mk:12: possible typo: GCC_TARGET_ARCH -> *ARCH*
> arch/arch.mk:13: possible typo: GCC_TARGET_ABI -> *ARCH*
> arch/arch.mk:14: possible typo: GCC_TARGET_NAN -> *ARCH*
> arch/arch.mk:15: possible typo: GCC_TARGET_FP32_MODE -> *ARCH*
> arch/arch.mk:16: possible typo: GCC_TARGET_CPU -> *ARCH*
> arch/arch.mk:17: possible typo: GCC_TARGET_FPU -> *ARCH*
> arch/arch.mk:18: possible typo: GCC_TARGET_FLOAT_ABI -> *ARCH*
> arch/arch.mk:19: possible typo: GCC_TARGET_MODE -> *ARCH*
> 
> Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> ---
>   utils/checkpackagelib/lib_mk.py | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
> index d09922f724..9178931276 100644
> --- a/utils/checkpackagelib/lib_mk.py
> +++ b/utils/checkpackagelib/lib_mk.py
> @@ -236,6 +236,7 @@ class TypoInPackageVariable(_CheckFunction):
>           "BR_CCACHE_INITIAL_SETUP",
>           "BR_LIBC",
>           "BR_NO_CHECK_HASH_FOR",
> +        "GCC_TARGET",
>           "LINUX_EXTENSIONS",
>           "LINUX_POST_PATCH_HOOKS",
>           "LINUX_TOOLS",
> @@ -248,7 +249,7 @@ class TypoInPackageVariable(_CheckFunction):
>           "TARGET_FINALIZE_HOOKS",
>           "TARGETS_ROOTFS",
>           "XTENSA_CORE_NAME"]))
> -    VARIABLE = re.compile(r"^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
> +    VARIABLE = re.compile(r"^(define )?([A-Z0-9_]+_[A-Z0-9_]+)")

  This no longer matches inline assignments... So I think this should be 
something like
^(define )?([A-Z0-9_]+_[A-Z0-9_]+)|^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=

  Ideally, we should have a runtime test that validates that check-package 
catches the errors we want it to catch...

>   
>       def before(self):
>           package, _ = os.path.splitext(os.path.basename(self.filename))
> @@ -258,7 +259,7 @@ class TypoInPackageVariable(_CheckFunction):
>           # linux extensions do not use LINUX_EXT_ prefix for variables
>           package = package.replace("LINUX_EXT_", "")
>           self.package = package
> -        self.REGEX = re.compile(r"^(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
> +        self.REGEX = re.compile(r"(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
>           self.FIND_VIRTUAL = re.compile(
>               r"^{}_PROVIDES\s*(\+|)=\s*(.*)".format(package))
>           self.virtual = []
> @@ -268,7 +269,7 @@ class TypoInPackageVariable(_CheckFunction):
>           if m is None:
>               return
>   
> -        variable = m.group(1)
> +        variable = m.group(2)

  Ah, with the regex that matches both, this will be tricky... Perhaps we should 
use named groups then.


  Regards,
  Arnout


>   
>           # allow to set variables for virtual package this package provides
>           v = self.FIND_VIRTUAL.search(text)
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define
  2021-11-26  7:42 ` Arnout Vandecappelle
@ 2021-11-26 17:26   ` Fabrice Fontaine
  2021-11-29  0:30     ` Ricardo Martincoski
  0 siblings, 1 reply; 4+ messages in thread
From: Fabrice Fontaine @ 2021-11-26 17:26 UTC (permalink / raw)
  To: Arnout Vandecappelle; +Cc: Ricardo Martincoski, Buildroot Mailing List

Dear Arnout,

Le ven. 26 nov. 2021 à 08:42, Arnout Vandecappelle <arnout@mind.be> a écrit :
>
>
>
> On 25/11/2021 22:56, Fabrice Fontaine wrote:
> > Check typo in define to detect SMAKE_LINUX_CONFIG_FIXUPS in smack
> > (fixed by 41e2132fbe8a8fc237ca4a2cd2eff9bd9ced09a6)
> >
> > Add GCC_TARGET in ALLOWED variable to avoid the following warnings:
> >
> > arch/arch.mk:12: possible typo: GCC_TARGET_ARCH -> *ARCH*
> > arch/arch.mk:13: possible typo: GCC_TARGET_ABI -> *ARCH*
> > arch/arch.mk:14: possible typo: GCC_TARGET_NAN -> *ARCH*
> > arch/arch.mk:15: possible typo: GCC_TARGET_FP32_MODE -> *ARCH*
> > arch/arch.mk:16: possible typo: GCC_TARGET_CPU -> *ARCH*
> > arch/arch.mk:17: possible typo: GCC_TARGET_FPU -> *ARCH*
> > arch/arch.mk:18: possible typo: GCC_TARGET_FLOAT_ABI -> *ARCH*
> > arch/arch.mk:19: possible typo: GCC_TARGET_MODE -> *ARCH*
> >
> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
> > ---
> >   utils/checkpackagelib/lib_mk.py | 7 ++++---
> >   1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
> > index d09922f724..9178931276 100644
> > --- a/utils/checkpackagelib/lib_mk.py
> > +++ b/utils/checkpackagelib/lib_mk.py
> > @@ -236,6 +236,7 @@ class TypoInPackageVariable(_CheckFunction):
> >           "BR_CCACHE_INITIAL_SETUP",
> >           "BR_LIBC",
> >           "BR_NO_CHECK_HASH_FOR",
> > +        "GCC_TARGET",
> >           "LINUX_EXTENSIONS",
> >           "LINUX_POST_PATCH_HOOKS",
> >           "LINUX_TOOLS",
> > @@ -248,7 +249,7 @@ class TypoInPackageVariable(_CheckFunction):
> >           "TARGET_FINALIZE_HOOKS",
> >           "TARGETS_ROOTFS",
> >           "XTENSA_CORE_NAME"]))
> > -    VARIABLE = re.compile(r"^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
> > +    VARIABLE = re.compile(r"^(define )?([A-Z0-9_]+_[A-Z0-9_]+)")
>
>   This no longer matches inline assignments... So I think this should be
> something like
> ^(define )?([A-Z0-9_]+_[A-Z0-9_]+)|^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=
Can you give me an example of an inline assignment that is not catched anymore?

This new expression will catch:
SMAKE_CONF_OPTS
as well as
define SMAKE_LINUX_CONFIG_FIXUPS

I made only two modifications:
 - add (define )? which will match "define " but also an empty value.
Thanks to this, the second group will always contain the variable
name.
 - remove \s*(\+|)= which seems superfluous
>
>   Ideally, we should have a runtime test that validates that check-package
> catches the errors we want it to catch...
>
> >
> >       def before(self):
> >           package, _ = os.path.splitext(os.path.basename(self.filename))
> > @@ -258,7 +259,7 @@ class TypoInPackageVariable(_CheckFunction):
> >           # linux extensions do not use LINUX_EXT_ prefix for variables
> >           package = package.replace("LINUX_EXT_", "")
> >           self.package = package
> > -        self.REGEX = re.compile(r"^(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
> > +        self.REGEX = re.compile(r"(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
> >           self.FIND_VIRTUAL = re.compile(
> >               r"^{}_PROVIDES\s*(\+|)=\s*(.*)".format(package))
> >           self.virtual = []
> > @@ -268,7 +269,7 @@ class TypoInPackageVariable(_CheckFunction):
> >           if m is None:
> >               return
> >
> > -        variable = m.group(1)
> > +        variable = m.group(2)
>
>   Ah, with the regex that matches both, this will be tricky... Perhaps we should
> use named groups then.
>
>
>   Regards,
>   Arnout
>
>
> >
> >           # allow to set variables for virtual package this package provides
> >           v = self.FIND_VIRTUAL.search(text)
> >
Best Regards,

Fabrice
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define
  2021-11-26 17:26   ` Fabrice Fontaine
@ 2021-11-29  0:30     ` Ricardo Martincoski
  0 siblings, 0 replies; 4+ messages in thread
From: Ricardo Martincoski @ 2021-11-29  0:30 UTC (permalink / raw)
  To: fontaine.fabrice, arnout; +Cc: buildroot

[-- Attachment #1: Type: text/plain, Size: 5129 bytes --]

Fabrice,
Arnout,

On Fri, Nov 26, 2021 at 02:26 PM, Fabrice Fontaine wrote:

> Dear Arnout,
> 
> Le ven. 26 nov. 2021 à 08:42, Arnout Vandecappelle <arnout@mind.be> a écrit :
>>
>>
>>
>> On 25/11/2021 22:56, Fabrice Fontaine wrote:
>> > Check typo in define to detect SMAKE_LINUX_CONFIG_FIXUPS in smack
>> > (fixed by 41e2132fbe8a8fc237ca4a2cd2eff9bd9ced09a6)

Great to have this new check.

>> >
>> > Add GCC_TARGET in ALLOWED variable to avoid the following warnings:
>> >
>> > arch/arch.mk:12: possible typo: GCC_TARGET_ARCH -> *ARCH*
>> > arch/arch.mk:13: possible typo: GCC_TARGET_ABI -> *ARCH*
>> > arch/arch.mk:14: possible typo: GCC_TARGET_NAN -> *ARCH*
>> > arch/arch.mk:15: possible typo: GCC_TARGET_FP32_MODE -> *ARCH*
>> > arch/arch.mk:16: possible typo: GCC_TARGET_CPU -> *ARCH*
>> > arch/arch.mk:17: possible typo: GCC_TARGET_FPU -> *ARCH*
>> > arch/arch.mk:18: possible typo: GCC_TARGET_FLOAT_ABI -> *ARCH*
>> > arch/arch.mk:19: possible typo: GCC_TARGET_MODE -> *ARCH*
>> >
>> > Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com>
>> > ---
>> >   utils/checkpackagelib/lib_mk.py | 7 ++++---
>> >   1 file changed, 4 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
>> > index d09922f724..9178931276 100644
>> > --- a/utils/checkpackagelib/lib_mk.py
>> > +++ b/utils/checkpackagelib/lib_mk.py
>> > @@ -236,6 +236,7 @@ class TypoInPackageVariable(_CheckFunction):
>> >           "BR_CCACHE_INITIAL_SETUP",
>> >           "BR_LIBC",
>> >           "BR_NO_CHECK_HASH_FOR",
>> > +        "GCC_TARGET",
>> >           "LINUX_EXTENSIONS",
>> >           "LINUX_POST_PATCH_HOOKS",
>> >           "LINUX_TOOLS",
>> > @@ -248,7 +249,7 @@ class TypoInPackageVariable(_CheckFunction):
>> >           "TARGET_FINALIZE_HOOKS",
>> >           "TARGETS_ROOTFS",
>> >           "XTENSA_CORE_NAME"]))
>> > -    VARIABLE = re.compile(r"^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
>> > +    VARIABLE = re.compile(r"^(define )?([A-Z0-9_]+_[A-Z0-9_]+)")

Do not assume that there will be only one space after 'define':
-    VARIABLE = re.compile(r"^(define )?([A-Z0-9_]+_[A-Z0-9_]+)")
+    VARIABLE = re.compile(r"^(define\s+)?([A-Z0-9_]+_[A-Z0-9_]+)")

If we decide to check, for instance, that we always want only one space after
'define', we could do it (there would be 2 warnings today), but IMO it should be
a different rule (a new function in the lib_mk.py).

>>
>>   This no longer matches inline assignments... So I think this should be
>> something like
>> ^(define )?([A-Z0-9_]+_[A-Z0-9_]+)|^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=
> Can you give me an example of an inline assignment that is not catched anymore?
> 
> This new expression will catch:
> SMAKE_CONF_OPTS
> as well as
> define SMAKE_LINUX_CONFIG_FIXUPS
> 
> I made only two modifications:
>  - add (define )? which will match "define " but also an empty value.
> Thanks to this, the second group will always contain the variable
> name.
>  - remove \s*(\+|)= which seems superfluous

Indeed, I was unable to come up with a snippet to justify to keep the
\s*(\+|)= part.

So, with the change from space to \s+ mentioned above, you can add my
 Reviewed-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>

>>
>>   Ideally, we should have a runtime test that validates that check-package
>> catches the errors we want it to catch...

Working on this, I hope I can send it in December.
I created some unit tests but I still need to connect them to the runtime test
infra.
https://gitlab.com/RicardoMartincoski/check-package/-/blob/13bda7652063a72cc3f6137b20dd9b9064e757f3/pytest/test_lib_mk.py#L198

Regards,
Ricardo

>>
>> >
>> >       def before(self):
>> >           package, _ = os.path.splitext(os.path.basename(self.filename))
>> > @@ -258,7 +259,7 @@ class TypoInPackageVariable(_CheckFunction):
>> >           # linux extensions do not use LINUX_EXT_ prefix for variables
>> >           package = package.replace("LINUX_EXT_", "")
>> >           self.package = package
>> > -        self.REGEX = re.compile(r"^(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
>> > +        self.REGEX = re.compile(r"(HOST_|ROOTFS_)?({}_[A-Z0-9_]+)".format(package))
>> >           self.FIND_VIRTUAL = re.compile(
>> >               r"^{}_PROVIDES\s*(\+|)=\s*(.*)".format(package))
>> >           self.virtual = []
>> > @@ -268,7 +269,7 @@ class TypoInPackageVariable(_CheckFunction):
>> >           if m is None:
>> >               return
>> >
>> > -        variable = m.group(1)
>> > +        variable = m.group(2)
>>
>>   Ah, with the regex that matches both, this will be tricky... Perhaps we should
>> use named groups then.
>>
>>
>>   Regards,
>>   Arnout
>>
>>
>> >
>> >           # allow to set variables for virtual package this package provides
>> >           v = self.FIND_VIRTUAL.search(text)
>> >
> Best Regards,
> 
> Fabrice


[-- 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] 4+ messages in thread
end of thread, other threads:[~2021-11-29  0:30 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-25 21:56 [Buildroot] [PATCH/next 1/1] utils/checkpackagelib/lib_mk.py: check typo in define Fabrice Fontaine
2021-11-26  7:42 ` Arnout Vandecappelle
2021-11-26 17:26   ` Fabrice Fontaine
2021-11-29  0:30     ` Ricardo Martincoski

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.