[2/9] support/scripts/check-package: new script

Create the infra to check the style of new packages before submitting.
The overall function of the script is described inside a txt file.
It is designed to process the actual files and NOT the patch files
generated by git format-patch.

Also add the first check function, to warn if a file (Config.in*, *.mk,
*.hash, *.patch) has no newline at the last line of the file, see [1].

Basic usage for simple packages:
support/scripts/check-package -vvv package/newpackage/*

Basic usage for packages with subdirs:
support/scripts/check-package -vvv $(find package/newpackage/ -type f)

See "checkpackage" in [2].

[1] http://patchwork.ozlabs.org/patch/631129/
[2] http://elinux.org/Buildroot#Todo_list

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m0.415s
    user	0m0.392s
    sys	0m0.024s
    
    CHECK_NEWLINE_AT_EOF:
     support/scripts/check-package --include-only check_newline_at_eof \
     $(find package -type f) 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_newline_at_eof -vv package/*/*)
      package/package1/wrong-name.patch:13: missing newline at end of file
       #include <curses.h>
      159 lines processed
      1 warnings generated

 support/scripts/check-package             | 136 ++++++++++++++++++++++++++++++
 support/scripts/check-package.txt         |  76 +++++++++++++++++
 support/scripts/checkpackagelib.py        |  18 ++++
 support/scripts/checkpackagelib_config.py |   7 ++
 support/scripts/checkpackagelib_hash.py   |   7 ++
 support/scripts/checkpackagelib_mk.py     |   8 ++
 support/scripts/checkpackagelib_patch.py  |   7 ++
 7 files changed, 259 insertions(+)
 create mode 100755 support/scripts/check-package
 create mode 100644 support/scripts/check-package.txt
 create mode 100644 support/scripts/checkpackagelib.py
 create mode 100644 support/scripts/checkpackagelib_config.py
 create mode 100644 support/scripts/checkpackagelib_hash.py
 create mode 100644 support/scripts/checkpackagelib_mk.py
 create mode 100644 support/scripts/checkpackagelib_patch.py

Hi Ricardo,

On Sat, Dec 31, 2016 at 4:21 AM, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
> Create the infra to check the style of new packages before submitting.
> The overall function of the script is described inside a txt file.
> It is designed to process the actual files and NOT the patch files
> generated by git format-patch.
>
[..]

Interesting!
Please check below where I have some comments on the implementation of
the infrastructure.

> diff --git a/support/scripts/check-package b/support/scripts/check-package
> new file mode 100755
> index 000000000..42af82e2c
> --- /dev/null
> +++ b/support/scripts/check-package
> @@ -0,0 +1,136 @@
> +#!/usr/bin/env python
> +# See support/scripts/check-package.txt before editing this file.
> +
> +from __future__ import print_function
> +import argparse
> +import inspect
> +import re
> +import sys
> +
> +import checkpackagelib_config
> +import checkpackagelib_hash
> +import checkpackagelib_mk
> +import checkpackagelib_patch
> +
> +
> +def parse_args():
> +    parser = argparse.ArgumentParser()
> +
> +    # Do not use argparse.FileType("r") here because only files with known
> +    # format will be open based on the filename.
> +    parser.add_argument("files", metavar="F", type=str, nargs="*",
> +                        help="list of files")
> +
> +    parser.add_argument("--manual-url", action="store",
> +                        default="http://nightly.buildroot.org/",
> +                        help="default: %(default)s")
> +    parser.add_argument("--verbose", "-v", action="count", default=0)
> +
> +    # Now the debug options in the order they are processed.
> +    parser.add_argument("--include-only", dest="include_list", action="append",
> +                        help="run only the specified functions (debug)")
> +    parser.add_argument("--exclude", dest="exclude_list", action="append",
> +                        help="do not run the specified functions (debug)")
> +    parser.add_argument("--dry-run", action="store_true", help="print the "
> +                        "functions that would be called for each file (debug)")
> +
> +    return parser.parse_args()
> +
> +
> +CONFIG_IN_FILENAME = re.compile("/Config\.in(\.host)?$")

