| Message ID | 20190714124400.29431-1-arnout@mind.be |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | utils/checkpackagelib: CommentsMenusPackagesOrder: fix 'menuconfig' handling | expand |
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@busybox.net > http://lists.busybox.net/mailman/listinfo/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.
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
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
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
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"):
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(-)