[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[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.

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] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Yann E. MORIN]

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 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 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Ricardo Martincoski]

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

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Ricardo Martincoski]

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

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Bernd Kuhls]

[posted and mailed]

Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote in
news:5a3872168413e_bd03fa7729df43c853dd at 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

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Ricardo Martincoski]

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

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Thomas Petazzoni]

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 Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Yann E. MORIN]

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

-- 
.-----------------.--------------------.------------------.--------------------.
|  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.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Ricardo Martincoski]

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

[Buildroot] [PATCH] utils/checkpackagelib: exclude two files from Config.in indentation check

[Thomas Petazzoni]

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
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com