Beware, there are packages with different config filenames:

linux/Config.ext.in
linux/Config.tools.in
package/php/Config.ext
package/qt/Config.gfx.in
package/qt/Config.keyboard.in
package/qt/Config.mouse.in
package/qt/Config.sql.in


> +FILE_IS_FROM_A_PACKAGE = re.compile("package/[^/]*/")
> +
> +
> +def get_lib_from_filename(fname):
> +    if FILE_IS_FROM_A_PACKAGE.search(fname) is None:
> +        return None
> +    if CONFIG_IN_FILENAME.search(fname):
> +        return checkpackagelib_config
> +    if fname.endswith(".hash"):
> +        return checkpackagelib_hash
> +    if fname.endswith(".mk"):
> +        return checkpackagelib_mk
> +    if fname.endswith(".patch"):
> +        return checkpackagelib_patch
> +    return None
> +
> +
> +def call_check_function(function, fname, args, **kwargs):

What are these 'args' ? Shouldn't it be '*args' with asterisk?
https://pythontips.com/2013/08/04/args-and-kwargs-in-python-explained/
Same in other functions.

> +    warnings = function(fname, args, **kwargs)
> +
> +    # Avoid the need to use 'return []' at the end of every check function.
> +    if warnings is None:
> +        return 0  # No warning generated.
> +
> +    for level, message in enumerate(warnings):
> +        if args.verbose >= level:
> +            print(message.replace("\t", "< tab  >").rstrip())
> +    return 1  # One more warning to count.
> +
> +
> +def check_file_using_lib(fname, args):
> +    # Count number of warnings generated and lines processed.
> +    nwarnings = 0
> +    nlines = 0
> +
> +    lib = get_lib_from_filename(fname)
> +    if not lib:
> +        if args.verbose >= 3:

perhaps define named constants instead of magic '3' ?

> +            print("{}: ignored".format(fname))
> +        return nwarnings, nlines
> +    callbacks = inspect.getmembers(lib, inspect.isfunction)
> +
> +    # Do not call helper functions.
> +    callbacks = [cb for cb in callbacks if not cb[0].startswith("_")]
> +
> +    if args.include_list:
> +        callbacks = [cb for cb in callbacks if cb[0] in args.include_list]
> +    if args.exclude_list:
> +        callbacks = [cb for cb in callbacks if cb[0] not in args.exclude_list]

It looks cleaner to me to create a custom function with all the checks
(no helper functions, include list, exclude list) and pass that to
inspect.getmembers, rather than doing it in four steps. The custom
function should then return True only if the function should be kept,
and False otherwise. See
https://docs.python.org/2/library/inspect.html#inspect.getmembers

> +
> +    if args.dry_run:
> +        functions_to_run = [cb[0] for cb in callbacks]
> +        print("{}: would run: {}".format(fname, functions_to_run))
> +        return nwarnings, nlines
> +
> +    for cb in callbacks:
> +        nwarnings += call_check_function(cb[1], fname, args, start=True)
> +    for lineno, text in enumerate(open(fname, "r").readlines()):
> +        nlines += 1
> +        for cb in callbacks:
> +            nwarnings += call_check_function(cb[1], fname, args,
> +                                             lineno=lineno + 1, text=text)
> +    for cb in callbacks:
> +        nwarnings += call_check_function(cb[1], fname, args, end=True)

I find the concept with start and end parameter odd and unpythonic.
Why do you not let each checker be a class, rather than a function?
Each class can then define a start, check and end method (or other
names), which you call above.

