* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
@ 2019-07-14 12:44 Arnout Vandecappelle
2019-07-14 13:15 ` Yann E. MORIN
0 siblings, 1 reply; 6+ messages in thread
From: Arnout Vandecappelle @ 2019-07-14 12:44 UTC (permalink / raw)
To: buildroot
The CommentsMenusPackagesOrder check builds the 'state' to track the
depth of menus and conditions. However, a menuconfig doesn't create a
menu by itself - it is always followed by a condition that implies the
menu. As a result, when unwinding the 'state', the level will be wrong.
Fix this by checking for menu followed by a space, so it no longer
matches menuconfig.
Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/251214899
Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
Cc: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com>
---
utils/checkpackagelib/lib_config.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utils/checkpackagelib/lib_config.py b/utils/checkpackagelib/lib_config.py
index f0edb9993d..94faf1b0fc 100644
--- a/utils/checkpackagelib/lib_config.py
+++ b/utils/checkpackagelib/lib_config.py
@@ -73,7 +73,7 @@ class CommentsMenusPackagesOrder(_CheckFunction):
def check_line(self, lineno, text):
if text.startswith("comment") or text.startswith("if") or \
- text.startswith("menu"):
+ text.startswith("menu "):
if text.startswith("comment"):
if not self.state.endswith("-comment"):
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
2019-07-14 12:44 [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling Arnout Vandecappelle
@ 2019-07-14 13:15 ` Yann E. MORIN
2019-07-14 13:23 ` Yann E. MORIN
0 siblings, 1 reply; 6+ messages in thread
From: Yann E. MORIN @ 2019-07-14 13:15 UTC (permalink / raw)
To: buildroot
Arnout, All,
On 2019-07-14 14:44 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> The CommentsMenusPackagesOrder check builds the 'state' to track the
> depth of menus and conditions. However, a menuconfig doesn't create a
> menu by itself - it is always followed by a condition that implies the
> menu. As a result, when unwinding the 'state', the level will be wrong.
>
> Fix this by checking for menu followed by a space, so it no longer
> matches menuconfig.
>
> Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/251214899
No, it does not fix it totally. It only fixes the python traceback.
The Kodi issues is till present.
Hint: the Kodi package is the only one that indents the "source" lines
with a TAB.
Regards,
Yann E. MORIN.
> Signed-off-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> Cc: Jerzy Grzegorek <jerzy.m.grzegorek@gmail.com>
> ---
> utils/checkpackagelib/lib_config.py | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/utils/checkpackagelib/lib_config.py b/utils/checkpackagelib/lib_config.py
> index f0edb9993d..94faf1b0fc 100644
> --- a/utils/checkpackagelib/lib_config.py
> +++ b/utils/checkpackagelib/lib_config.py
> @@ -73,7 +73,7 @@ class CommentsMenusPackagesOrder(_CheckFunction):
>
> def check_line(self, lineno, text):
> if text.startswith("comment") or text.startswith("if") or \
> - text.startswith("menu"):
> + text.startswith("menu "):
>
> if text.startswith("comment"):
> if not self.state.endswith("-comment"):
> --
> 2.21.0
>
> _______________________________________________
> 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] 6+ messages in thread
* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
2019-07-14 13:15 ` Yann E. MORIN
@ 2019-07-14 13:23 ` Yann E. MORIN
2019-07-14 18:50 ` Arnout Vandecappelle
0 siblings, 1 reply; 6+ messages in thread
From: Yann E. MORIN @ 2019-07-14 13:23 UTC (permalink / raw)
To: buildroot
Arnout, Jerzy, All,
(sorry, I sent the previous one too quickly)
On 2019-07-14 15:15 +0200, Yann E. MORIN spake thusly:
> On 2019-07-14 14:44 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
[--SNIP--]
> > Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/251214899
> No, it does not fix it totally. It only fixes the python traceback.
> The Kodi issues is till present.
> Hint: the Kodi package is the only one that indents the "source" lines
> with a TAB.
If one changes Kodi to not indent the source lines, then the issue
disappears. Of course, this is not the correct solution, but...
If one changes another package to also idnent the source lines with a
TAB, then the error happens there too (for example, fftw):
package/fftw/Config.in:18: Packages in: if BR2_PACKAGE_FFTW,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: ftw/fftw-d
OK, so, weird. The package included is in fact fftw/fftw-double and only
part of the bname is reported.
But if one also looks more closely at the Kodi issue, packages names are
also incorrectly reported:
package/kodi/Config.in:303: Packages in: menu "Audio decoder addons",
[--SNIP--]
first incorrect package: kodi-audiodecoder
And in fact, there is one thing I don;t understand in utils/checkpackagelib/lib_config.py,
line 106:
new_package = text[17: -(len(self.filename)-5):]
Why are we using the current filename to strip away parts of the new
package filename?
Hope that helps.
Regards,
Yann E. MORIN.
--
.-----------------.--------------------.------------------.--------------------.
| 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] 6+ messages in thread
* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
2019-07-14 13:23 ` Yann E. MORIN
@ 2019-07-14 18:50 ` Arnout Vandecappelle
2019-07-14 20:24 ` Arnout Vandecappelle
0 siblings, 1 reply; 6+ messages in thread
From: Arnout Vandecappelle @ 2019-07-14 18:50 UTC (permalink / raw)
To: buildroot
On 14/07/2019 15:23, Yann E. MORIN wrote:
> Arnout, Jerzy, All,
>
> (sorry, I sent the previous one too quickly)
>
> On 2019-07-14 15:15 +0200, Yann E. MORIN spake thusly:
>> On 2019-07-14 14:44 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
> [--SNIP--]
>>> Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/251214899
>> No, it does not fix it totally. It only fixes the python traceback.
>> The Kodi issues is till present.
>> Hint: the Kodi package is the only one that indents the "source" lines
>> with a TAB.
>
> If one changes Kodi to not indent the source lines, then the issue
> disappears. Of course, this is not the correct solution, but...
Yes, because the test only looks at source lines which are indented, which is
wrong too. It should be a regex.
>
> If one changes another package to also idnent the source lines with a
> TAB, then the error happens there too (for example, fftw):
>
> package/fftw/Config.in:18: Packages in: if BR2_PACKAGE_FFTW,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: ftw/fftw-d
>
> OK, so, weird. The package included is in fact fftw/fftw-double and only
> part of the bname is reported.
>
> But if one also looks more closely at the Kodi issue, packages names are
> also incorrectly reported:
>
> package/kodi/Config.in:303: Packages in: menu "Audio decoder addons",
> [--SNIP--]
> first incorrect package: kodi-audiodecoder
>
> And in fact, there is one thing I don;t understand in utils/checkpackagelib/lib_config.py,
> line 106:
>
> new_package = text[17: -(len(self.filename)-5):]
>
> Why are we using the current filename to strip away parts of the new
> package filename?
That is indeed the problem. I didn't look too hard at that line (because I
already looked to much at all the rest :-). The len(self.filename)-5 works for
package/Config.in because that exactly strips off the /Config.in part of the
line. But that's a horrible hack, of course...
I'll look at correcting it shortly.
Regards,
Arnout
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
2019-07-14 18:50 ` Arnout Vandecappelle
@ 2019-07-14 20:24 ` Arnout Vandecappelle
2019-07-14 20:47 ` Yann E. MORIN
0 siblings, 1 reply; 6+ messages in thread
From: Arnout Vandecappelle @ 2019-07-14 20:24 UTC (permalink / raw)
To: buildroot
On 14/07/2019 20:50, Arnout Vandecappelle wrote:
>
>
> On 14/07/2019 15:23, Yann E. MORIN wrote:
>> Arnout, Jerzy, All,
>>
>> (sorry, I sent the previous one too quickly)
>>
>> On 2019-07-14 15:15 +0200, Yann E. MORIN spake thusly:
>>> On 2019-07-14 14:44 +0200, Arnout Vandecappelle (Essensium/Mind) spake thusly:
>> [--SNIP--]
>>>> Fixes: https://gitlab.com/buildroot.org/buildroot/-/jobs/251214899
>>> No, it does not fix it totally. It only fixes the python traceback.
>>> The Kodi issues is till present.
>>> Hint: the Kodi package is the only one that indents the "source" lines
>>> with a TAB.
>>
>> If one changes Kodi to not indent the source lines, then the issue
>> disappears. Of course, this is not the correct solution, but...
>
> Yes, because the test only looks at source lines which are indented, which is
> wrong too. It should be a regex.
So, I changed this into a regex...
>
>>
>> If one changes another package to also idnent the source lines with a
>> TAB, then the error happens there too (for example, fftw):
>>
>> package/fftw/Config.in:18: Packages in: if BR2_PACKAGE_FFTW,
>> are not alphabetically ordered;
>> correct order: '-', '_', digits, capitals, lowercase;
>> first incorrect package: ftw/fftw-d
>>
>> OK, so, weird. The package included is in fact fftw/fftw-double and only
>> part of the bname is reported.
>>
>> But if one also looks more closely at the Kodi issue, packages names are
>> also incorrectly reported:
>>
>> package/kodi/Config.in:303: Packages in: menu "Audio decoder addons",
>> [--SNIP--]
>> first incorrect package: kodi-audiodecoder
>>
>> And in fact, there is one thing I don;t understand in utils/checkpackagelib/lib_config.py,
>> line 106:
>>
>> new_package = text[17: -(len(self.filename)-5):]
>>
>> Why are we using the current filename to strip away parts of the new
>> package filename?
>
> That is indeed the problem. I didn't look too hard at that line (because I
> already looked to much at all the rest :-). The len(self.filename)-5 works for
> package/Config.in because that exactly strips off the /Config.in part of the
> line. But that's a horrible hack, of course...
... and used the same regex to get the package name.
It now seems to work correctly. However, it turns up 10 "errors":
boot/Config.in:7: Packages in: menu "Bootloaders",
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: arm-trusted-firmware
package/fftw/Config.in:18: Packages in: if BR2_PACKAGE_FFTW,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: fftw-double
package/gstreamer/Config.in:7: Packages in: if BR2_PACKAGE_GSTREAMER,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: gst-plugins-bad
package/gstreamer1/Config.in:6: Packages in: if BR2_PACKAGE_GSTREAMER1,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals,
lowercase;
first incorrect package: gst1-plugins-base
package/opengl/Config.in:2: Packages in: ,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: libegl
package/qt5/Config.in:84: Packages in: comment "Latest Qt version needs
host/toolchain w/ gcc >= 4.8",
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: qt5webengine
package/freescale-imx/Config.in:96: Packages in: if BR2_PACKAGE_FREESCALE_IMX,
are not alphabetically ordered;
correct order: '-', '_', digits, capitals,
lowercase;
first incorrect package: firmware-imx
toolchain/toolchain-buildroot/Config.in:109: Packages in: comment "glibc on MIPS
w/ NAN2008 needs a toolchain w/ headers >= 4.5",
are not alphabetically ordered;
correct order: '-', '_', digits,
capitals, lowercase;
first incorrect package: glibc
toolchain/toolchain-external/Config.in:17: Packages in: comment "glibc
toolchains only available with shared lib support",
are not alphabetically ordered;
correct order: '-', '_', digits,
capitals, lowercase;
first incorrect package:
toolchain-external-codesourcery-aarch64
toolchain/Config.in:70: Packages in: menu "Toolchain",
are not alphabetically ordered;
correct order: '-', '_', digits, capitals, lowercase;
first incorrect package: gdb
I haven't looked at the details yet, but I think most of them are bogus. So,
maybe we should just do it for package/Config.in and package/Config.in.host.
However, some of them *are* relevant: external toolchains, bootloaders, maybe
qt5... Note however that for those the "comment" handling is not correct (in
package/Config.in, comments are generally used to indicate submenus, but in
other Config.in files this is not the case).
Ideas?
Regards,
Arnout
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling
2019-07-14 20:24 ` Arnout Vandecappelle
@ 2019-07-14 20:47 ` Yann E. MORIN
0 siblings, 0 replies; 6+ messages in thread
From: Yann E. MORIN @ 2019-07-14 20:47 UTC (permalink / raw)
To: buildroot
Arnout, All,
On 2019-07-14 22:24 +0200, Arnout Vandecappelle spake thusly:
> On 14/07/2019 20:50, Arnout Vandecappelle wrote:
> >> On 2019-07-14 15:15 +0200, Yann E. MORIN spake thusly:
[--SNIP--]
> >> And in fact, there is one thing I don;t understand in utils/checkpackagelib/lib_config.py,
> >> line 106:
> >> new_package = text[17: -(len(self.filename)-5):]
> >> Why are we using the current filename to strip away parts of the new
> >> package filename?
> > That is indeed the problem. I didn't look too hard at that line (because I
> > already looked to much at all the rest :-). The len(self.filename)-5 works for
> > package/Config.in because that exactly strips off the /Config.in part of the
> > line. But that's a horrible hack, of course...
> ... and used the same regex to get the package name.
>
> It now seems to work correctly. However, it turns up 10 "errors":
>
> boot/Config.in:7: Packages in: menu "Bootloaders",
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: arm-trusted-firmware
Good catch.
> package/fftw/Config.in:18: Packages in: if BR2_PACKAGE_FFTW,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: fftw-double
Unfortunately, this is where we readh the limitation: we do want the
list to be ordered not by name, but by the precision. And indeed, the
current ordering makes more sense for fftw than an alphabetical one
would.
> package/gstreamer/Config.in:7: Packages in: if BR2_PACKAGE_GSTREAMER,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: gst-plugins-bad
Ditto, the current ordering is correct.
> package/gstreamer1/Config.in:6: Packages in: if BR2_PACKAGE_GSTREAMER1,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals,
> lowercase;
> first incorrect package: gst1-plugins-base
Ditto.
> package/opengl/Config.in:2: Packages in: ,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: libegl
This is more questionable, because the current ordering make sense, but
the packages are all vrtual and have no prompt. So it would not be too
bad for users, only hackers.
> package/qt5/Config.in:84: Packages in: comment "Latest Qt version needs
> host/toolchain w/ gcc >= 4.8",
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: qt5webengine
Here I am not so sure. I'd like to ensure that the "core" packages come
first, and that we were not forced to choose the alphabetical order. But
it so happens that their names naturally fits the bill (more or less).
As for the web monsters, yes, they should be alphabetically sorted.
> package/freescale-imx/Config.in:96: Packages in: if BR2_PACKAGE_FREESCALE_IMX,
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals,
> lowercase;
> first incorrect package: firmware-imx
Here, even if we were to move imx-sc-firmware (the one really not
ordered), we'd still have to put it after more "low-level" packages. And
there are also imx-gpu-g2d and imx-gpu-viv that may cause false
positives.
> toolchain/toolchain-buildroot/Config.in:109: Packages in: comment "glibc on MIPS
> w/ NAN2008 needs a toolchain w/ headers >= 4.5",
> are not alphabetically ordered;
> correct order: '-', '_', digits,
> capitals, lowercase;
> first incorrect package: glibc
> toolchain/toolchain-external/Config.in:17: Packages in: comment "glibc
> toolchains only available with shared lib support",
> are not alphabetically ordered;
> correct order: '-', '_', digits,
> capitals, lowercase;
> first incorrect package:
> toolchain-external-codesourcery-aarch64
> toolchain/Config.in:70: Packages in: menu "Toolchain",
> are not alphabetically ordered;
> correct order: '-', '_', digits, capitals, lowercase;
> first incorrect package: gdb
Toolchain is always a little bit special, isn't it? :-)
> I haven't looked at the details yet, but I think most of them are bogus.
There are two that are correct (ATF, and qt5webengin). The others we
want to ignore.
> So,
> maybe we should just do it for package/Config.in and package/Config.in.host.
> However, some of them *are* relevant: external toolchains, bootloaders, maybe
> qt5... Note however that for those the "comment" handling is not correct (in
> package/Config.in, comments are generally used to indicate submenus, but in
> other Config.in files this is not the case).
>
> Ideas?
I like the flake8 notation:
# noqa: NO_ORDER
Regards,
Yann E. MORIN.
> Regards,
> Arnout
--
.-----------------.--------------------.------------------.--------------------.
| 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] 6+ messages in thread
end of thread, other threads:[~2019-07-14 20:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-14 12:44 [Buildroot] [PATCH] utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling Arnout Vandecappelle
2019-07-14 13:15 ` Yann E. MORIN
2019-07-14 13:23 ` Yann E. MORIN
2019-07-14 18:50 ` Arnout Vandecappelle
2019-07-14 20:24 ` Arnout Vandecappelle
2019-07-14 20:47 ` Yann E. MORIN
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox