utils/checkpackagelib: exclude two files from Config.in indentation check
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
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.
---
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(+)
utils/checkpackagelib/lib_config.py +4 -0
@@ -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]
Signature · 1 line
2.14.3
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Thomas, All,
On 2017-12-18 09:43 +0100, Thomas Petazzoni spake thusly:
Quoted 4 lines
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.
Quoted 35 lines
This removes the last 314 remaining warnings on Config.in files.
Signed-off-by Thomas Petazzoni <[email protected]>---
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(+)utils/checkpackagelib/lib_config.py +4
-0@@ -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]Signature · 6 lines
2.14.3 _______________________________________________ buildroot mailing list buildroot at busybox.net http://lists.busybox.net/mailman/listinfo/buildroot
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Hello,
Quoted 6 lines · Yann E. MORIN
On 2017-12-18 09:43 +0100, Thomas Petazzoni spake thusly:
Quoted 4 lines
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.
Quoted 6 lines
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
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
[posted and mailed]
Ricardo Martincoski <[email protected]> wrote in news:5a3872168413e_bd03fa7729df43c853dd at ultri3.mail:
Quoted 7 lines
Quoted 3 lines
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
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
In reply to Yann E. MORINHello,
Quoted 6 lines · Yann E. MORIN
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:
package/Config.in +1 -1
@@ -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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Thomas, All,
On 2017-12-19 09:28 +0100, Thomas Petazzoni spake thusly:
Quoted 11 lines · Yann E. MORIN
Quoted 6 lines
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.
Quoted 17 lines
For example, if I do a change in the middle of "Audio and video applications", I get:
package/Config.in +1
-1@@ -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"
Quoted 2 lines
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:
package/Config.in +2 -0
@@ -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"
Quoted 4 lines
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.
Quoted 7 lines
Best regards,
Thomas Petazzoni
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
Signature · 6 lines
.-----------------.--------------------.------------------.--------------------. | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: | | +33 662 376 056 | Software Designer | \ / CAMPAIGN | ___ | | +33 223 225 172 `------------.-------: X AGAINST | \e/ There is no | | http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL | v conspiracy. | '------------------------------^-------^------------------^--------------------'
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
In reply to Thomas PetazzoniHello,
Quoted 6 lines · Thomas Petazzoni
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.
Quoted 8 lines
Signed-off-by Thomas Petazzoni <[email protected]>---
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.
Quoted 1 line
---
[snip]
utils/checkpackagelib/lib_config.py class Indent(_CheckFunction): · line 138
Quoted 2 lines
utils/checkpackagelib/lib_config.py +2
-0+ 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
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Hello,
Quoted 11 lines · Ricardo Martincoski
Quoted 6 lines · Thomas Petazzoni
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.
Quoted 5 lines
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.
Quoted 8 lines
Quoted 7 lines
Signed-off-by Thomas Petazzoni <[email protected]>---
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.
Quoted 6 lines
See
FILE_IS_FROM_A_PACKAGEin 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.
Quoted 11 lines
utils/checkpackagelib/lib_config.py class Indent(_CheckFunction): · line 138
Quoted 2 lines
utils/checkpackagelib/lib_config.py +2
-0+ 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Hello,
Quoted 4 lines · Thomas Petazzoni
Quoted 2 lines · Ricardo Martincoski
On Mon, Dec 18, 2017 at 06:43 AM, Thomas Petazzoni wrote:
[snip]
Quoted 7 lines
Quoted 7 lines
utils/checkpackagelib/lib_config.py class Indent(_CheckFunction): · line 138
Quoted 2 lines
utils/checkpackagelib/lib_config.py +2
-0+ 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).
Quoted 7 lines
Quoted 4 lines
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
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Hello,
Quoted 8 lines · Ricardo Martincoski
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:
Quoted 8 lines
$ ./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/nullTo 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
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
In reply to Thomas PetazzoniHello,
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
PATCH 1/1 utils/checkpackagelib: exclude two files from Config.in indentation check
Hello,
Quoted 11 lines · Ricardo Martincoski
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
Signature · 3 lines
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com