> +
> +    return nwarnings, nlines
> +
> +
> +def __main__():
> +    args = parse_args()
> +
> +    if len(args.files) == 0:
> +        print("No files to check style")
> +        sys.exit(1)
> +
> +    # Accumulate number of warnings generated and lines processed.
> +    total_warnings = 0
> +    total_lines = 0
> +
> +    for fname in args.files:
> +        nwarnings, nlines = check_file_using_lib(fname, args)
> +        total_warnings += nwarnings
> +        total_lines += nlines
> +
> +    # The warning messages are printed to stdout and can be post-processed
> +    # (e.g. counted by 'wc'), so for stats use stderr. Wait all warnings are
> +    # printed, for the case there are many of them, before printing stats.
> +    sys.stdout.flush()
> +    print("{} lines processed".format(total_lines), file=sys.stderr)
> +    print("{} warnings generated".format(total_warnings), file=sys.stderr)
> +
> +    if total_warnings > 0:
> +        sys.exit(1)
> +
> +
> +__main__()
> diff --git a/support/scripts/check-package.txt b/support/scripts/check-package.txt
> new file mode 100644
> index 000000000..2e71323a2
> --- /dev/null
> +++ b/support/scripts/check-package.txt
> @@ -0,0 +1,76 @@
> +How the scripts are structured:
> +- check-package is the main engine, called by the user.
> +  For each input file, this script decides which parser should be used and it
> +  collects all functions declared in the library file. It opens the input files
> +  and it serves each raw line (including newline!) to every check function in
> +  the library. Two special parameters are used to call the initialization of
> +  each check function (for the case it needs to keep data across calls) and the
> +  equivalent finalization (e.g. for the case a message must be issued if some
> +  pattern is not in the input file).
> +- checkpackagelib.py contains common check functions.
> +  Each check function is explicitly included in a given type-parsing library.
> +  Do not include every single check function in this file, a function that will
> +  only parse hash files should be implemented in the hash-parsing library.
> +  When a warning must be issued, the check function returns an array of strings.
> +  Each string is a warning message and is displayed if the corresponding verbose
> +  level is active. When the script is called without --verbose only the first
> +  warning in the returned array is printed; when called with --verbose both
> +  first and second warnings are printed; when called with -vv until the third
> +  warning is printed; an so on.
> +  Helper functions can be defined by starting the name with "_"; they will not
> +  be called by the main script.
> +- checkpackagelib_type.py contains check functions specific to files of this
> +  type.
> +
> +Some hints when changing this code:
> +- prefer O(n) algorithms, where n is the total number of lines in the files
> +  processed.
> +- when there is no other reason for ordering, use alphabetical order (e.g. keep
> +  the check functions in alphabetical order, keep the imports in alphabetical
> +  order, and so on).
> +- use pyflakes to detect and fix potential problems.
> +- use pep8 formatting.
> +- keep in mind each function will be called with start=True before any line is
> +  served to be checked. A function that checks the filename should only
> +  implement this behavior. A function that needs to keep data across calls (e.g.
> +  keep the last line before the one being processed) should initialize all
> +  "static" variables at this stage.
> +- keep in mind each function will be called with end=True after all lines were
> +  served to be checked. A function that checks the absence of a pattern in the
> +  file will need to use this stage.
> +- try to avoid false warnings. It's better to not issue a warning message to a
> +  corner case than have too many false warnings. The second can make users stop
> +  using the script.
> +- do not check spacing in the input line in every single function. Trailing
> +  whitespace and wrong indentation should be checked by separate functions.
> +- avoid duplicate tests. Try to test only one thing in each function.
> +- in the warning message, include the url to a section from the manual, when
> +  applicable. It potentially will make more people know the manual.
> +- use short sentences in the warning messages. A complete explanation can be
> +  added to show when --verbose is used.
> +- when testing, verify the error message is displayed when the error pattern is
> +  found, but also verify the error message is not displayed for few
> +  well-formatted packages... there are many of these, just pick your favorite
> +  as golden package that should not trigger any warning message.
> +- check the url displayed by the warning message works.
> +
> +Usage examples:
> +- to get a list of check functions that would be called without actually
> +  calling them you can use the --dry-run option:
> +$ support/scripts/check-package --dry-run package/yourfavorite/*
> +
> +- when you just added a new check function, e.g. check_something, check how it
> +  behaves for all current packages:
> +$ support/scripts/check-package --include-only check_something \
> +  $(find package -type f)
> +
> +- the effective processing time (when the .pyc were already generated and all
> +  files to be processed are cached in the RAM) should stay in the order of few
> +  seconds:
> +$ support/scripts/check-package $(find package -type f) >/dev/null ; \
> +  time support/scripts/check-package $(find package -type f) >/dev/null
> +
> +- vim users can navigate the warnings (most editors probably have similar
> +  function) since warnings are generated in the form 'path/file:line: warning':
> +$ find package/ -name 'Config.*' > filelist && vim -c \
> +  'set makeprg=support/scripts/check-package\ $(cat\ filelist)' -c make -c copen
> diff --git a/support/scripts/checkpackagelib.py b/support/scripts/checkpackagelib.py
> new file mode 100644
> index 000000000..987399954
> --- /dev/null
> +++ b/support/scripts/checkpackagelib.py
> @@ -0,0 +1,18 @@
> +# See support/scripts/check-package.txt before editing this file.
> +
> +
> +def check_newline_at_eof(
> +        fname, args, lineno=0, text=None, start=False, end=False):
> +    if start:
> +        check_newline_at_eof.lastlineno = 0
> +        check_newline_at_eof.lastline = "\n"

With a class, you could just assign to self here, rather than using
function statics.

> +        return
> +    if end:
> +        line = check_newline_at_eof.lastline
> +        if line == line.rstrip("\r\n"):
> +            return ["{}:{}: missing newline at end of file"
> +                    .format(fname, check_newline_at_eof.lastlineno),
> +                    line]
> +        return
> +    check_newline_at_eof.lastlineno = lineno
> +    check_newline_at_eof.lastline = text
> diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
> new file mode 100644
> index 000000000..2660cccd7
> --- /dev/null
> +++ b/support/scripts/checkpackagelib_config.py
> @@ -0,0 +1,7 @@
> +# See support/scripts/check-package.txt before editing this file.
> +# Kconfig generates errors if someone introduces a typo like "boool" instead of
> +# "bool", so below check functions don't need to check for things already
> +# checked by running "make menuconfig".
> +
> +# Notice: ignore 'imported but unused' from pyflakes for check functions.
> +from checkpackagelib import check_newline_at_eof
> diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
> new file mode 100644
> index 000000000..590d5a7c4
> --- /dev/null
> +++ b/support/scripts/checkpackagelib_hash.py
> @@ -0,0 +1,7 @@
> +# See support/scripts/check-package.txt before editing this file.
> +# The validity of the hashes itself is checked when building, so below check
> +# functions don't need to check for things already checked by running
> +# "make package-dirclean package-source".
> +
> +# Notice: ignore 'imported but unused' from pyflakes for check functions.
> +from checkpackagelib import check_newline_at_eof
> diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
> new file mode 100644
> index 000000000..ad7362335
> --- /dev/null
> +++ b/support/scripts/checkpackagelib_mk.py
> @@ -0,0 +1,8 @@
> +# See support/scripts/check-package.txt before editing this file.
> +# There are already dependency checks during the build, so below check
> +# functions don't need to check for things already checked by exploring the
> +# menu options using "make menuconfig" and by running "make" with appropriate
> +# packages enabled.
> +
> +# Notice: ignore 'imported but unused' from pyflakes for check functions.
> +from checkpackagelib import check_newline_at_eof
> diff --git a/support/scripts/checkpackagelib_patch.py b/support/scripts/checkpackagelib_patch.py
> new file mode 100644
> index 000000000..0fb3685a7
> --- /dev/null
> +++ b/support/scripts/checkpackagelib_patch.py
> @@ -0,0 +1,7 @@
> +# See support/scripts/check-package.txt before editing this file.
> +# The format of the patch files is tested during the build, so below check
> +# functions don't need to check for things already checked by running
> +# "make package-dirclean package-patch".
> +
> +# Notice: ignore 'imported but unused' from pyflakes for check functions.
> +from checkpackagelib import check_newline_at_eof
> --
> 2.11.0

Best regards,
Thomas

Hi Ricardo,

On Tue, Jan 24, 2017 at 10:14 PM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Hi Ricardo,
>
> On Sat, Dec 31, 2016 at 4:21 AM, Ricardo Martincoski
> <ricardo.martincoski@gmail.com> wrote:
>> Create the infra to check the style of new packages before submitting.
>> The overall function of the script is described inside a txt file.
>> It is designed to process the actual files and NOT the patch files
>> generated by git format-patch.
>>
> [..]
>
> Interesting!
> Please check below where I have some comments on the implementation of
> the infrastructure.
>

I wanted to check whether you correctly received my comments.
Are you planning to send a new version of this series?

Thanks,
Thomas

Thomas,

On Mon, Feb 06, 2017 at 04:53 PM, Thomas De Schampheleire wrote:

> On Tue, Jan 24, 2017 at 10:14 PM, Thomas De Schampheleire
> <patrickdepinguin@gmail.com> wrote:

[snip]

> I wanted to check whether you correctly received my comments.
> Are you planning to send a new version of this series?

Thank you for your review.
Sorry the delay.

Yes. I plan to send v2 of the series with the corrections you suggested.
I think next week I will have time to work on this again.

In the meantime, I will mark all the scripts on the series as Change Requested.

Best regards,
Ricardo

Thomas,

On Tue, Jan 24, 2017 at 07:14 PM, Thomas De Schampheleire wrote:

[snip]
>> +CONFIG_IN_FILENAME = re.compile("/Config\.in(\.host)?$")
> 
> Beware, there are packages with different config filenames:
> 
> linux/Config.ext.in
> linux/Config.tools.in
> package/php/Config.ext
> package/qt/Config.gfx.in
> package/qt/Config.keyboard.in
> package/qt/Config.mouse.in
> package/qt/Config.sql.in

Thanks for pointing that out. I missed that.
Fixed in v2.

[snip]
>> +def call_check_function(function, fname, args, **kwargs):
> 
> What are these 'args' ? Shouldn't it be '*args' with asterisk?
> https://pythontips.com/2013/08/04/args-and-kwargs-in-python-explained/
> Same in other functions.

No, this parameter holds the command line options after processed by argparse.
In v2 I renamed it to flags and it is not used as parameters to functions.

[snip]
>> +        if args.verbose >= 3:
> 
> perhaps define named constants instead of magic '3' ?

Done in v2.

[snip]
>> +    callbacks = inspect.getmembers(lib, inspect.isfunction)
>> +
>> +    # Do not call helper functions.
>> +    callbacks = [cb for cb in callbacks if not cb[0].startswith("_")]
>> +
>> +    if args.include_list:
>> +        callbacks = [cb for cb in callbacks if cb[0] in args.include_list]
>> +    if args.exclude_list:
>> +        callbacks = [cb for cb in callbacks if cb[0] not in args.exclude_list]
> 
> It looks cleaner to me to create a custom function with all the checks
> (no helper functions, include list, exclude list) and pass that to
> inspect.getmembers, rather than doing it in four steps. The custom
> function should then return True only if the function should be kept,
> and False otherwise. See
> https://docs.python.org/2/library/inspect.html#inspect.getmembers

Fixed in v2.

Please notice I used a bit of hack to make this happen: flags, previously called
args, is now a global variable. I am still unsure if it should be _flags.
Another way to accomplish the same would be dynamically define the custom
predicate function in the main function and pass it as parameter. I think it
would be hard to understand.
If you or somebody else knows a better way, please let me know and I can respin
the series or send a followup patch.

[snip]
> 
> I find the concept with start and end parameter odd and unpythonic.
> Why do you not let each checker be a class, rather than a function?
> Each class can then define a start, check and end method (or other
> names), which you call above.

Thanks. Much better now in v2.
I also:
- rename the methods to before(), check_line() and after();
- create a base class;
- remove the useless "check_" prefix from the names;
- follow CapWords convention since now they are classes;

[snip]
>> +        check_newline_at_eof.lastline = "\n"
> 
> With a class, you could just assign to self here, rather than using
> function statics.

Done.

[snip]

Best regards,
Ricardo