| Message ID | 20171218084303.12472-1-thomas.petazzoni@free-electrons.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | utils/checkpackagelib: exclude two files from Config.in indentation check | expand |
Thomas, All, On 2017-12-18 09:43 +0100, Thomas Petazzoni spake thusly: > package/x11r7/Config.in and package/kodi/Config.in do not comply with > the normal Config.in indentation rules. However, this violation of the > rule is legitimate, so let's skip them in check-package for this > specific indentation check. Can't we simply fix those two files to not have that unusual indentation in the first place? AFAICS, that special indentation is about the 'source' lines... Surely that does not bring much to have them indented, and they could simply be left-aligned... Regards, Yann E. MORIN. > This removes the last 314 remaining warnings on Config.in files. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > Note: I am not totally sure about this patch. Indeed package/Config.in > uses the same rule as package/{x11r7,kodi}/Config.in, but > check-package doesn't report warnings about it. Perhaps I'm missing > something in the check-package logic. > --- > utils/checkpackagelib/lib_config.py | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/utils/checkpackagelib/lib_config.py b/utils/checkpackagelib/lib_config.py > index 26ebb393d1..fb39182b91 100644 > --- a/utils/checkpackagelib/lib_config.py > +++ b/utils/checkpackagelib/lib_config.py > @@ -133,6 +133,10 @@ class Indent(_CheckFunction): > text] > elif entry in entries_that_should_not_be_indented: > if not text.startswith(entry): > + # two Config.in files have a special but legitimate indentation rule > + if self.filename in [ "./package/x11r7/Config.in", > + "./package/kodi/Config.in" ]: > + return > return ["{}:{}: should not be indented" > .format(self.filename, lineno), > text] > -- > 2.14.3 > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hello, On Mon, Dec 18, 2017 at 06:43 AM, Thomas Petazzoni wrote: > package/x11r7/Config.in and package/kodi/Config.in do not comply with > the normal Config.in indentation rules. However, this violation of the > rule is legitimate, so let's skip them in check-package for this > specific indentation check. > > This removes the last 314 remaining warnings on Config.in files. There are also 7 warnings for Config.in.host They can obviously be fixed in another patch(es). After this/these we can add -o -name 'Config.in' or -o -name 'Config.*' to the check-package job in the gitlab yml, but I guess it is your plan already. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > --- > Note: I am not totally sure about this patch. Indeed package/Config.in > uses the same rule as package/{x11r7,kodi}/Config.in, but > check-package doesn't report warnings about it. Perhaps I'm missing > something in the check-package logic. See FILE_IS_FROM_A_PACKAGE in the main script. I first limited the script to files in the package directory because there are a lot of false warnings from other directories, many of them for historical reasons. And... well... it also named check-"package". I planned to expand it first to boot, then to to linux and toolchain, but I didn't get to it yet. > --- [snip] > + if self.filename in [ "./package/x11r7/Config.in", > + "./package/kodi/Config.in" ]: The 4 warnings from flake8 already in the file I plan to fix in a series for all flake8 warnings in the tree (those 4 actually adding '# noqa' since check-package uses 'inspect' to know which check functions to run). But (in the case you stick to this solution, see the other e-mail) could you fix those 2 new warnings? utils/checkpackagelib/lib_config.py:137:38: E201 whitespace after '[' utils/checkpackagelib/lib_config.py:138:65: E202 whitespace before ']' Regards, Ricardo
Hello, On Mon, Dec 18, 2017 at 08:03 PM, Yann E. MORIN wrote: > On 2017-12-18 09:43 +0100, Thomas Petazzoni spake thusly: >> package/x11r7/Config.in and package/kodi/Config.in do not comply with >> the normal Config.in indentation rules. However, this violation of the >> rule is legitimate, so let's skip them in check-package for this >> specific indentation check. > > Can't we simply fix those two files to not have that unusual indentation > in the first place? > > AFAICS, that special indentation is about the 'source' lines... Surely > that does not bring much to have them indented, and they could simply be > left-aligned... Makes sense to me. Let's include the developers for those 2 files to see if they have arguments to keep or change. Regards, Ricardo
[posted and mailed] Ricardo Martincoski <ricardo.martincoski@gmail.com> wrote in news:5a3872168413e_bd03fa7729df43c853dd@ultri3.mail: >> AFAICS, that special indentation is about the 'source' lines... Surely >> that does not bring much to have them indented, and they could simply >> be left-aligned... > > Makes sense to me. > Let's include the developers for those 2 files to see if they have > arguments to keep or change. Hi, the layout of package/x11r7/Config.in was introduced in the initial commit more than ten years ago: https://git.busybox.net/buildroot/commit/package/x11r7/Config.in? id=a7e49eb2af5d2ca6e53fb908fddfddd92696910a When starting to add more Kodi packages since 2015 https://git.busybox.net/buildroot/commit/package/kodi/Config.in?id= 81c438aa207e8fdea776ae946a4d2d3f57cc143f I used a similar layout without any particular reason. I have no objections to remove the current indentations to make checkpackagelib happy. Regards, Bernd
Hello, On Mon, 18 Dec 2017 23:03:44 +0100, Yann E. MORIN wrote: > Can't we simply fix those two files to not have that unusual indentation > in the first place? > > AFAICS, that special indentation is about the 'source' lines... Surely > that does not bring much to have them indented, and they could simply be > left-aligned... package/Config.in is also indented in the same way, and there's a reason: when you do a diff, diff will tell you in which section the change is. For example, if I do a change in the middle of "Audio and video applications", I get: diff --git a/package/Config.in b/package/Config.in index bd39a374f0..dbcf53d1ce 100644 --- a/package/Config.in +++ b/package/Config.in @@ -31,7 +31,7 @@ menu "Audio and video applications" source "package/mimic/Config.in" source "package/miraclecast/Config.in" source "package/mjpegtools/Config.in" - source "package/modplugtools/Config.in" + source "package/foomodplugtools/Config.in" source "package/motion/Config.in" source "package/mpd/Config.in" source "package/mpd-mpc/Config.in" See the @@ menu "Audio and video applications" ? You wouldn't get this without the indentation. So the indentation does serve a purpose, and has been intentionally added, at least to package/Config.in. I think the same reason/motivation applies to package/kodi/Config.in and package/x11r7/Config.in. Best regards, Thomas Petazzoni
Hello, On Mon, 18 Dec 2017 23:56:29 -0200, Ricardo Martincoski wrote: > On Mon, Dec 18, 2017 at 06:43 AM, Thomas Petazzoni wrote: > > > package/x11r7/Config.in and package/kodi/Config.in do not comply with > > the normal Config.in indentation rules. However, this violation of the > > rule is legitimate, so let's skip them in check-package for this > > specific indentation check. > > > > This removes the last 314 remaining warnings on Config.in files. > > There are also 7 warnings for Config.in.host > They can obviously be fixed in another patch(es). Ah, right, forgot about Config.in.host. > After this/these we can add > -o -name 'Config.in' > or > -o -name 'Config.*' > to the check-package job in the gitlab yml, but I guess it is your plan already. This is obviously the plan. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > > --- > > Note: I am not totally sure about this patch. Indeed package/Config.in > > uses the same rule as package/{x11r7,kodi}/Config.in, but > > check-package doesn't report warnings about it. Perhaps I'm missing > > something in the check-package logic. > > See FILE_IS_FROM_A_PACKAGE in the main script. > I first limited the script to files in the package directory because there are > a lot of false warnings from other directories, many of them for historical > reasons. And... well... it also named check-"package". > I planned to expand it first to boot, then to to linux and toolchain, but I > didn't get to it yet. Ah, ok, makes sense. I think we will progressively want to extend it indeed, starting first with bootloaders, linux and toolchain as you suggest. > > + if self.filename in [ "./package/x11r7/Config.in", > > + "./package/kodi/Config.in" ]: > > The 4 warnings from flake8 already in the file I plan to fix in a series for > all flake8 warnings in the tree (those 4 actually adding '# noqa' since > check-package uses 'inspect' to know which check functions to run). > > But (in the case you stick to this solution, see the other e-mail) could you > fix those 2 new warnings? > utils/checkpackagelib/lib_config.py:137:38: E201 whitespace after '[' > utils/checkpackagelib/lib_config.py:138:65: E202 whitespace before ']' ACK. I don't use flake8, so I don't notice such warnings. I'll fix and resend if we agree that this is the appropriate solution. Best regards, Thomas
Hello, On Tue, Dec 19, 2017 at 06:36 AM, Thomas Petazzoni wrote: > On Mon, 18 Dec 2017 23:56:29 -0200, Ricardo Martincoski wrote: > >> On Mon, Dec 18, 2017 at 06:43 AM, Thomas Petazzoni wrote: >> [snip] >> > + if self.filename in [ "./package/x11r7/Config.in", >> > + "./package/kodi/Config.in" ]: >> >> The 4 warnings from flake8 already in the file I plan to fix in a series for >> all flake8 warnings in the tree (those 4 actually adding '# noqa' since >> check-package uses 'inspect' to know which check functions to run). >> >> But (in the case you stick to this solution, see the other e-mail) could you >> fix those 2 new warnings? >> utils/checkpackagelib/lib_config.py:137:38: E201 whitespace after '[' >> utils/checkpackagelib/lib_config.py:138:65: E202 whitespace before ']' > > ACK. I don't use flake8, so I don't notice such warnings. I'll fix and > resend if we agree that this is the appropriate solution. Thank you for explaining (in the other e-mail) the reasoning of the special indentation. One last thought about the code... Until now the script can be called passing relative or absolute filenames to check. These 3 commands have the same result: $ ./utils/check-package $(find $(readlink -f package) -type f) >/dev/null $ ./utils/check-package $(find package -type f) >/dev/null $ ./utils/check-package $(find ./package -type f) >/dev/null To keep this behavior we could use instead: if self.filename.endswith("package/x11r7/Config.in") or \ self.filename.endswith("package/kodi/Config.in"): or its equivalent in one line (it fits in 132). Regards, Ricardo
Hello, On Tue, 19 Dec 2017 21:32:04 -0200, Ricardo Martincoski wrote: > Thank you for explaining (in the other e-mail) the reasoning of the special > indentation. > > > One last thought about the code... > > Until now the script can be called passing relative or absolute filenames to > check. These 3 commands have the same result: > $ ./utils/check-package $(find $(readlink -f package) -type f) >/dev/null > $ ./utils/check-package $(find package -type f) >/dev/null > $ ./utils/check-package $(find ./package -type f) >/dev/null > > To keep this behavior we could use instead: > if self.filename.endswith("package/x11r7/Config.in") or \ > self.filename.endswith("package/kodi/Config.in"): > or its equivalent in one line (it fits in 132). Sure, makes sense. Thomas
Thomas, All, On 2017-12-19 09:28 +0100, Thomas Petazzoni spake thusly: > On Mon, 18 Dec 2017 23:03:44 +0100, Yann E. MORIN wrote: > > Can't we simply fix those two files to not have that unusual indentation > > in the first place? > > > > AFAICS, that special indentation is about the 'source' lines... Surely > > that does not bring much to have them indented, and they could simply be > > left-aligned... > > package/Config.in is also indented in the same way, and there's a > reason: when you do a diff, diff will tell you in which section the > change is. I know very well why package/Config.in is indented, I even Acked the patch doing so, back in the day... ;-) I'm just questioning if we could instead drop that indentation in the two other files, because I believe it does not bring much for them. > For example, if I do a change in the middle of "Audio and > video applications", I get: > > diff --git a/package/Config.in b/package/Config.in > index bd39a374f0..dbcf53d1ce 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -31,7 +31,7 @@ menu "Audio and video applications" > source "package/mimic/Config.in" > source "package/miraclecast/Config.in" > source "package/mjpegtools/Config.in" > - source "package/modplugtools/Config.in" > + source "package/foomodplugtools/Config.in" > source "package/motion/Config.in" > source "package/mpd/Config.in" > source "package/mpd-mpc/Config.in" > > See the @@ menu "Audio and video applications" ? You wouldn't get this > without the indentation. Thanks, I know how to read a diff... ;-) But it does not always work, see: diff --git a/package/Config.in b/package/Config.in index bd39a374f0..83bb126cb8 100644 --- a/package/Config.in +++ b/package/Config.in @@ -312,6 +312,7 @@ endif source "package/tekui/Config.in" source "package/weston/Config.in" source "package/x11r7/Config.in" + source "package/blabla/Config.in" comment "X applications" depends on BR2_PACKAGE_XORG7 @@ -375,6 +376,7 @@ endmenu source "package/a10disp/Config.in" source "package/acpica/Config.in" source "package/acpid/Config.in" + source "package/foo/Config.in" source "package/acpitool/Config.in" source "package/aer-inject/Config.in" source "package/am335x-pru-package/Config.in" > So the indentation does serve a purpose, and has been intentionally > added, at least to package/Config.in. I think the same > reason/motivation applies to package/kodi/Config.in and > package/x11r7/Config.in. Arguably, and that's what I'm sayging: at least for those last two files, we could well drop the idnentation because it is not so useful (although it works better for them than for package/Config.in). But oh well... Regards, Yann E. MORIN. > Best regards, > > Thomas Petazzoni > -- > Thomas Petazzoni, CTO, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com
Hello, I am creating a series to improve check-package. There will be not many changes to the code itself, but I will add an entry to the manual, fix scanpypi to generate compliant help text, enable to check other directories, add Config.* to GitLab job and fix the remaining warnings to make GitLab job to pass. So let me adopt this patch and include it into my series. It will be very similar to your original patch. I think it is feasible to submit the series (~ 50 patches to make the review easier) before the hackathon. Regards, Ricardo
Hello, On Thu, 22 Mar 2018 00:20:44 -0300, Ricardo Martincoski wrote: > I am creating a series to improve check-package. > There will be not many changes to the code itself, but I will add an entry to > the manual, fix scanpypi to generate compliant help text, enable to check other > directories, add Config.* to GitLab job and fix the remaining warnings to make > GitLab job to pass. > > So let me adopt this patch and include it into my series. It will be very > similar to your original patch. > > I think it is feasible to submit the series (~ 50 patches to make the review > easier) before the hackathon. Sure, feel free to adopt my patch. Thanks! Thomas
diff --git a/utils/checkpackagelib/lib_config.py b/utils/checkpackagelib/lib_config.py index 26ebb393d1..fb39182b91 100644 --- a/utils/checkpackagelib/lib_config.py +++ b/utils/checkpackagelib/lib_config.py @@ -133,6 +133,10 @@ class Indent(_CheckFunction): text] elif entry in entries_that_should_not_be_indented: if not text.startswith(entry): + # two Config.in files have a special but legitimate indentation rule + if self.filename in [ "./package/x11r7/Config.in", + "./package/kodi/Config.in" ]: + return return ["{}:{}: should not be indented" .format(self.filename, lineno), text]
package/x11r7/Config.in and package/kodi/Config.in do not comply with the normal Config.in indentation rules. However, this violation of the rule is legitimate, so let's skip them in check-package for this specific indentation check. This removes the last 314 remaining warnings on Config.in files. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- Note: I am not totally sure about this patch. Indeed package/Config.in uses the same rule as package/{x11r7,kodi}/Config.in, but check-package doesn't report warnings about it. Perhaps I'm missing something in the check-package logic. --- utils/checkpackagelib/lib_config.py | 4 ++++ 1 file changed, 4 insertions(+)