All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style
@ 2016-12-31  3:21 Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 1/9] support/scripts/check-package: example Ricardo Martincoski
                   ` (10 more replies)
  0 siblings, 11 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

This patch series is only a prototype.
Please feel free to reject the series if it is too far from what you want.

I started this patch series out of curiosity.
Later on, I saw that something similar was in the todo list [1].

It probably could be achieved using flex + bison, or pyacc, or perl, or
something else.
But I implemented using python and non-strict checks, trying to:
- reduce the chance of problems related to differences between host machines;
- allow incremental development based on feedback from patch reviews;
- keep the maintenance easy;
- generate low number of false warnings.

The first patch is an example package to be used to test the output of the
script for all patches in the series.

The second patch includes a txt file with an overall description and some hints,
the main engine that calls check functions for each type of file, the first
check function, and also many comments in the library files that reflect what I
attempted to accomplish. Of course, the comments can be cut if you disagree with
any of them.

Remaining patches include check functions for each type of file. I put the
patches that I think are most likely to be rejected in the end.

Also in the series there is a blob for the manual trying to clarify the expected
format for help text.

I did not include yet:
- entry to DEVELOPERS (just because it is the first iteration);
- explicit license entry (I guess this way the script inherits the Buildroot
  license).

For each patch I include using 'git notes':
- the time it takes to run for all current packages;
- for each check function:
  - the number of warnings it generates for all current packages;
  - sample output running on the (bad) example package.

Here is the list of check functions implemented in this series:

*.patch:
- check_apply_order
- check_newline_at_eof
- check_numbered_subject
- check_sob

Config.in.*:
- check_attributes_order
- check_consecutive_empty_lines
- check_empty_last_line
- check_help_text
- check_indent
- check_newline_at_eof
- check_trailing_space

*.hash:
- check_consecutive_empty_lines
- check_empty_last_line
- check_hash_filename
- check_hash_number_of_fields
- check_hash_type
- check_newline_at_eof
- check_trailing_space

*.mk:
- check_consecutive_empty_lines
- check_empty_last_line
- check_indent
- check_newline_at_eof
- check_package_header
- check_space_before_backslash
- check_trailing_backslash
- check_trailing_space
- check_typo_in_package_variable
- check_useless_flag

[1] http://elinux.org/Buildroot#Todo_list

Regards,
Ricardo


Ricardo Martincoski (9):
  support/scripts/check-package: example
  support/scripts/check-package: new script
  check-package: check whitespace and empty lines
  check-package: check *.hash files
  check-package: check *.patch files
  check-package: check *.mk files
  docs/manual: size of tab in package description
  check-package: check Config.in.* files
  check-package: check *.mk for typo in variable

 docs/manual/adding-packages-directory.txt          |   8 +-
 docs/manual/writing-rules.txt                      |   6 +-
 support/scripts/check-package                      | 136 ++++++++++++
 .../package/package1/0001-do-something.patch       |  24 +++
 .../package/package1/0002-do-something-else.patch  |  24 +++
 .../package/package1/Config.in                     |  42 ++++
 .../package/package1/package1.hash                 |   8 +
 .../package/package1/package1.mk                   |  48 +++++
 .../package/package1/wrong-name.patch              |  13 ++
 support/scripts/check-package.txt                  |  76 +++++++
 support/scripts/checkpackagelib.py                 |  56 +++++
 support/scripts/checkpackagelib_config.py          | 136 ++++++++++++
 support/scripts/checkpackagelib_hash.py            |  76 +++++++
 support/scripts/checkpackagelib_mk.py              | 229 +++++++++++++++++++++
 support/scripts/checkpackagelib_patch.py           |  50 +++++
 15 files changed, 927 insertions(+), 5 deletions(-)
 create mode 100755 support/scripts/check-package
 create mode 100644 support/scripts/check-package-example/package/package1/0001-do-something.patch
 create mode 100644 support/scripts/check-package-example/package/package1/0002-do-something-else.patch
 create mode 100644 support/scripts/check-package-example/package/package1/Config.in
 create mode 100644 support/scripts/check-package-example/package/package1/package1.hash
 create mode 100644 support/scripts/check-package-example/package/package1/package1.mk
 create mode 100644 support/scripts/check-package-example/package/package1/wrong-name.patch
 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

-- 
2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 1/9] support/scripts/check-package: example
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 2/9] support/scripts/check-package: new script Ricardo Martincoski
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Create an example package with many style problems.

These files can be used to test the check-package script.
Ideally each new warning added to the check script will have a bad style
example in these files.

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

Notes:
    There are also bad styles inside these files that currently do not
    trigger a warning message from the script in this patch series.
    They provide an insight of what can be tested in the future.

 .../package/package1/0001-do-something.patch       | 24 +++++++++++
 .../package/package1/0002-do-something-else.patch  | 24 +++++++++++
 .../package/package1/Config.in                     | 42 +++++++++++++++++++
 .../package/package1/package1.hash                 |  8 ++++
 .../package/package1/package1.mk                   | 48 ++++++++++++++++++++++
 .../package/package1/wrong-name.patch              | 13 ++++++
 6 files changed, 159 insertions(+)
 create mode 100644 support/scripts/check-package-example/package/package1/0001-do-something.patch
 create mode 100644 support/scripts/check-package-example/package/package1/0002-do-something-else.patch
 create mode 100644 support/scripts/check-package-example/package/package1/Config.in
 create mode 100644 support/scripts/check-package-example/package/package1/package1.hash
 create mode 100644 support/scripts/check-package-example/package/package1/package1.mk
 create mode 100644 support/scripts/check-package-example/package/package1/wrong-name.patch

diff --git a/support/scripts/check-package-example/package/package1/0001-do-something.patch b/support/scripts/check-package-example/package/package1/0001-do-something.patch
new file mode 100644
index 000000000..bcd01dc1a
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/0001-do-something.patch
@@ -0,0 +1,24 @@
+From 79752a7ce44e60e276fd22031f88c796eeebf69b Mon Sep 17 00:00:00 2001
+From: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+Date: Mon, 5 Dec 2016 23:03:16 -0200
+Subject: [PATCH 25/39] do something
+
+ Signed-off-bye: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+---
+ file | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/file b/file
+index 8a1218a..8ef9013 100644
+--- a/file
++++ b/file
+@@ -1,5 +1,5 @@
+ 1
+ 2
+-3
++33
+ 4
+ 5
+-- 
+2.11.0
+
diff --git a/support/scripts/check-package-example/package/package1/0002-do-something-else.patch b/support/scripts/check-package-example/package/package1/0002-do-something-else.patch
new file mode 100644
index 000000000..d95ec4876
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/0002-do-something-else.patch
@@ -0,0 +1,24 @@
+From 79752a7ce44e60e276fd22031f88c796eeebf69b Mon Sep 17 00:00:00 2001
+From: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+Date: Mon, 5 Dec 2016 23:03:16 -0200
+Subject: [PATCH] do something else
+
+Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+---
+ file | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/file b/file
+index 8a1218a..8ef9013 100644
+--- a/file
++++ b/file
+@@ -1,5 +1,5 @@
+ 1
+ 2
+-3
++33
+ 4
+ 5
+-- 
+2.11.0
+
diff --git a/support/scripts/check-package-example/package/package1/Config.in b/support/scripts/check-package-example/package/package1/Config.in
new file mode 100644
index 000000000..8cc47390f
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/Config.in
@@ -0,0 +1,42 @@
+config BR2_PACKAGE_PACKAGE1
+	select BR2_PACKAGE_LIBEVENT
+	bool "pAcKaGe"
+	depends on BR2_USE_MMU
+	select BR2_PACKAGE_NCURSES
+	depends on BR2_USE_WCHAR
+	help
+	  package1 is a bad stylized package. Its only purpose is to exemplify
+          common style mistakes
+	some more help text but no url
+
+if BR2_PACKAGE_PACKAGE1
+ config BR2_PACKAGE_PACKAGE1_OPTION
+        bool "package1 option"
+        depends on BR2_USE_MMU
+        select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
+        help
+	  This paragraph is properly wrapped. Since a tab counts as 8
+	  spaces and the help text must be wrapped at column 72, only 62
+	  characters for the text itself are expect per line.
+
+
+	  Another paragraph. - But this time we cross the column 72 by 1.
+	 wrong_line_with_single_word
+	  http://www.example.com/ urls do not have spaces and this line is too long.
+
+	  http://www.example.com/folder/even_long_url_should_not_be_wrapped
+
+config BR2_PACKAGE_PACKAGE1_OPTION2
+	string "option2"
+	default "aarch64-unknown-linux-gnu" \
+		if BR2_aarch64 || BR2_aarch64_eb
+
+config BR2_PACKAGE_PACKAGE1_OPTION3
+	string "option4"
+	default "value" \
+                if BR2_aarch64
+endif
+
+comment "package1 needs a toolchain w/ locale"
+        depends on BR2_USE_MMU
+	depends on BR2_USE_WCHAR
diff --git a/support/scripts/check-package-example/package/package1/package1.hash b/support/scripts/check-package-example/package/package1/package1.hash
new file mode 100644
index 000000000..78559bd4a
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/package1.hash
@@ -0,0 +1,8 @@
+# some comment:
+sha256 1234567890123456789012345678901234567890123456789012345678901234 package1-1.0.tar.gz 
+sha256 123456789 package1-1.0.tar.gz
+crc16	123456789	package1-1.0.tar.gz
+sha256	12345678901234567890123456789012345678901234567890123456789012345	dl/package1-1.0.tar.gz
+sha256 1234567890123456789012345678901234567890123456789012345678901234
+  
+
diff --git a/support/scripts/check-package-example/package/package1/package1.mk b/support/scripts/check-package-example/package/package1/package1.mk
new file mode 100644
index 000000000..bbeb34b69
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/package1.mk
@@ -0,0 +1,48 @@
+########################################
+#
+# wrong name
+#
+########################################################################################################################
+PACKAGE1_VERSION=1.0
+PACKAGE1_SITE = https://localhost        
+PACKAGE1_LICENSE = GPL
+PACKAGE1_LICENSE_FILE = README
+PACKAGE1_LICENSE_FILES += COPYING
+# now some unneeded flags because they are the default value
+PACKAGE1_INSTALL_STAGING=NO 
+PACKAGE1_INSTALL_TARGET = YES	
+PACKAGE1_INSTALL_IMAGES  =  NO
+ PACKAGE1_INSTALL_REDISTRIBUTE = YES
+PACKAGE1_AUTORECONF = NO
+PACKAGE1_LIBTOOL_PATCH	=	YES
+ # but non-default conditionally overridden by default is allowed
+ifeq ($(BR2_STATIC_LIBS),y)
+	PACKAGE1_INSTALL_STAGING = NO
+endif
+
+
+PACKAGE1_DEPENDENCIES = depend1 depend2  \
+                       depend3
+PACKAGE1_DEPENDENCIES += depend5	\
+	depend4 \
+
+PACKAGE1_DEPENDENCIES = overwriting
+PACKAGE1_DEEEEEEEEEES = typo
+LINUX_DEPENDENCIES = messing with others
+PACKACE1_DEPENDENCIES = typo
+
+define PACKAGE1_INSTALL_SOMETHING
+        mkdir -p $(TARGET_DIR)/var/lib
+	$(INSTALL) -m 0755 -D file1 \
+		$(TARGET_DIR)/var/lib/file
+	$(INSTALL) -m 0755 -D file2 \
+	$(TARGET_DIR)/etc/file
+endef
+
+define PACKAGE1_INSTALL_TARGET_CMDS
+	$(PACKAGE1_INSTALL_SOMETHING)
+        $(PACKAGE1_INSTALL_SOMETHING_ELSE)
+endef
+
+$(eval $(autotools-package))
+	
diff --git a/support/scripts/check-package-example/package/package1/wrong-name.patch b/support/scripts/check-package-example/package/package1/wrong-name.patch
new file mode 100644
index 000000000..d4455338e
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/wrong-name.patch
@@ -0,0 +1,13 @@
+no sob
+---
+diff -purN package.orig/file package/file
+--- package.orig/file	2000-01-01 22:07:43.275324499 -0300
++++ package/file	2016-01-01 22:08:16.171453283 -0300
+@@ -268,7 +268,6 @@ line
+ #include <errno.h>
+ #include <fcntl.h>
+ #include <string.h>
+-#include <termio.h>
+ #include <unistd.h>
+ #include <stdarg.h>
+ #include <curses.h>
\ No newline at end of file
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 2/9] support/scripts/check-package: new script
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 1/9] support/scripts/check-package: example Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2017-01-24 21:14   ` Thomas De Schampheleire
  2016-12-31  3:21 ` [Buildroot] [PATCH 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
                   ` (8 subsequent siblings)
  10 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

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

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)?$")
+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):
+    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:
+            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]
+
+    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)
+
+    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"
+        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

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 3/9] check-package: check whitespace and empty lines
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 1/9] support/scripts/check-package: example Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 2/9] support/scripts/check-package: new script Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 4/9] check-package: check *.hash files Ricardo Martincoski
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Create 3 new check functions to warn when:
- there are consecutive empty lines in the file, see [1];
- the last line of the file is empty, see [2];
- there are lines with trailing whitespace, see [3].

Apply these functions to Config.in, .mk and .hash, but not for .patch
files since they can contain any of these and still be valid.

[1] http://patchwork.ozlabs.org/patch/682660/
[2] http://patchwork.ozlabs.org/patch/643288/
[3] http://patchwork.ozlabs.org/patch/398984/

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.787s
    user	0m0.772s
    sys	0m0.012s
    
    CHECK_CONSECUTIVE_EMPTY_LINES:
     support/scripts/check-package --include-only check_consecutive_empty_lines \
     $(find package -type f) 2>/dev/null | wc -l
      22
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_consecutive_empty_lines -vv package/*/*)
      package/package1/Config.in:22: consecutive empty lines
      package/package1/package1.hash:8: consecutive empty lines
      package/package1/package1.mk:23: consecutive empty lines
      159 lines processed
      3 warnings generated
    
    CHECK_EMPTY_LAST_LINE:
     support/scripts/check-package --include-only check_empty_last_line \
     $(find package -type f) 2>/dev/null | wc -l
      17
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_empty_last_line -vv package/*/*)
      package/package1/package1.hash:8: empty line at end of file
      package/package1/package1.mk:48: empty line at end of file
      159 lines processed
      2 warnings generated
    
    CHECK_TRAILING_SPACE:
     support/scripts/check-package --include-only check_trailing_space \
     $(find package -type f) 2>/dev/null | wc -l
      3
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_trailing_space -vv package/*/*)
      package/package1/package1.hash:2: line contains trailing whitespace
      sha256 1234567890123456789012345678901234567890123456789012345678901234 package1-1.0.tar.gz
      package/package1/package1.hash:7: line contains trailing whitespace
    
      package/package1/package1.mk:7: line contains trailing whitespace
      PACKAGE1_SITE = https://localhost
      package/package1/package1.mk:12: line contains trailing whitespace
      PACKAGE1_INSTALL_STAGING=NO
      package/package1/package1.mk:13: line contains trailing whitespace
      PACKAGE1_INSTALL_TARGET = YES< tab  >
      package/package1/package1.mk:48: line contains trailing whitespace
      < tab  >
      159 lines processed
      6 warnings generated

 support/scripts/checkpackagelib.py        | 38 +++++++++++++++++++++++++++++++
 support/scripts/checkpackagelib_config.py |  3 +++
 support/scripts/checkpackagelib_hash.py   |  3 +++
 support/scripts/checkpackagelib_mk.py     |  3 +++
 4 files changed, 47 insertions(+)

diff --git a/support/scripts/checkpackagelib.py b/support/scripts/checkpackagelib.py
index 987399954..787b22551 100644
--- a/support/scripts/checkpackagelib.py
+++ b/support/scripts/checkpackagelib.py
@@ -1,6 +1,33 @@
 # See support/scripts/check-package.txt before editing this file.
 
 
+def check_consecutive_empty_lines(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_consecutive_empty_lines.lastline = "non empty"
+        return
+    if end:
+        return
+    if text.strip() == "" == check_consecutive_empty_lines.lastline.strip():
+        return ["{}:{}: consecutive empty lines".format(fname, lineno)]
+    check_consecutive_empty_lines.lastline = text
+
+
+def check_empty_last_line(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_empty_last_line.lastlineno = 0
+        check_empty_last_line.lastline = "non empty"
+        return
+    if end:
+        if check_empty_last_line.lastline.strip() == "":
+            return ["{}:{}: empty line at end of file"
+                    .format(fname, check_empty_last_line.lastlineno)]
+        return
+    check_empty_last_line.lastlineno = lineno
+    check_empty_last_line.lastline = text
+
+
 def check_newline_at_eof(
         fname, args, lineno=0, text=None, start=False, end=False):
     if start:
@@ -16,3 +43,14 @@ def check_newline_at_eof(
         return
     check_newline_at_eof.lastlineno = lineno
     check_newline_at_eof.lastline = text
+
+
+def check_trailing_space(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end:
+        return
+    line = text.rstrip("\r\n")
+    if line != line.rstrip():
+        return ["{}:{}: line contains trailing whitespace"
+                .format(fname, lineno),
+                text]
diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
index 2660cccd7..aec8b3ea4 100644
--- a/support/scripts/checkpackagelib_config.py
+++ b/support/scripts/checkpackagelib_config.py
@@ -4,4 +4,7 @@
 # checked by running "make menuconfig".
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import check_consecutive_empty_lines
+from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
+from checkpackagelib import check_trailing_space
diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
index 590d5a7c4..d095f9255 100644
--- a/support/scripts/checkpackagelib_hash.py
+++ b/support/scripts/checkpackagelib_hash.py
@@ -4,4 +4,7 @@
 # "make package-dirclean package-source".
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import check_consecutive_empty_lines
+from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
+from checkpackagelib import check_trailing_space
diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index ad7362335..1ff0973dd 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -5,4 +5,7 @@
 # packages enabled.
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import check_consecutive_empty_lines
+from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
+from checkpackagelib import check_trailing_space
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 4/9] check-package: check *.hash files
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (2 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2017-01-24 21:18   ` Thomas De Schampheleire
  2016-12-31  3:21 ` [Buildroot] [PATCH 5/9] check-package: check *.patch files Ricardo Martincoski
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Check each hash entry (see [1]) and warn when:
- it does not have three fields;
- its type is unknown;
- its length does not match its type;
- the name of the file contains a directory component.

[1] http://nightly.buildroot.org/#adding-packages-hash

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.837s
    user	0m0.780s
    sys	0m0.056s
    
    CHECK_HASH_FILENAME:
     support/scripts/check-package --include-only check_hash_filename \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_hash_filename -vv package/*/*)
      package/package1/package1.hash:5: use filename without directory component (http://nightly.buildroot.org/#adding-packages-hash)
      sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
      159 lines processed
      1 warnings generated
    
    CHECK_HASH_NUMBER_OF_FIELDS:
     support/scripts/check-package --include-only check_hash_number_of_fields \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_hash_number_of_fields -vv package/*/*)
      package/package1/package1.hash:6: expected three fields (http://nightly.buildroot.org/#adding-packages-hash)
      sha256 1234567890123456789012345678901234567890123456789012345678901234
      159 lines processed
      1 warnings generated
    
    CHECK_HASH_TYPE:
     support/scripts/check-package --include-only check_hash_type \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      1
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_hash_type -vv package/*/*)
      package/package1/package1.hash:3: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
      sha256 123456789 package1-1.0.tar.gz
      expected 64 hex digits
      package/package1/package1.hash:4: unexpected type of hash (http://nightly.buildroot.org/#adding-packages-hash)
      crc16< tab  >123456789< tab  >package1-1.0.tar.gz
      package/package1/package1.hash:5: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
      sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
      expected 64 hex digits
      159 lines processed
      3 warnings generated

 support/scripts/checkpackagelib_hash.py | 66 +++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
index d095f9255..646c6cc12 100644
--- a/support/scripts/checkpackagelib_hash.py
+++ b/support/scripts/checkpackagelib_hash.py
@@ -3,8 +3,74 @@
 # functions don't need to check for things already checked by running
 # "make package-dirclean package-source".
 
+import re
+
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import check_consecutive_empty_lines
 from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
 from checkpackagelib import check_trailing_space
+
+
+def _empty_line_or_comment(text):
+    return text.strip() == "" or text.startswith("#")
+
+
+FILENAME_WITH_SLASH = re.compile("/")
+
+
+def check_hash_filename(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end or _empty_line_or_comment(text):
+        return
+
+    fields = text.split()
+    if len(fields) < 3:
+        return
+
+    filename = fields[2]
+    if FILENAME_WITH_SLASH.search(filename):
+        return ["{}:{}: use filename without directory component"
+                " ({}#adding-packages-hash)"
+                .format(fname, lineno, args.manual_url),
+                text]
+
+
+def check_hash_number_of_fields(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end or _empty_line_or_comment(text):
+        return
+
+    fields = text.split()
+    if len(fields) != 3:
+        return ["{}:{}: expected three fields ({}#adding-packages-hash)"
+                .format(fname, lineno, args.manual_url),
+                text]
+
+
+len_of_hash = {"md5": 32, "sha1": 40, "sha224": 56, "sha256": 64, "sha384": 96,
+               "sha512": 128}
+
+
+def check_hash_type(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end or _empty_line_or_comment(text):
+        return
+
+    fields = text.split()
+    if len(fields) < 2:
+        return
+
+    hash_type, hexa = fields[:2]
+    if hash_type == "none":
+        return
+    if hash_type not in len_of_hash.keys():
+        return ["{}:{}: unexpected type of hash ({}#adding-packages-hash)"
+                .format(fname, lineno, args.manual_url),
+                text]
+    if not re.match("^[0-9A-Fa-f]{%s}$" % len_of_hash[hash_type], hexa):
+        return ["{}:{}: hash size does not match type "
+                "({}#adding-packages-hash)"
+                .format(fname, lineno, args.manual_url),
+                text,
+                "expected {} hex digits".format(len_of_hash[hash_type])]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 5/9] check-package: check *.patch files
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (3 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 4/9] check-package: check *.hash files Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2017-01-24 21:21   ` Thomas De Schampheleire
  2016-12-31  3:21 ` [Buildroot] [PATCH 6/9] check-package: check *.mk files Ricardo Martincoski
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Warn when the name of the patch file does not start with number (apply
order), see [1].
Warn when the patch was generated using git format-patch without -N, see
[2].
Warn when the patch file has no SoB, see [3].

[1] http://nightly.buildroot.org/#_providing_patches
[2] http://patchwork.ozlabs.org/patch/704753/
[3] http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches

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	0m1.153s
    user	0m1.096s
    sys	0m0.056s
    
    CHECK_APPLY_ORDER:
     support/scripts/check-package --include-only check_apply_order \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      4
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_apply_order -vv package/*/*)
      package/package1/wrong-name.patch:0: use name <number>-<description>.patch (http://nightly.buildroot.org/#_providing_patches)
      159 lines processed
      1 warnings generated
    
    CHECK_NUMBERED_SUBJECT:
     support/scripts/check-package --include-only check_numbered_subject \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      149
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_numbered_subject -vv package/*/*)
      package/package1/0001-do-something.patch:4: generate your patches with 'git format-patch -N'
      Subject: [PATCH 25/39] do something
      159 lines processed
      1 warnings generated
    
    CHECK_SOB:
     support/scripts/check-package --include-only check_sob \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      143
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_sob -vv package/*/*)
      package/package1/0001-do-something.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
      package/package1/wrong-name.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
      159 lines processed
      2 warnings generated

 support/scripts/checkpackagelib_patch.py | 43 ++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/support/scripts/checkpackagelib_patch.py b/support/scripts/checkpackagelib_patch.py
index 0fb3685a7..f10dc31c4 100644
--- a/support/scripts/checkpackagelib_patch.py
+++ b/support/scripts/checkpackagelib_patch.py
@@ -3,5 +3,48 @@
 # functions don't need to check for things already checked by running
 # "make package-dirclean package-patch".
 
+import re
+
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import check_newline_at_eof
+
+
+APPLY_ORDER = re.compile("/\d{1,4}-[^/]*$")
+
+
+def check_apply_order(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start and not APPLY_ORDER.search(fname):
+        return ["{}:0: use name <number>-<description>.patch "
+                "({}#_providing_patches)".format(fname, args.manual_url)]
+
+
+NUMBERED_PATCH = re.compile("Subject:\s*\[PATCH\s*\d+/\d+\]")
+
+
+def check_numbered_subject(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end:
+        return
+    if NUMBERED_PATCH.search(text):
+        return ["{}:{}: generate your patches with 'git format-patch -N'"
+                .format(fname, lineno),
+                text]
+
+
+SOB_ENTRY = re.compile("^Signed-off-by: .*$")
+
+
+def check_sob(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_sob.found = False
+        return
+    if check_sob.found:
+        return
+    if end:
+        return ["{}:0: missing Signed-off-by in the header "
+                "({}#_format_and_licensing_of_the_package_patches)"
+                .format(fname, args.manual_url)]
+    if SOB_ENTRY.search(text):
+        check_sob.found = True
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 6/9] check-package: check *.mk files
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (4 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 5/9] check-package: check *.patch files Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description Ricardo Martincoski
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Warn when there are obvious indentation errors:
- the number of expect tabs is not yet checked since it is more complex
  to achieve;
- the content inside define ... endef should be indented with tab(s),
  see [1];
- line just after a backslash should be indented with tab(s), see [2];
- other lines should not be indented, see [3];
- ignore empty lines and comments.
Warn when there is no well-formatted header in the file:
- 80 hashes at lines 1 and 5;
- 1 hash at lines 2 and 4;
- empty line at line 6;
- see [4];
- ignore files that only include other mk files.
Warn when there are more than one space before backslash, see [5].
Warn when there is a trailing backslash [6].
Warn for flags set to default value YES or NO, see [7], [8], [9].

[1] http://patchwork.ozlabs.org/patch/681429/
[2] http://patchwork.ozlabs.org/patch/681430/
[3] http://patchwork.ozlabs.org/patch/559209/
[4] http://nightly.buildroot.org/#writing-rules-mk
[5] http://patchwork.ozlabs.org/patch/649084/
[6] http://patchwork.ozlabs.org/patch/535550/
[7] http://patchwork.ozlabs.org/patch/704718/
[8] http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems
[9] http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages

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	0m2.125s
    user	0m2.084s
    sys	0m0.040s
    
    CHECK_INDENT:
     support/scripts/check-package --include-only check_indent \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      23
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_indent -vv package/*/*)
      package/package1/package1.mk:20: unexpected indent with tabs
      < tab  >PACKAGE1_INSTALL_STAGING = NO
      package/package1/package1.mk:25: expected indent with tabs
                             depend3
      package/package1/package1.mk:35: expected indent with tabs
              mkdir -p $(TARGET_DIR)/var/lib
      package/package1/package1.mk:44: expected indent with tabs
              $(PACKAGE1_INSTALL_SOMETHING_ELSE)
      159 lines processed
      4 warnings generated
    
    CHECK_PACKAGE_HEADER:
     support/scripts/check-package --include-only check_package_header \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      20
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_package_header -vv package/*/*)
      package/package1/package1.mk:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)
      ########################################
      ################################################################################
      package/package1/package1.mk:5: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)
      ########################################################################################################################
      ################################################################################
      package/package1/package1.mk:6: should be a blank line (http://nightly.buildroot.org/#writing-rules-mk)
      PACKAGE1_VERSION=1.0
      159 lines processed
      3 warnings generated
    
    CHECK_SPACE_BEFORE_BACKSLASH:
     support/scripts/check-package --include-only check_space_before_backslash \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      342
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_space_before_backslash -vv package/*/*)
      package/package1/package1.mk:24: use only one space before backslash
      PACKAGE1_DEPENDENCIES = depend1 depend2  \
      package/package1/package1.mk:26: use only one space before backslash
      PACKAGE1_DEPENDENCIES += depend5< tab  >\
      159 lines processed
      2 warnings generated
    
    CHECK_TRAILING_BACKSLASH:
     support/scripts/check-package --include-only check_trailing_backslash \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      21
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_trailing_backslash -vv package/*/*)
      package/package1/package1.mk:27: remove trailing backslash
      < tab  >depend4 \
      159 lines processed
      1 warnings generated
    
    CHECK_USELESS_FLAG:
     support/scripts/check-package --include-only check_useless_flag \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_useless_flag -vv package/*/*)
      package/package1/package1.mk:12: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_STAGING=NO
      package/package1/package1.mk:13: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_TARGET = YES< tab  >
      package/package1/package1.mk:14: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_IMAGES  =  NO
      package/package1/package1.mk:15: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
       PACKAGE1_INSTALL_REDISTRIBUTE = YES
      package/package1/package1.mk:16: useless default value (http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages)
      PACKAGE1_AUTORECONF = NO
      package/package1/package1.mk:17: useless default value (http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages)
      PACKAGE1_LIBTOOL_PATCH< tab  >=< tab  >YES
      159 lines processed
      6 warnings generated

 support/scripts/checkpackagelib_mk.py | 174 ++++++++++++++++++++++++++++++++++
 1 file changed, 174 insertions(+)

diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index 1ff0973dd..40574584a 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -4,8 +4,182 @@
 # menu options using "make menuconfig" and by running "make" with appropriate
 # packages enabled.
 
+import re
+
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import check_consecutive_empty_lines
 from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
 from checkpackagelib import check_trailing_space
+
+# used by more than one function
+ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
+
+COMMENT = re.compile("^\s*#")
+CONDITIONAL = re.compile("^\s*(ifeq|ifneq|endif)\s")
+END_DEFINE = re.compile("^\s*endef\s")
+MAKEFILE_TARGET = re.compile("^[^# \t]+:\s")
+START_DEFINE = re.compile("^\s*define\s")
+
+
+def check_indent(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_indent.define = False
+        check_indent.backslash = False
+        check_indent.makefile_target = False
+        return
+    if end:
+        return
+
+    if START_DEFINE.search(text):
+        check_indent.define = True
+        return
+    if END_DEFINE.search(text):
+        check_indent.define = False
+        return
+
+    expect_tabs = False
+    if (check_indent.define or check_indent.backslash or
+            check_indent.makefile_target):
+        expect_tabs = True
+    if CONDITIONAL.search(text):
+        expect_tabs = False
+
+    # calculate for next line
+    if ENDS_WITH_BACKSLASH.search(text):
+        check_indent.backslash = True
+    else:
+        check_indent.backslash = False
+
+    if MAKEFILE_TARGET.search(text):
+        check_indent.makefile_target = True
+        return
+    if text.strip() == "":
+        check_indent.makefile_target = False
+        return
+
+    # comment can be indented or not inside define ... endef, so ignore it
+    if check_indent.define and COMMENT.search(text):
+        return
+
+    if expect_tabs:
+        if not text.startswith("\t"):
+            return ["{}:{}: expected indent with tabs"
+                    .format(fname, lineno),
+                    text]
+    else:
+        if text.startswith("\t"):
+            return ["{}:{}: unexpected indent with tabs"
+                    .format(fname, lineno),
+                    text]
+
+
+def check_package_header(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_package_header.skip = False
+        return
+    if end or check_package_header.skip or lineno > 6:
+        return
+
+    if lineno in [1, 5]:
+        if lineno == 1 and text.startswith("include "):
+            check_package_header.skip = True
+            return
+        if text.rstrip() != "#" * 80:
+            return ["{}:{}: should be 80 hashes ({}#writing-rules-mk)"
+                    .format(fname, lineno, args.manual_url),
+                    text,
+                    "#" * 80]
+    elif lineno in [2, 4]:
+        if text.rstrip() != "#":
+            return ["{}:{}: should be 1 hash ({}#writing-rules-mk)"
+                    .format(fname, lineno, args.manual_url),
+                    text]
+    elif lineno == 6:
+        if text.rstrip() != "":
+            return ["{}:{}: should be a blank line ({}#writing-rules-mk)"
+                    .format(fname, lineno, args.manual_url),
+                    text]
+
+
+TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*(  |\t)\\$")
+
+
+def check_space_before_backslash(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end:
+        return
+
+    if TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH.match(text.rstrip()):
+        return ["{}:{}: use only one space before backslash"
+                .format(fname, lineno),
+                text]
+
+
+def check_trailing_backslash(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_trailing_backslash.backslash = False
+        return
+    if end:
+        return
+
+    last_line_ends_in_backslash = check_trailing_backslash.backslash
+
+    # calculate for next line
+    if ENDS_WITH_BACKSLASH.search(text):
+        check_trailing_backslash.backslash = True
+        check_trailing_backslash.lastline = text
+        return
+    check_trailing_backslash.backslash = False
+
+    if last_line_ends_in_backslash and text.strip() == "":
+        return ["{}:{}: remove trailing backslash"
+                .format(fname, lineno - 1),
+                check_trailing_backslash.lastline]
+
+
+DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
+    "_AUTORECONF\s*=\s*NO",
+    "_LIBTOOL_PATCH\s*=\s*YES"])))
+DEFAULT_GENERIC_FLAG = re.compile("^.*{}".format("|".join([
+    "_INSTALL_IMAGES\s*=\s*NO",
+    "_INSTALL_REDISTRIBUTE\s*=\s*YES",
+    "_INSTALL_STAGING\s*=\s*NO",
+    "_INSTALL_TARGET\s*=\s*YES"])))
+END_CONDITIONAL = re.compile("^\s*(endif)")
+START_CONDITIONAL = re.compile("^\s*(ifeq|ifneq)")
+
+
+def check_useless_flag(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_useless_flag.conditional = 0
+        return
+    if end:
+        return
+
+    if START_CONDITIONAL.search(text):
+        check_useless_flag.conditional += 1
+        return
+    if END_CONDITIONAL.search(text):
+        check_useless_flag.conditional -= 1
+        return
+
+    # allow non-default conditionally overridden by default
+    if check_useless_flag.conditional > 0:
+        return
+
+    if DEFAULT_GENERIC_FLAG.search(text):
+        return ["{}:{}: useless default value "
+                "({}#_infrastructure_for_packages_with_specific_build_systems)"
+                .format(fname, lineno, args.manual_url),
+                text]
+
+    if DEFAULT_AUTOTOOLS_FLAG.search(text):
+        return ["{}:{}: useless default value "
+                "({}#_infrastructure_for_autotools_based_packages)"
+                .format(fname, lineno, args.manual_url),
+                text]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (5 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 6/9] check-package: check *.mk files Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2017-01-21 16:58   ` Romain Naour
  2016-12-31  3:21 ` [Buildroot] [PATCH 8/9] check-package: check Config.in.* files Ricardo Martincoski
                   ` (3 subsequent siblings)
  10 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Explicitly state that one tab counts for 8 columns in package
description, leaving 62 characters to the text itself.
Update the text and the example in the two places where the Config.in
format is described.
Also mention a newline is expected between the help text itself and the
upstream URL.

This blob can help newcomers to understand the expected formatting.
Also, it can be referenced by reviewers.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 docs/manual/adding-packages-directory.txt | 8 +++++---
 docs/manual/writing-rules.txt             | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
index 5dba962bd..08f5d42f9 100644
--- a/docs/manual/adding-packages-directory.txt
+++ b/docs/manual/adding-packages-directory.txt
@@ -28,7 +28,8 @@ contain:
 config BR2_PACKAGE_LIBFOO
 	bool "libfoo"
 	help
-	  This is a comment that explains what libfoo is.
+	  This is a comment that explains what libfoo is. The help text
+	  should be wrapped.
 
 	  http://foosoftware.org/libfoo/
 ---------------------------
@@ -36,8 +37,9 @@ config BR2_PACKAGE_LIBFOO
 The +bool+ line, +help+ line and other metadata information about the
 configuration option must be indented with one tab. The help text
 itself should be indented with one tab and two spaces, lines should
-not be longer than 72 columns, and it must mention the upstream URL
-of the project.
+be wrapped to fit 72 columns, where tab counts for 8, so 62 characters
+in the text itself. The help text must mention the upstream URL of the
+project after an empty line.
 
 As a convention specific to Buildroot, the ordering of the attributes
 is as follows:
diff --git a/docs/manual/writing-rules.txt b/docs/manual/writing-rules.txt
index ec1ddb191..e2ad41ebc 100644
--- a/docs/manual/writing-rules.txt
+++ b/docs/manual/writing-rules.txt
@@ -29,7 +29,8 @@ config BR2_PACKAGE_LIBFOO
 	depends on BR2_PACKAGE_LIBBAZ
 	select BR2_PACKAGE_LIBBAR
 	help
-	  This is a comment that explains what libfoo is.
+	  This is a comment that explains what libfoo is. The help text
+	  should be wrapped.
 
 	  http://foosoftware.org/libfoo/
 ---------------------
@@ -40,7 +41,8 @@ config BR2_PACKAGE_LIBFOO
 * The help text itself should be indented with one tab and two
   spaces.
 
-* The help text should be wrapped to fit 72 columns.
+* The help text should be wrapped to fit 72 columns, where tab counts
+  for 8, so 62 characters in the text itself.
 
 The +Config.in+ files are the input for the configuration tool
 used in Buildroot, which is the regular _Kconfig_. For further
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 8/9] check-package: check Config.in.* files
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (6 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2016-12-31  3:21 ` [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Warn when help text is larger than 72 columns, see [1].
Warn for wrongly indented attributes, see [1].
Warn when the convention of attributes order are not followed, see [2].

[1] http://nightly.buildroot.org/#writing-rules-config-in
[2] http://nightly.buildroot.org/#_config_files

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	0m2.453s
    user	0m2.396s
    sys	0m0.056s
    
    CHECK_ATTRIBUTES_ORDER:
     support/scripts/check-package --include-only check_attributes_order \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      359
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_attributes_order -vv package/*/*)
      package/package1/Config.in:3: attributes order: type, default, depends on, select, help (http://nightly.buildroot.org/#_config_files)
      < tab  >bool "pAcKaGe"
      package/package1/Config.in:6: attributes order: type, default, depends on, select, help (http://nightly.buildroot.org/#_config_files)
      < tab  >depends on BR2_USE_WCHAR
      159 lines processed
      2 warnings generated
    
    CHECK_HELP_TEXT:
     support/scripts/check-package --include-only check_help_text \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      988
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_help_text -vv package/*/*)
      package/package1/Config.in:8: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  package1 is a bad stylized package. Its only purpose is to exemplify
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:9: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
                common style mistakes
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:10: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >some more help text but no url
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:23: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  Another paragraph. - But this time we cross the column 72 by 1.
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:24: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  > wrong_line_with_single_word
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:25: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  http://www.example.com/ urls do not have spaces and this line is too long.
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      159 lines processed
      6 warnings generated
    
    CHECK_INDENT:
     support/scripts/check-package --include-only check_indent \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      495
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_indent -vv package/*/*)
      package/package1/Config.in:13: should not be indented
       config BR2_PACKAGE_PACKAGE1_OPTION
      package/package1/Config.in:14: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              bool "package1 option"
      package/package1/Config.in:15: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              depends on BR2_USE_MMU
      package/package1/Config.in:16: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
      package/package1/Config.in:17: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              help
      package/package1/Config.in:37: continuation line should be indented using tabs
                      if BR2_aarch64
      package/package1/Config.in:41: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              depends on BR2_USE_MMU
      package/package1/package1.mk:20: unexpected indent with tabs
      < tab  >PACKAGE1_INSTALL_STAGING = NO
      package/package1/package1.mk:25: expected indent with tabs
                             depend3
      package/package1/package1.mk:35: expected indent with tabs
              mkdir -p $(TARGET_DIR)/var/lib
      package/package1/package1.mk:44: expected indent with tabs
              $(PACKAGE1_INSTALL_SOMETHING_ELSE)
      159 lines processed
      11 warnings generated

 support/scripts/checkpackagelib_config.py | 126 ++++++++++++++++++++++++++++++
 1 file changed, 126 insertions(+)

diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
index aec8b3ea4..a52f06247 100644
--- a/support/scripts/checkpackagelib_config.py
+++ b/support/scripts/checkpackagelib_config.py
@@ -3,8 +3,134 @@
 # "bool", so below check functions don't need to check for things already
 # checked by running "make menuconfig".
 
+import re
+
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import check_consecutive_empty_lines
 from checkpackagelib import check_empty_last_line
 from checkpackagelib import check_newline_at_eof
 from checkpackagelib import check_trailing_space
+
+
+def _empty_or_comment(text):
+    line = text.strip()
+    # ignore empty lines and comment lines indented or not
+    return line == "" or line.startswith("#")
+
+
+def _part_of_help_text(text):
+    return text.startswith("\t  ")
+
+
+# used in more than one function
+entries_that_should_not_be_indented = [
+    "choice", "comment", "config", "endchoice", "endif", "endmenu", "if",
+    "menu", "menuconfig", "source"]
+
+
+attributes_order_convention = {
+    "bool": 1, "prompt": 1, "string": 1, "default": 2, "depends": 3,
+    "select": 4, "help": 5}
+
+
+def check_attributes_order(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_attributes_order.state = 0
+        return
+    if end or _empty_or_comment(text) or _part_of_help_text(text):
+        return
+
+    attribute = text.split()[0]
+
+    if attribute in entries_that_should_not_be_indented:
+        check_attributes_order.state = 0
+        return
+    if attribute not in attributes_order_convention.keys():
+        return
+    new_state = attributes_order_convention[attribute]
+    wrong_order = check_attributes_order.state > new_state
+
+    # save to process next line
+    check_attributes_order.state = new_state
+
+    if wrong_order:
+        return ["{}:{}: attributes order: type, default, depends on,"
+                " select, help ({}#_config_files)"
+                .format(fname, lineno, args.manual_url),
+                text]
+
+
+HELP_TEXT_FORMAT = re.compile("^\t  .{,62}$")
+URL_ONLY = re.compile("^(http|https|git)://\S*$")
+
+
+def check_help_text(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        check_help_text.help_text = False
+        return
+    if end or _empty_or_comment(text):
+        return
+
+    entry = text.split()[0]
+
+    if entry in entries_that_should_not_be_indented:
+        check_help_text.help_text = False
+        return
+    if text.strip() == "help":
+        check_help_text.help_text = True
+        return
+
+    if not check_help_text.help_text:
+        return
+
+    if HELP_TEXT_FORMAT.match(text.rstrip()):
+        return
+    if URL_ONLY.match(text.strip()):
+        return
+    return ["{}:{}: help text: <tab><2 spaces><62 chars>"
+            " ({}#writing-rules-config-in)"
+            .format(fname, lineno, args.manual_url),
+            text,
+            "\t  " + "123456789 " * 6 + "12"]
+
+
+ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
+entries_that_should_be_indented = [
+    "bool", "default", "depends", "help", "prompt", "select", "string"]
+
+
+def check_indent(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start or end or _empty_or_comment(text) or _part_of_help_text(text):
+        check_indent.backslash = False
+        return
+
+    entry = text.split()[0]
+
+    last_line_ends_in_backslash = check_indent.backslash
+
+    # calculate for next line
+    if ENDS_WITH_BACKSLASH.search(text):
+        check_indent.backslash = True
+    else:
+        check_indent.backslash = False
+
+    if last_line_ends_in_backslash:
+        if text.startswith("\t"):
+            return
+        else:
+            return ["{}:{}: continuation line should be indented using tabs"
+                    .format(fname, lineno),
+                    text]
+
+    if entry in entries_that_should_be_indented:
+        if not text.startswith("\t{}".format(entry)):
+            return ["{}:{}: should be indented with one tab ({}#_config_files)"
+                    .format(fname, lineno, args.manual_url),
+                    text]
+    elif entry in entries_that_should_not_be_indented:
+        if not text.startswith(entry):
+            return ["{}:{}: should not be indented".format(fname, lineno),
+                    text]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (7 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 8/9] check-package: check Config.in.* files Ricardo Martincoski
@ 2016-12-31  3:21 ` Ricardo Martincoski
  2017-01-21 17:19   ` Romain Naour
  2017-02-07  9:03   ` Peter Korsgaard
  2017-01-21 17:56 ` [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Romain Naour
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
  10 siblings, 2 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2016-12-31  3:21 UTC (permalink / raw)
  To: buildroot

Warn when a variable is defined in a .mk file and it don't start with
the package name.

This function generates false warnings and the maintenance of the
whitelist can be an extra burden, but it catches some typos really hard
to see:
- POPLER_CONF_OPTS [1]
- BALELD_LICENSE [2]
- DRDB_UTILS_DEPENDENCIES [3]
- PERL_LIBWWW_LICENSE_FILES [4]
- AVRDUDR_LICENSE_FILES [5]
- GST1_PLUGINS_ULGY_HAS_GPL_LICENSE [6]
- ON2_8170_LICENSE [7]
- LIBFDTI_CONF_OPTS [8][9]
- IPSEC_DEPENDENCIES [10]

[1] http://patchwork.ozlabs.org/patch/681533
[2] http://patchwork.ozlabs.org/patch/643293
[3] http://patchwork.ozlabs.org/patch/449589
[4] http://patchwork.ozlabs.org/patch/464545
[5] http://patchwork.ozlabs.org/patch/305060
[6] http://patchwork.ozlabs.org/patch/253089
[7] http://patchwork.ozlabs.org/patch/250523
[8] http://patchwork.ozlabs.org/patch/394125
[9] https://github.com/buildroot/buildroot/commit/fe7a4b524b72bcb448f7e723873d8244620cb2f1
[10] https://github.com/buildroot/buildroot/commit/dff1d590b2a0fadf58b6eed60029b2ecbab7c710

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	0m3.088s
    user	0m3.012s
    sys	0m0.072s
    
    CHECK_TYPO_IN_PACKAGE_VARIABLE:
     support/scripts/check-package --include-only check_typo_in_package_variable \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      17
     (cd support/scripts/check-package-example && \
     ../check-package --include-only check_typo_in_package_variable -vv package/*/*)
      package/package1/package1.mk:31: possible typo: LINUX_DEPENDENCIES -> *PACKAGE1*
      LINUX_DEPENDENCIES = messing with others
      package/package1/package1.mk:32: possible typo: PACKACE1_DEPENDENCIES -> *PACKAGE1*
      PACKACE1_DEPENDENCIES = typo
      159 lines processed
      2 warnings generated

 support/scripts/checkpackagelib_mk.py | 44 +++++++++++++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index 40574584a..50d22abfc 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -141,6 +141,50 @@ def check_trailing_backslash(
                 check_trailing_backslash.lastline]
 
 
+ALLOWED = re.compile("|".join([
+    "ACLOCAL_DIR",
+    "ACLOCAL_HOST_DIR",
+    "BR_CCACHE_INITIAL_SETUP",
+    "BR_NO_CHECK_HASH_FOR",
+    "LINUX_POST_PATCH_HOOKS",
+    "LINUX_TOOLS",
+    "LUA_RUN",
+    "MKFS_JFFS2",
+    "MKIMAGE_ARCH",
+    "PKG_CONFIG_HOST_BINARY",
+    "TARGET_FINALIZE_HOOKS",
+    "XTENSA_CORE_NAME"]))
+PACKAGE_NAME = re.compile("/([^/]+)\.mk")
+VARIABLE = re.compile("^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
+
+
+def check_typo_in_package_variable(
+        fname, args, lineno=0, text=None, start=False, end=False):
+    if start:
+        package = PACKAGE_NAME.search(fname).group(1).replace("-", "_").upper()
+        # linux tools do not use LINUX_TOOL_ prefix for variables
+        package = package.replace("LINUX_TOOL_", "")
+        check_typo_in_package_variable.package = package
+        check_typo_in_package_variable.REGEX = re.compile(
+            "^(HOST_)?({}_[A-Z0-9_]+)".format(package))
+        return
+    if end:
+        return
+
+    m = VARIABLE.search(text)
+    if m is None:
+        return
+
+    variable = m.group(1)
+    if ALLOWED.match(variable):
+        return
+    if check_typo_in_package_variable.REGEX.search(text) is None:
+        return ["{}:{}: possible typo: {} -> *{}*"
+                .format(fname, lineno, variable,
+                        check_typo_in_package_variable.package),
+                text]
+
+
 DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
     "_AUTORECONF\s*=\s*NO",
     "_LIBTOOL_PATCH\s*=\s*YES"])))
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description
  2016-12-31  3:21 ` [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description Ricardo Martincoski
@ 2017-01-21 16:58   ` Romain Naour
  2017-02-07  1:10     ` Ricardo Martincoski
  0 siblings, 1 reply; 41+ messages in thread
From: Romain Naour @ 2017-01-21 16:58 UTC (permalink / raw)
  To: buildroot

Hi Ricardo,

Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :
> Explicitly state that one tab counts for 8 columns in package
> description, leaving 62 characters to the text itself.
> Update the text and the example in the two places where the Config.in
> format is described.
> Also mention a newline is expected between the help text itself and the
> upstream URL.

I started to use these scripts today, and it's really a nice work!
My current setting in my editor use 4 characters for one tab, so all packages I
made until now produce a check_help_text warning :-(
But ok, 8 spaces for one tab seems to be the default for a long time [1].

[1] https://en.wikipedia.org/wiki/Tab_key

Best regards,
Romain

> 
> This blob can help newcomers to understand the expected formatting.
> Also, it can be referenced by reviewers.
> 
> Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  docs/manual/adding-packages-directory.txt | 8 +++++---
>  docs/manual/writing-rules.txt             | 6 ++++--
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
> index 5dba962bd..08f5d42f9 100644
> --- a/docs/manual/adding-packages-directory.txt
> +++ b/docs/manual/adding-packages-directory.txt
> @@ -28,7 +28,8 @@ contain:
>  config BR2_PACKAGE_LIBFOO
>  	bool "libfoo"
>  	help
> -	  This is a comment that explains what libfoo is.
> +	  This is a comment that explains what libfoo is. The help text
> +	  should be wrapped.
>  
>  	  http://foosoftware.org/libfoo/
>  ---------------------------
> @@ -36,8 +37,9 @@ config BR2_PACKAGE_LIBFOO
>  The +bool+ line, +help+ line and other metadata information about the
>  configuration option must be indented with one tab. The help text
>  itself should be indented with one tab and two spaces, lines should
> -not be longer than 72 columns, and it must mention the upstream URL
> -of the project.
> +be wrapped to fit 72 columns, where tab counts for 8, so 62 characters
> +in the text itself. The help text must mention the upstream URL of the
> +project after an empty line.
>  
>  As a convention specific to Buildroot, the ordering of the attributes
>  is as follows:
> diff --git a/docs/manual/writing-rules.txt b/docs/manual/writing-rules.txt
> index ec1ddb191..e2ad41ebc 100644
> --- a/docs/manual/writing-rules.txt
> +++ b/docs/manual/writing-rules.txt
> @@ -29,7 +29,8 @@ config BR2_PACKAGE_LIBFOO
>  	depends on BR2_PACKAGE_LIBBAZ
>  	select BR2_PACKAGE_LIBBAR
>  	help
> -	  This is a comment that explains what libfoo is.
> +	  This is a comment that explains what libfoo is. The help text
> +	  should be wrapped.
>  
>  	  http://foosoftware.org/libfoo/
>  ---------------------
> @@ -40,7 +41,8 @@ config BR2_PACKAGE_LIBFOO
>  * The help text itself should be indented with one tab and two
>    spaces.
>  
> -* The help text should be wrapped to fit 72 columns.
> +* The help text should be wrapped to fit 72 columns, where tab counts
> +  for 8, so 62 characters in the text itself.
>  
>  The +Config.in+ files are the input for the configuration tool
>  used in Buildroot, which is the regular _Kconfig_. For further
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable
  2016-12-31  3:21 ` [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
@ 2017-01-21 17:19   ` Romain Naour
  2017-02-07  0:33     ` Ricardo Martincoski
  2017-02-07  9:03   ` Peter Korsgaard
  1 sibling, 1 reply; 41+ messages in thread
From: Romain Naour @ 2017-01-21 17:19 UTC (permalink / raw)
  To: buildroot

Hi Ricardo,

Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :
> Warn when a variable is defined in a .mk file and it don't start with
> the package name.
> 
> This function generates false warnings and the maintenance of the
> whitelist can be an extra burden, but it catches some typos really hard
> to see:
> - POPLER_CONF_OPTS [1]
> - BALELD_LICENSE [2]
> - DRDB_UTILS_DEPENDENCIES [3]
> - PERL_LIBWWW_LICENSE_FILES [4]
> - AVRDUDR_LICENSE_FILES [5]
> - GST1_PLUGINS_ULGY_HAS_GPL_LICENSE [6]
> - ON2_8170_LICENSE [7]
> - LIBFDTI_CONF_OPTS [8][9]
> - IPSEC_DEPENDENCIES [10]
> 
> [1] http://patchwork.ozlabs.org/patch/681533
> [2] http://patchwork.ozlabs.org/patch/643293
> [3] http://patchwork.ozlabs.org/patch/449589
> [4] http://patchwork.ozlabs.org/patch/464545
> [5] http://patchwork.ozlabs.org/patch/305060
> [6] http://patchwork.ozlabs.org/patch/253089
> [7] http://patchwork.ozlabs.org/patch/250523
> [8] http://patchwork.ozlabs.org/patch/394125
> [9] https://github.com/buildroot/buildroot/commit/fe7a4b524b72bcb448f7e723873d8244620cb2f1
> [10] https://github.com/buildroot/buildroot/commit/dff1d590b2a0fadf58b6eed60029b2ecbab7c710

This one produce a false positive with MYSQL_SOCKET from oracle-mysql because
it's a virtual package providing mysql:

package/oracle-mysql/oracle-mysql.mk:19: possible typo: MYSQL_SOCKET ->
*ORACLE_MYSQL*

MYSQL_SOCKET = /run/mysql/mysql.sock

ORACLE_MYSQL_PROVIDES = mysql

Maybe we could add an exception for variables named <<provider_name>_PROVIDES>_* ?

MYSQL_SOCKET is used in mariadb and php packages.

Other warnings (15) are real real typo or variable without the right package prefix.

Best regards,
Romain

> 
> 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	0m3.088s
>     user	0m3.012s
>     sys	0m0.072s
>     
>     CHECK_TYPO_IN_PACKAGE_VARIABLE:
>      support/scripts/check-package --include-only check_typo_in_package_variable \
>      $(find package -name '*.mk') 2>/dev/null | wc -l
>       17
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_typo_in_package_variable -vv package/*/*)
>       package/package1/package1.mk:31: possible typo: LINUX_DEPENDENCIES -> *PACKAGE1*
>       LINUX_DEPENDENCIES = messing with others
>       package/package1/package1.mk:32: possible typo: PACKACE1_DEPENDENCIES -> *PACKAGE1*
>       PACKACE1_DEPENDENCIES = typo
>       159 lines processed
>       2 warnings generated
> 
>  support/scripts/checkpackagelib_mk.py | 44 +++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
> index 40574584a..50d22abfc 100644
> --- a/support/scripts/checkpackagelib_mk.py
> +++ b/support/scripts/checkpackagelib_mk.py
> @@ -141,6 +141,50 @@ def check_trailing_backslash(
>                  check_trailing_backslash.lastline]
>  
>  
> +ALLOWED = re.compile("|".join([
> +    "ACLOCAL_DIR",
> +    "ACLOCAL_HOST_DIR",
> +    "BR_CCACHE_INITIAL_SETUP",
> +    "BR_NO_CHECK_HASH_FOR",
> +    "LINUX_POST_PATCH_HOOKS",
> +    "LINUX_TOOLS",
> +    "LUA_RUN",
> +    "MKFS_JFFS2",
> +    "MKIMAGE_ARCH",
> +    "PKG_CONFIG_HOST_BINARY",
> +    "TARGET_FINALIZE_HOOKS",
> +    "XTENSA_CORE_NAME"]))
> +PACKAGE_NAME = re.compile("/([^/]+)\.mk")
> +VARIABLE = re.compile("^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
> +
> +
> +def check_typo_in_package_variable(
> +        fname, args, lineno=0, text=None, start=False, end=False):
> +    if start:
> +        package = PACKAGE_NAME.search(fname).group(1).replace("-", "_").upper()
> +        # linux tools do not use LINUX_TOOL_ prefix for variables
> +        package = package.replace("LINUX_TOOL_", "")
> +        check_typo_in_package_variable.package = package
> +        check_typo_in_package_variable.REGEX = re.compile(
> +            "^(HOST_)?({}_[A-Z0-9_]+)".format(package))
> +        return
> +    if end:
> +        return
> +
> +    m = VARIABLE.search(text)
> +    if m is None:
> +        return
> +
> +    variable = m.group(1)
> +    if ALLOWED.match(variable):
> +        return
> +    if check_typo_in_package_variable.REGEX.search(text) is None:
> +        return ["{}:{}: possible typo: {} -> *{}*"
> +                .format(fname, lineno, variable,
> +                        check_typo_in_package_variable.package),
> +                text]
> +
> +
>  DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
>      "_AUTORECONF\s*=\s*NO",
>      "_LIBTOOL_PATCH\s*=\s*YES"])))
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (8 preceding siblings ...)
  2016-12-31  3:21 ` [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
@ 2017-01-21 17:56 ` Romain Naour
  2017-02-07  0:52   ` Ricardo Martincoski
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
  10 siblings, 1 reply; 41+ messages in thread
From: Romain Naour @ 2017-01-21 17:56 UTC (permalink / raw)
  To: buildroot

Hi Ricardo,

Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :
> This patch series is only a prototype.
> Please feel free to reject the series if it is too far from what you want.
> 
> I started this patch series out of curiosity.
> Later on, I saw that something similar was in the todo list [1].
> 
> It probably could be achieved using flex + bison, or pyacc, or perl, or
> something else.
> But I implemented using python and non-strict checks, trying to:
> - reduce the chance of problems related to differences between host machines;
> - allow incremental development based on feedback from patch reviews;
> - keep the maintenance easy;
> - generate low number of false warnings.
> 
> The first patch is an example package to be used to test the output of the
> script for all patches in the series.
> 
> The second patch includes a txt file with an overall description and some hints,
> the main engine that calls check functions for each type of file, the first
> check function, and also many comments in the library files that reflect what I
> attempted to accomplish. Of course, the comments can be cut if you disagree with
> any of them.
> 
> Remaining patches include check functions for each type of file. I put the
> patches that I think are most likely to be rejected in the end.

I don't really agree with that, especially for the last one (check *.mk for typo
in variable) since it will catch easily our/my keyboard dyslexia symptoms :)

> 
> Also in the series there is a blob for the manual trying to clarify the expected
> format for help text.
> 
> I did not include yet:
> - entry to DEVELOPERS (just because it is the first iteration);
> - explicit license entry (I guess this way the script inherits the Buildroot
>   license).
> 
> For each patch I include using 'git notes':
> - the time it takes to run for all current packages;

Well ~3 seconds to check all packages with all check enable is not bad.

> - for each check function:
>   - the number of warnings it generates for all current packages;
>   - sample output running on the (bad) example package.
> 
> Here is the list of check functions implemented in this series:
> 
> *.patch:
> - check_apply_order
> - check_newline_at_eof
> - check_numbered_subject
> - check_sob
> 
> Config.in.*:
> - check_attributes_order
> - check_consecutive_empty_lines
> - check_empty_last_line
> - check_help_text
> - check_indent
> - check_newline_at_eof
> - check_trailing_space
> 
> *.hash:
> - check_consecutive_empty_lines
> - check_empty_last_line
> - check_hash_filename
> - check_hash_number_of_fields
> - check_hash_type
> - check_newline_at_eof
> - check_trailing_space
> 
> *.mk:
> - check_consecutive_empty_lines
> - check_empty_last_line
> - check_indent
> - check_newline_at_eof
> - check_package_header
> - check_space_before_backslash
> - check_trailing_backslash
> - check_trailing_space
> - check_typo_in_package_variable
> - check_useless_flag
> 
> [1] http://elinux.org/Buildroot#Todo_list

The check-package script find several thousand (~2500) coding style issue or
typos. I think the most important now is to fix all typos in variable since it
can lead to a build failure or a licensing issue.

I'll continue to test this series and probably use it during the next Buildroot
meeting.

Thanks!

Best regards,
Romain

> 
> Regards,
> Ricardo
> 
> 
> Ricardo Martincoski (9):
>   support/scripts/check-package: example
>   support/scripts/check-package: new script
>   check-package: check whitespace and empty lines
>   check-package: check *.hash files
>   check-package: check *.patch files
>   check-package: check *.mk files
>   docs/manual: size of tab in package description
>   check-package: check Config.in.* files
>   check-package: check *.mk for typo in variable
> 
>  docs/manual/adding-packages-directory.txt          |   8 +-
>  docs/manual/writing-rules.txt                      |   6 +-
>  support/scripts/check-package                      | 136 ++++++++++++
>  .../package/package1/0001-do-something.patch       |  24 +++
>  .../package/package1/0002-do-something-else.patch  |  24 +++
>  .../package/package1/Config.in                     |  42 ++++
>  .../package/package1/package1.hash                 |   8 +
>  .../package/package1/package1.mk                   |  48 +++++
>  .../package/package1/wrong-name.patch              |  13 ++
>  support/scripts/check-package.txt                  |  76 +++++++
>  support/scripts/checkpackagelib.py                 |  56 +++++
>  support/scripts/checkpackagelib_config.py          | 136 ++++++++++++
>  support/scripts/checkpackagelib_hash.py            |  76 +++++++
>  support/scripts/checkpackagelib_mk.py              | 229 +++++++++++++++++++++
>  support/scripts/checkpackagelib_patch.py           |  50 +++++
>  15 files changed, 927 insertions(+), 5 deletions(-)
>  create mode 100755 support/scripts/check-package
>  create mode 100644 support/scripts/check-package-example/package/package1/0001-do-something.patch
>  create mode 100644 support/scripts/check-package-example/package/package1/0002-do-something-else.patch
>  create mode 100644 support/scripts/check-package-example/package/package1/Config.in
>  create mode 100644 support/scripts/check-package-example/package/package1/package1.hash
>  create mode 100644 support/scripts/check-package-example/package/package1/package1.mk
>  create mode 100644 support/scripts/check-package-example/package/package1/wrong-name.patch
>  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
> 

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 2/9] support/scripts/check-package: new script
  2016-12-31  3:21 ` [Buildroot] [PATCH 2/9] support/scripts/check-package: new script Ricardo Martincoski
@ 2017-01-24 21:14   ` Thomas De Schampheleire
  2017-02-06 18:53     ` Thomas De Schampheleire
  2017-02-19 23:13     ` Ricardo Martincoski
  0 siblings, 2 replies; 41+ messages in thread
From: Thomas De Schampheleire @ 2017-01-24 21:14 UTC (permalink / raw)
  To: buildroot

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 4/9] check-package: check *.hash files
  2016-12-31  3:21 ` [Buildroot] [PATCH 4/9] check-package: check *.hash files Ricardo Martincoski
@ 2017-01-24 21:18   ` Thomas De Schampheleire
  2017-02-19 23:16     ` Ricardo Martincoski
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas De Schampheleire @ 2017-01-24 21:18 UTC (permalink / raw)
  To: buildroot

On Sat, Dec 31, 2016 at 4:21 AM, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
> Check each hash entry (see [1]) and warn when:
> - it does not have three fields;
> - its type is unknown;
> - its length does not match its type;
> - the name of the file contains a directory component.
>
> [1] http://nightly.buildroot.org/#adding-packages-hash
>
> 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.837s
>     user        0m0.780s
>     sys 0m0.056s
>
>     CHECK_HASH_FILENAME:
>      support/scripts/check-package --include-only check_hash_filename \
>      $(find package -name '*.hash') 2>/dev/null | wc -l
>       0
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_hash_filename -vv package/*/*)
>       package/package1/package1.hash:5: use filename without directory component (http://nightly.buildroot.org/#adding-packages-hash)
>       sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
>       159 lines processed
>       1 warnings generated
>
>     CHECK_HASH_NUMBER_OF_FIELDS:
>      support/scripts/check-package --include-only check_hash_number_of_fields \
>      $(find package -name '*.hash') 2>/dev/null | wc -l
>       0
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_hash_number_of_fields -vv package/*/*)
>       package/package1/package1.hash:6: expected three fields (http://nightly.buildroot.org/#adding-packages-hash)
>       sha256 1234567890123456789012345678901234567890123456789012345678901234
>       159 lines processed
>       1 warnings generated
>
>     CHECK_HASH_TYPE:
>      support/scripts/check-package --include-only check_hash_type \
>      $(find package -name '*.hash') 2>/dev/null | wc -l
>       1
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_hash_type -vv package/*/*)
>       package/package1/package1.hash:3: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
>       sha256 123456789 package1-1.0.tar.gz
>       expected 64 hex digits
>       package/package1/package1.hash:4: unexpected type of hash (http://nightly.buildroot.org/#adding-packages-hash)
>       crc16< tab  >123456789< tab  >package1-1.0.tar.gz
>       package/package1/package1.hash:5: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
>       sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
>       expected 64 hex digits
>       159 lines processed
>       3 warnings generated
>
>  support/scripts/checkpackagelib_hash.py | 66 +++++++++++++++++++++++++++++++++
>  1 file changed, 66 insertions(+)
>
> diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
> index d095f9255..646c6cc12 100644
> --- a/support/scripts/checkpackagelib_hash.py
> +++ b/support/scripts/checkpackagelib_hash.py
> @@ -3,8 +3,74 @@
>  # functions don't need to check for things already checked by running
>  # "make package-dirclean package-source".
>
> +import re
> +
>  # Notice: ignore 'imported but unused' from pyflakes for check functions.
>  from checkpackagelib import check_consecutive_empty_lines
>  from checkpackagelib import check_empty_last_line
>  from checkpackagelib import check_newline_at_eof
>  from checkpackagelib import check_trailing_space
> +
> +
> +def _empty_line_or_comment(text):
> +    return text.strip() == "" or text.startswith("#")
> +
> +
> +FILENAME_WITH_SLASH = re.compile("/")
> +
> +
> +def check_hash_filename(
> +        fname, args, lineno=0, text=None, start=False, end=False):
> +    if start or end or _empty_line_or_comment(text):
> +        return
> +
> +    fields = text.split()
> +    if len(fields) < 3:
> +        return
> +
> +    filename = fields[2]
> +    if FILENAME_WITH_SLASH.search(filename):

This is overkill. It is more simple and I expect more performant to just do:

    if '/' in filename:

Best regards,
Thomas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 5/9] check-package: check *.patch files
  2016-12-31  3:21 ` [Buildroot] [PATCH 5/9] check-package: check *.patch files Ricardo Martincoski
@ 2017-01-24 21:21   ` Thomas De Schampheleire
  2017-02-07  9:58     ` Thomas Petazzoni
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas De Schampheleire @ 2017-01-24 21:21 UTC (permalink / raw)
  To: buildroot

On Sat, Dec 31, 2016 at 4:21 AM, Ricardo Martincoski
<ricardo.martincoski@gmail.com> wrote:
> Warn when the name of the patch file does not start with number (apply
> order), see [1].
> Warn when the patch was generated using git format-patch without -N, see
> [2].
> Warn when the patch file has no SoB, see [3].
>
> [1] http://nightly.buildroot.org/#_providing_patches
> [2] http://patchwork.ozlabs.org/patch/704753/
> [3] http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches
>
> 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        0m1.153s
>     user        0m1.096s
>     sys 0m0.056s
>
>     CHECK_APPLY_ORDER:
>      support/scripts/check-package --include-only check_apply_order \
>      $(find package -name '*.patch') 2>/dev/null | wc -l
>       4
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_apply_order -vv package/*/*)
>       package/package1/wrong-name.patch:0: use name <number>-<description>.patch (http://nightly.buildroot.org/#_providing_patches)
>       159 lines processed
>       1 warnings generated
>
>     CHECK_NUMBERED_SUBJECT:
>      support/scripts/check-package --include-only check_numbered_subject \
>      $(find package -name '*.patch') 2>/dev/null | wc -l
>       149
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_numbered_subject -vv package/*/*)
>       package/package1/0001-do-something.patch:4: generate your patches with 'git format-patch -N'
>       Subject: [PATCH 25/39] do something
>       159 lines processed
>       1 warnings generated
>
>     CHECK_SOB:
>      support/scripts/check-package --include-only check_sob \
>      $(find package -name '*.patch') 2>/dev/null | wc -l
>       143
>      (cd support/scripts/check-package-example && \
>      ../check-package --include-only check_sob -vv package/*/*)
>       package/package1/0001-do-something.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
>       package/package1/wrong-name.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
>       159 lines processed
>       2 warnings generated
>
>  support/scripts/checkpackagelib_patch.py | 43 ++++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
>
> diff --git a/support/scripts/checkpackagelib_patch.py b/support/scripts/checkpackagelib_patch.py
> index 0fb3685a7..f10dc31c4 100644
> --- a/support/scripts/checkpackagelib_patch.py
> +++ b/support/scripts/checkpackagelib_patch.py
> @@ -3,5 +3,48 @@
>  # functions don't need to check for things already checked by running
>  # "make package-dirclean package-patch".
>
> +import re
> +
>  # Notice: ignore 'imported but unused' from pyflakes for check functions.
>  from checkpackagelib import check_newline_at_eof
> +
> +
> +APPLY_ORDER = re.compile("/\d{1,4}-[^/]*$")
> +
> +
> +def check_apply_order(
> +        fname, args, lineno=0, text=None, start=False, end=False):
> +    if start and not APPLY_ORDER.search(fname):
> +        return ["{}:0: use name <number>-<description>.patch "
> +                "({}#_providing_patches)".format(fname, args.manual_url)]
> +
> +
> +NUMBERED_PATCH = re.compile("Subject:\s*\[PATCH\s*\d+/\d+\]")

Is this really a requirement? This numbering in the patch header is
unrelated to the number used in the filename. One can expect many
'PATCH 1/1' in the same package directory. I would leave out this
test.

/Thomas

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 2/9] support/scripts/check-package: new script
  2017-01-24 21:14   ` Thomas De Schampheleire
@ 2017-02-06 18:53     ` Thomas De Schampheleire
  2017-02-07  0:17       ` Ricardo Martincoski
  2017-02-19 23:13     ` Ricardo Martincoski
  1 sibling, 1 reply; 41+ messages in thread
From: Thomas De Schampheleire @ 2017-02-06 18:53 UTC (permalink / raw)
  To: buildroot

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 2/9] support/scripts/check-package: new script
  2017-02-06 18:53     ` Thomas De Schampheleire
@ 2017-02-07  0:17       ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-07  0:17 UTC (permalink / raw)
  To: buildroot

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable
  2017-01-21 17:19   ` Romain Naour
@ 2017-02-07  0:33     ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-07  0:33 UTC (permalink / raw)
  To: buildroot

Romain,

On Sat, Jan 21, 2017 at 03:19 PM, Romain Naour wrote:

> Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :

[snip]

> This one produce a false positive with MYSQL_SOCKET from oracle-mysql because
> it's a virtual package providing mysql:
> 
> package/oracle-mysql/oracle-mysql.mk:19: possible typo: MYSQL_SOCKET ->
> *ORACLE_MYSQL*
> 
> MYSQL_SOCKET = /run/mysql/mysql.sock

The easy way is to just add MYSQL_SOCKET to the ALLOWED regex ...

> 
> ORACLE_MYSQL_PROVIDES = mysql
> 
> Maybe we could add an exception for variables named <<provider_name>_PROVIDES>_* ?

... but I will try this generic approach, probably in a similar way I did for
the LINUX_TOOL_ prefix. It sounds more correct.

> 
> MYSQL_SOCKET is used in mariadb and php packages.
> 
> Other warnings (15) are real real typo or variable without the right package prefix.

Thank you for your review.
Sorry the delay.

Best regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style
  2017-01-21 17:56 ` [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Romain Naour
@ 2017-02-07  0:52   ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-07  0:52 UTC (permalink / raw)
  To: buildroot

Romain,

On Sat, Jan 21, 2017 at 03:56 PM, Romain Naour wrote:

> Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :

[snip]

>> Remaining patches include check functions for each type of file. I put the
>> patches that I think are most likely to be rejected in the end.
> 
> I don't really agree with that, especially for the last one (check *.mk for typo
> in variable) since it will catch easily our/my keyboard dyslexia symptoms :)

:)

> 
>> 
>> Also in the series there is a blob for the manual trying to clarify the expected
>> format for help text.
>> 
>> I did not include yet:
>> - entry to DEVELOPERS (just because it is the first iteration);
>> - explicit license entry (I guess this way the script inherits the Buildroot
>>   license).
>> 
>> For each patch I include using 'git notes':
>> - the time it takes to run for all current packages;
> 
> Well ~3 seconds to check all packages with all check enable is not bad.
> 
>> - for each check function:
>>   - the number of warnings it generates for all current packages;
>>   - sample output running on the (bad) example package.

[snip]

>> 
>> [1] http://elinux.org/Buildroot#Todo_list
> 
> The check-package script find several thousand (~2500) coding style issue or
> typos. I think the most important now is to fix all typos in variable since it
> can lead to a build failure or a licensing issue.

I will start with the easy ones :) among the typos in variables.

> 
> I'll continue to test this series and probably use it during the next Buildroot
> meeting.

Nice. Thanks.

Best regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description
  2017-01-21 16:58   ` Romain Naour
@ 2017-02-07  1:10     ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-07  1:10 UTC (permalink / raw)
  To: buildroot

Romain,

On Sat, Jan 21, 2017 at 02:58 PM, Romain Naour wrote:

> Le 31/12/2016 ? 04:21, Ricardo Martincoski a ?crit :
>> Explicitly state that one tab counts for 8 columns in package
>> description, leaving 62 characters to the text itself.
>> Update the text and the example in the two places where the Config.in
>> format is described.
>> Also mention a newline is expected between the help text itself and the
>> upstream URL.
> 
> I started to use these scripts today, and it's really a nice work!
> My current setting in my editor use 4 characters for one tab, so all packages I
> made until now produce a check_help_text warning :-(
> But ok, 8 spaces for one tab seems to be the default for a long time [1].
> 
> [1] https://en.wikipedia.org/wiki/Tab_key

I missed to add to the commit message that I based that info in past reviews...

http://patchwork.ozlabs.org/patch/611289/
http://patchwork.ozlabs.org/patch/606866/
http://patchwork.ozlabs.org/patch/459960/

Best regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable
  2016-12-31  3:21 ` [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
  2017-01-21 17:19   ` Romain Naour
@ 2017-02-07  9:03   ` Peter Korsgaard
  1 sibling, 0 replies; 41+ messages in thread
From: Peter Korsgaard @ 2017-02-07  9:03 UTC (permalink / raw)
  To: buildroot

>>>>> "Ricardo" == Ricardo Martincoski <ricardo.martincoski@gmail.com> writes:

 > Warn when a variable is defined in a .mk file and it don't start with
 > the package name.

 > This function generates false warnings and the maintenance of the
 > whitelist can be an extra burden, but it catches some typos really hard
 > to see:
 > - POPLER_CONF_OPTS [1]
 > - BALELD_LICENSE [2]
 > - DRDB_UTILS_DEPENDENCIES [3]
 > - PERL_LIBWWW_LICENSE_FILES [4]
 > - AVRDUDR_LICENSE_FILES [5]
 > - GST1_PLUGINS_ULGY_HAS_GPL_LICENSE [6]
 > - ON2_8170_LICENSE [7]
 > - LIBFDTI_CONF_OPTS [8][9]
 > - IPSEC_DEPENDENCIES [10]

Thanks, I've fixed these typos!

-- 
Bye, Peter Korsgaard

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 5/9] check-package: check *.patch files
  2017-01-24 21:21   ` Thomas De Schampheleire
@ 2017-02-07  9:58     ` Thomas Petazzoni
  2017-02-19 23:41       ` Ricardo Martincoski
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Petazzoni @ 2017-02-07  9:58 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 24 Jan 2017 22:21:31 +0100, Thomas De Schampheleire wrote:

> > +NUMBERED_PATCH = re.compile("Subject:\s*\[PATCH\s*\d+/\d+\]")  
> 
> Is this really a requirement? This numbering in the patch header is
> unrelated to the number used in the filename. One can expect many
> 'PATCH 1/1' in the same package directory. I would leave out this
> test.

We normally require patches in package/<foo>/ to be generated with "git
format-patch -N", so that their prefix is [PATCH] and not [PATCH x/y].

However, we do not require all patches to be Git formatted. It's kind
of required when the upstream project uses Git as its version control
system. But when the upstream project uses some other version control
system, other patch formats are excepted, like hand-generated patches
with "diff", or patches generated with "quilt". Not everyone wants to
find with Mercurial to find out to generate a patch with this damn
thing :-)

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
                   ` (9 preceding siblings ...)
  2017-01-21 17:56 ` [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Romain Naour
@ 2017-02-19 22:17 ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 1/9] support/scripts/check-package: example Ricardo Martincoski
                     ` (9 more replies)
  10 siblings, 10 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

I started this patch series out of curiosity.
Later on, I saw that something similar was in the todo list [1].

It probably could be achieved using flex + bison, or pyacc, or perl, or
something else.
But I implemented using python and non-strict checks, trying to:
- reduce the chance of problems related to differences between host machines;
- allow incremental development based on feedback from patch reviews;
- keep the maintenance easy;
- generate low number of false warnings.

The first patch is an example package to be used to test the output of the
script for all patches in the series.

The second patch includes a txt file with an overall description and some hints,
the main engine that calls check functions for each type of file, the first
check function, and also many comments in the library files that reflect what I
attempted to accomplish. Of course, the comments can be cut if you disagree with
any of them.

Remaining patches include check functions for each type of file. I put the
patches that I think are most likely to be rejected in the end.

Also in the series there is a blob for the manual trying to clarify the expected
format for help text.

For each patch I include using 'git notes':
- the time it takes to run for all current packages;
- for each check function:
  - the number of warnings it generates for all current packages;
  - sample output running on the (bad) example package.

Here is the list of check functions implemented in this series:

*.patch:
- ApplyOrder
- NewlineAtEof
- NumberedSubject
- Sob

Config.*:
- AttributesOrder
- ConsecutiveEmptyLines
- EmptyLastLine
- HelpText
- Indent
- NewlineAtEof
- TrailingSpace

*.hash:
- ConsecutiveEmptyLines
- EmptyLastLine
- HashFilename
- HashNumberOfFields
- HashType
- NewlineAtEof
- TrailingSpace

*.mk:
- ConsecutiveEmptyLines
- EmptyLastLine
- Indent
- NewlineAtEof
- PackageHeader
- SpaceBeforeBackslash
- TrailingBackslash
- TrailingSpace
- TypoInPackageVariable
- UselessFlag

[1] http://elinux.org/Buildroot#Todo_list

Regards,
Ricardo
---
Changes v1 -> v2:
  - avoid false positive for variable of virtual package the package
    under analysis provides (e.g. MYSQL_SOCKET) (Romain);
  - do not warn for numbered subject if the patch was not generated with
    git (based on comments from Thomas DS and Thomas P);
  - support packages with different config filenames (Thomas DS);
  - add sample in the example patch for above cases;
  - use classes instead of functions to declare each check (Thomas DS);
  - update some commit messages;
  - do not use regex when a simple 'in' is enough (Thomas DS);
  - define named constants instead of magic '3' (Thomas DS);
  - assign to self (using classes) instead of using static variables
    (Thomas DS);
  - use a custom function as predicate to inspect.getmembers
    (Thomas DS);
  - rework how the command lines are passed to functions;
  - add entry do DEVELOPERS file;
  - remove the useless "check" prefix from all check functions;


Ricardo Martincoski (9):
  support/scripts/check-package: example
  support/scripts/check-package: new script
  check-package: check whitespace and empty lines
  check-package: check *.hash files
  check-package: check *.patch files
  check-package: check *.mk files
  docs/manual: size of tab in package description
  check-package: check Config.* files
  check-package: check *.mk for typo in variable

 DEVELOPERS                                         |   3 +
 docs/manual/adding-packages-directory.txt          |   8 +-
 docs/manual/writing-rules.txt                      |   6 +-
 support/scripts/check-package                      | 144 +++++++++++++
 .../package/package1/0001-do-something.patch       |  24 +++
 .../package/package1/0002-do-something-else.patch  |  24 +++
 .../package/package1/Config.in                     |  45 +++++
 .../package/package1/Config.something              |  10 +
 .../package/package1/package1.hash                 |   8 +
 .../package/package1/package1.mk                   |  55 +++++
 .../package/package1/wrong-name.patch              |  14 ++
 support/scripts/check-package.txt                  |  76 +++++++
 support/scripts/checkpackagebase.py                |  16 ++
 support/scripts/checkpackagelib.py                 |  54 +++++
 support/scripts/checkpackagelib_config.py          | 138 +++++++++++++
 support/scripts/checkpackagelib_hash.py            |  72 +++++++
 support/scripts/checkpackagelib_mk.py              | 223 +++++++++++++++++++++
 support/scripts/checkpackagelib_patch.py           |  62 ++++++
 18 files changed, 977 insertions(+), 5 deletions(-)
 create mode 100755 support/scripts/check-package
 create mode 100644 support/scripts/check-package-example/package/package1/0001-do-something.patch
 create mode 100644 support/scripts/check-package-example/package/package1/0002-do-something-else.patch
 create mode 100644 support/scripts/check-package-example/package/package1/Config.in
 create mode 100644 support/scripts/check-package-example/package/package1/Config.something
 create mode 100644 support/scripts/check-package-example/package/package1/package1.hash
 create mode 100644 support/scripts/check-package-example/package/package1/package1.mk
 create mode 100644 support/scripts/check-package-example/package/package1/wrong-name.patch
 create mode 100644 support/scripts/check-package.txt
 create mode 100644 support/scripts/checkpackagebase.py
 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

-- 
2.11.0

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 1/9] support/scripts/check-package: example
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 2/9] support/scripts/check-package: new script Ricardo Martincoski
                     ` (8 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Create an example package with many style problems.

These files can be used to test the check-package script.
Ideally each new warning added to the check script will have a bad style
example in these files.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Changes v1 -> v2:
  - add example of patch with numbered subject but that does not trigger
    warning because it is not a git patch (based on comments from
    Thomas DS and Thomas P in the "check *.patch files" patch);
  - example of variable of virtual package the package under analysis
    provides (based on comment from Romain in the "check *.mk for typo
    in variable" patch);
  - add example of Config file with different name (bases on comment
    from Thomas DS in the "new script" patch;
---

Notes:
    There are also bad styles inside these files that currently do not
    trigger a warning message from the script in this patch series.
    They provide an insight of what can be tested in the future.

 .../package/package1/0001-do-something.patch       | 24 ++++++++++
 .../package/package1/0002-do-something-else.patch  | 24 ++++++++++
 .../package/package1/Config.in                     | 45 ++++++++++++++++++
 .../package/package1/Config.something              | 10 ++++
 .../package/package1/package1.hash                 |  8 ++++
 .../package/package1/package1.mk                   | 55 ++++++++++++++++++++++
 .../package/package1/wrong-name.patch              | 14 ++++++
 7 files changed, 180 insertions(+)
 create mode 100644 support/scripts/check-package-example/package/package1/0001-do-something.patch
 create mode 100644 support/scripts/check-package-example/package/package1/0002-do-something-else.patch
 create mode 100644 support/scripts/check-package-example/package/package1/Config.in
 create mode 100644 support/scripts/check-package-example/package/package1/Config.something
 create mode 100644 support/scripts/check-package-example/package/package1/package1.hash
 create mode 100644 support/scripts/check-package-example/package/package1/package1.mk
 create mode 100644 support/scripts/check-package-example/package/package1/wrong-name.patch

diff --git a/support/scripts/check-package-example/package/package1/0001-do-something.patch b/support/scripts/check-package-example/package/package1/0001-do-something.patch
new file mode 100644
index 000000000..bcd01dc1a
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/0001-do-something.patch
@@ -0,0 +1,24 @@
+From 79752a7ce44e60e276fd22031f88c796eeebf69b Mon Sep 17 00:00:00 2001
+From: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+Date: Mon, 5 Dec 2016 23:03:16 -0200
+Subject: [PATCH 25/39] do something
+
+ Signed-off-bye: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+---
+ file | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/file b/file
+index 8a1218a..8ef9013 100644
+--- a/file
++++ b/file
+@@ -1,5 +1,5 @@
+ 1
+ 2
+-3
++33
+ 4
+ 5
+-- 
+2.11.0
+
diff --git a/support/scripts/check-package-example/package/package1/0002-do-something-else.patch b/support/scripts/check-package-example/package/package1/0002-do-something-else.patch
new file mode 100644
index 000000000..d95ec4876
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/0002-do-something-else.patch
@@ -0,0 +1,24 @@
+From 79752a7ce44e60e276fd22031f88c796eeebf69b Mon Sep 17 00:00:00 2001
+From: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+Date: Mon, 5 Dec 2016 23:03:16 -0200
+Subject: [PATCH] do something else
+
+Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
+---
+ file | 2 +-
+ 1 file changed, 1 insertion(+), 1 deletion(-)
+
+diff --git a/file b/file
+index 8a1218a..8ef9013 100644
+--- a/file
++++ b/file
+@@ -1,5 +1,5 @@
+ 1
+ 2
+-3
++33
+ 4
+ 5
+-- 
+2.11.0
+
diff --git a/support/scripts/check-package-example/package/package1/Config.in b/support/scripts/check-package-example/package/package1/Config.in
new file mode 100644
index 000000000..a788735b7
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/Config.in
@@ -0,0 +1,45 @@
+config BR2_PACKAGE_PACKAGE1
+	select BR2_PACKAGE_LIBEVENT
+	bool "pAcKaGe"
+	depends on BR2_USE_MMU
+	select BR2_PACKAGE_NCURSES
+	depends on BR2_USE_WCHAR
+	help
+	  package1 is a bad stylized package. Its only purpose is to exemplify
+          common style mistakes
+	some more help text but no url
+
+if BR2_PACKAGE_PACKAGE1
+ config BR2_PACKAGE_PACKAGE1_OPTION
+        bool "package1 option"
+        depends on BR2_USE_MMU
+        select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
+        help
+	  This paragraph is properly wrapped. Since a tab counts as 8
+	  spaces and the help text must be wrapped at column 72, only 62
+	  characters for the text itself are expect per line.
+
+
+	  Another paragraph. - But this time we cross the column 72 by 1.
+	 wrong_line_with_single_word
+	  http://www.example.com/ urls do not have spaces and this line is too long.
+
+	  http://www.example.com/folder/even_long_url_should_not_be_wrapped
+
+config BR2_PACKAGE_PACKAGE1_OPTION2
+	string "option2"
+	default "aarch64-unknown-linux-gnu" \
+		if BR2_aarch64 || BR2_aarch64_eb
+
+config BR2_PACKAGE_PACKAGE1_OPTION3
+	string "option4"
+	default "value" \
+                if BR2_aarch64
+
+source "package/package1/Config.something"
+
+endif
+
+comment "package1 needs a toolchain w/ locale"
+        depends on BR2_USE_MMU
+	depends on BR2_USE_WCHAR
diff --git a/support/scripts/check-package-example/package/package1/Config.something b/support/scripts/check-package-example/package/package1/Config.something
new file mode 100644
index 000000000..153584860
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/Config.something
@@ -0,0 +1,10 @@
+menu "Extensions"
+
+
+config BR2_PACKAGE_PACKAGE1_EXTENSION1
+	select BR2_PACKAGE_BZIP2
+	bool "Extension 1"
+	help
+	  A wonderful extension.
+
+endmenu
\ No newline at end of file
diff --git a/support/scripts/check-package-example/package/package1/package1.hash b/support/scripts/check-package-example/package/package1/package1.hash
new file mode 100644
index 000000000..78559bd4a
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/package1.hash
@@ -0,0 +1,8 @@
+# some comment:
+sha256 1234567890123456789012345678901234567890123456789012345678901234 package1-1.0.tar.gz 
+sha256 123456789 package1-1.0.tar.gz
+crc16	123456789	package1-1.0.tar.gz
+sha256	12345678901234567890123456789012345678901234567890123456789012345	dl/package1-1.0.tar.gz
+sha256 1234567890123456789012345678901234567890123456789012345678901234
+  
+
diff --git a/support/scripts/check-package-example/package/package1/package1.mk b/support/scripts/check-package-example/package/package1/package1.mk
new file mode 100644
index 000000000..ebd38ceb5
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/package1.mk
@@ -0,0 +1,55 @@
+########################################
+#
+# wrong name
+#
+########################################################################################################################
+PACKAGE1_VERSION=1.0
+PACKAGE1_SITE = https://localhost        
+PACKAGE1_LICENSE = GPL
+PACKAGE1_LICENSE_FILE = README
+PACKAGE1_LICENSE_FILES += COPYING
+PACKAGE1_PROVIDES = provided1 provided2
+PACKAGE1_PROVIDES += provided3
+# now some unneeded flags because they are the default value
+PACKAGE1_INSTALL_STAGING=NO 
+PACKAGE1_INSTALL_TARGET = YES	
+PACKAGE1_INSTALL_IMAGES  =  NO
+ PACKAGE1_INSTALL_REDISTRIBUTE = YES
+PACKAGE1_AUTORECONF = NO
+PACKAGE1_LIBTOOL_PATCH	=	YES
+ # but non-default conditionally overridden by default is allowed
+ifeq ($(BR2_STATIC_LIBS),y)
+	PACKAGE1_INSTALL_STAGING = NO
+endif
+
+
+PACKAGE1_DEPENDENCIES = depend1 depend2  \
+                       depend3
+PACKAGE1_DEPENDENCIES += depend5	\
+	depend4 \
+
+PACKAGE1_DEPENDENCIES = overwriting
+PACKAGE1_DEEEEEEEEEES = typo
+LINUX_DEPENDENCIES = messing with others
+PACKACE1_DEPENDENCIES = typo
+# package1 provides 3 virtual packages and can set variables with that prefixes
+PROVIDED1_LOCK = lock1
+PROVIDED2_LOCK   =   lock2
+PROVIDED3_LOCK = lock3
+NOT_PROVIDED4_LOCK = lock4
+
+define PACKAGE1_INSTALL_SOMETHING
+        mkdir -p $(TARGET_DIR)/var/lib
+	$(INSTALL) -m 0755 -D file1 \
+		$(TARGET_DIR)/var/lib/file
+	$(INSTALL) -m 0755 -D file2 \
+	$(TARGET_DIR)/etc/file
+endef
+
+define PACKAGE1_INSTALL_TARGET_CMDS
+	$(PACKAGE1_INSTALL_SOMETHING)
+        $(PACKAGE1_INSTALL_SOMETHING_ELSE)
+endef
+
+$(eval $(autotools-package))
+	
diff --git a/support/scripts/check-package-example/package/package1/wrong-name.patch b/support/scripts/check-package-example/package/package1/wrong-name.patch
new file mode 100644
index 000000000..c260ccda2
--- /dev/null
+++ b/support/scripts/check-package-example/package/package1/wrong-name.patch
@@ -0,0 +1,14 @@
+Subject: [PATCH 57/99] upstream does not use git
+no sob
+---
+diff -purN package.orig/file package/file
+--- package.orig/file	2000-01-01 22:07:43.275324499 -0300
++++ package/file	2016-01-01 22:08:16.171453283 -0300
+@@ -268,7 +268,6 @@ line
+ #include <errno.h>
+ #include <fcntl.h>
+ #include <string.h>
+-#include <termio.h>
+ #include <unistd.h>
+ #include <stdarg.h>
+ #include <curses.h>
\ No newline at end of file
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 2/9] support/scripts/check-package: new script
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 1/9] support/scripts/check-package: example Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
                     ` (7 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

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.*, *.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>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
---
Changes v1 -> v2:
  - support packages with different config filenames (Thomas DS);
  - define named constants instead of magic '3' (Thomas DS);
  - use classes instead of functions to declare each check (Thomas DS);
  - assign to self (using classes) instead of using static variables
    (Thomas DS);
  - the only command line argument the check functions need now is the
    url to the manual, so pass only this as argument instead of all
    flags;
  - add entry do DEVELOPERS file;
  - hold command line arguments as a global (to allow below changes);
  - use a custom function as predicate to inspect.getmembers
    (Thomas DS);
  - do not pass command line arguments everywhere;
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m0.391s
    user	0m0.344s
    sys	0m0.044s
    
    NewlineAtEof:
     support/scripts/check-package --include-only NewlineAtEof \
     $(find package -type f) 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only NewlineAtEof -vv package/*/*)
      package/package1/Config.something:10: missing newline at end of file
      endmenu
      package/package1/wrong-name.patch:14: missing newline at end of file
       #include <curses.h>
      180 lines processed
      2 warnings generated

 DEVELOPERS                                |   3 +
 support/scripts/check-package             | 144 ++++++++++++++++++++++++++++++
 support/scripts/check-package.txt         |  76 ++++++++++++++++
 support/scripts/checkpackagebase.py       |  16 ++++
 support/scripts/checkpackagelib.py        |  19 ++++
 support/scripts/checkpackagelib_config.py |   7 ++
 support/scripts/checkpackagelib_hash.py   |   7 ++
 support/scripts/checkpackagelib_mk.py     |   8 ++
 support/scripts/checkpackagelib_patch.py  |   7 ++
 9 files changed, 287 insertions(+)
 create mode 100755 support/scripts/check-package
 create mode 100644 support/scripts/check-package.txt
 create mode 100644 support/scripts/checkpackagebase.py
 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

diff --git a/DEVELOPERS b/DEVELOPERS
index 09a0a6e85..4dda13ce1 100644
--- a/DEVELOPERS
+++ b/DEVELOPERS
@@ -1260,6 +1260,9 @@ F:	package/atop/
 N:	Rhys Williams <github@wilberforce.co.nz>
 F:	package/lirc-tools/
 
+N:	Ricardo Martincoski <ricardo.martincoski@gmail.com>
+F:	support/scripts/check*package*
+
 N:	Richard Braun <rbraun@sceen.net>
 F:	package/curlftpfs/
 F:	package/tzdata/
diff --git a/support/scripts/check-package b/support/scripts/check-package
new file mode 100755
index 000000000..2add4712c
--- /dev/null
+++ b/support/scripts/check-package
@@ -0,0 +1,144 @@
+#!/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
+
+VERBOSE_LEVEL_TO_SHOW_IGNORED_FILES = 3
+flags = None  # Command line arguments.
+
+
+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\.\S*$")
+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 is_a_check_function(m):
+    if not inspect.isclass(m):
+        return False
+    # do not call the base class
+    if m.__name__.startswith("_"):
+        return False
+    if flags.include_list and m.__name__ not in flags.include_list:
+        return False
+    if flags.exclude_list and m.__name__ in flags.exclude_list:
+        return False
+    return True
+
+
+def print_warnings(warnings):
+    # 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 flags.verbose >= level:
+            print(message.replace("\t", "< tab  >").rstrip())
+    return 1  # One more warning to count.
+
+
+def check_file_using_lib(fname):
+    # Count number of warnings generated and lines processed.
+    nwarnings = 0
+    nlines = 0
+
+    lib = get_lib_from_filename(fname)
+    if not lib:
+        if flags.verbose >= VERBOSE_LEVEL_TO_SHOW_IGNORED_FILES:
+            print("{}: ignored".format(fname))
+        return nwarnings, nlines
+    classes = inspect.getmembers(lib, is_a_check_function)
+
+    if flags.dry_run:
+        functions_to_run = [c[0] for c in classes]
+        print("{}: would run: {}".format(fname, functions_to_run))
+        return nwarnings, nlines
+
+    objects = [c[1](fname, flags.manual_url) for c in classes]
+
+    for cf in objects:
+        nwarnings += print_warnings(cf.before())
+    for lineno, text in enumerate(open(fname, "r").readlines()):
+        nlines += 1
+        for cf in objects:
+            nwarnings += print_warnings(cf.check_line(lineno + 1, text))
+    for cf in objects:
+        nwarnings += print_warnings(cf.after())
+
+    return nwarnings, nlines
+
+
+def __main__():
+    global flags
+    flags = parse_args()
+
+    if len(flags.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 flags.files:
+        nwarnings, nlines = check_file_using_lib(fname)
+        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..630cd04f0
--- /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 classes declared in the library file and instantiates them.
+  The main engine opens the input files and it serves each raw line (including
+  newline!) to the method check_line() of every check object.
+  Two special methods before() and after() are used to call the initialization
+  of variables (for the case it needs to keep data across calls) and the
+  equivalent finalization (e.g. for the case a warning must be issued if some
+  pattern is not in the input file).
+- checkpackagebase.py contains the base class for all check functions.
+- checkpackagelib.py contains the classes for 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 class 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 and 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 that for every class the method before() will be called before
+  any line is served to be checked by the method check_line(). A class that
+  checks the filename should only implement the method before(). A function that
+  needs to keep data across calls (e.g. keep the last line before the one being
+  processed) should initialize all variables using this method.
+- keep in mind that for every class the method after() will be called after all
+  lines were served to be checked by the method check_line(). A class that
+  checks the absence of a pattern in the file will need to use this method.
+- 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. Something, check how it behaves
+  for all current packages:
+$ support/scripts/check-package --include-only 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/checkpackagebase.py b/support/scripts/checkpackagebase.py
new file mode 100644
index 000000000..e4c664d24
--- /dev/null
+++ b/support/scripts/checkpackagebase.py
@@ -0,0 +1,16 @@
+# See support/scripts/check-package.txt before editing this file.
+
+
+class _CheckFunction(object):
+    def __init__(self, filename, url_to_manual):
+        self.filename = filename
+        self.url_to_manual = url_to_manual
+
+    def before(self):
+        pass
+
+    def check_line(self, lineno, text):
+        pass
+
+    def after(self):
+        pass
diff --git a/support/scripts/checkpackagelib.py b/support/scripts/checkpackagelib.py
new file mode 100644
index 000000000..1a4904183
--- /dev/null
+++ b/support/scripts/checkpackagelib.py
@@ -0,0 +1,19 @@
+# See support/scripts/check-package.txt before editing this file.
+
+from checkpackagebase import _CheckFunction
+
+
+class NewlineAtEof(_CheckFunction):
+    def before(self):
+        self.lastlineno = 0
+        self.lastline = "\n"
+
+    def check_line(self, lineno, text):
+        self.lastlineno = lineno
+        self.lastline = text
+
+    def after(self):
+        if self.lastline == self.lastline.rstrip("\r\n"):
+            return ["{}:{}: missing newline at end of file"
+                    .format(self.filename, self.lastlineno),
+                    self.lastline]
diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
new file mode 100644
index 000000000..f546d173e
--- /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 NewlineAtEof
diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
new file mode 100644
index 000000000..8c0337fc9
--- /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 NewlineAtEof
diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
new file mode 100644
index 000000000..84eeef889
--- /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 NewlineAtEof
diff --git a/support/scripts/checkpackagelib_patch.py b/support/scripts/checkpackagelib_patch.py
new file mode 100644
index 000000000..131e42347
--- /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 NewlineAtEof
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 3/9] check-package: check whitespace and empty lines
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 1/9] support/scripts/check-package: example Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 2/9] support/scripts/check-package: new script Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 4/9] check-package: check *.hash files Ricardo Martincoski
                     ` (6 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Create 3 new check functions to warn when:
- there are consecutive empty lines in the file, see [1];
- the last line of the file is empty, see [2];
- there are lines with trailing whitespace, see [3].

Apply these functions to Config.*, *.mk and *.hash, but not for *.patch
files since they can contain any of these and still be valid.

[1] http://patchwork.ozlabs.org/patch/682660/
[2] http://patchwork.ozlabs.org/patch/643288/
[3] http://patchwork.ozlabs.org/patch/398984/

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Changes v1 -> v2:
  - update commit subject since now Config.* are tested;
  - use classes instead of functions to declare each check (Thomas DS);
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m0.622s
    user	0m0.576s
    sys	0m0.048s
    
    ConsecutiveEmptyLines:
     support/scripts/check-package --include-only ConsecutiveEmptyLines \
     $(find package -type f) 2>/dev/null | wc -l
      23
     (cd support/scripts/check-package-example && \
     ../check-package --include-only ConsecutiveEmptyLines -vv package/*/*)
      package/package1/Config.in:22: consecutive empty lines
      package/package1/Config.something:3: consecutive empty lines
      package/package1/package1.hash:8: consecutive empty lines
      package/package1/package1.mk:25: consecutive empty lines
      180 lines processed
      4 warnings generated
    
    EmptyLastLine:
     support/scripts/check-package --include-only EmptyLastLine \
     $(find package -type f) 2>/dev/null | wc -l
      18
     (cd support/scripts/check-package-example && \
     ../check-package --include-only EmptyLastLine -vv package/*/*)
      package/package1/package1.hash:8: empty line at end of file
      package/package1/package1.mk:55: empty line at end of file
      180 lines processed
      2 warnings generated
    
    TrailingSpace:
     support/scripts/check-package --include-only TrailingSpace \
     $(find package -type f) 2>/dev/null | wc -l
      5
     (cd support/scripts/check-package-example && \
     ../check-package --include-only TrailingSpace -vv package/*/*)
      package/package1/package1.hash:2: line contains trailing whitespace
      sha256 1234567890123456789012345678901234567890123456789012345678901234 package1-1.0.tar.gz
      package/package1/package1.hash:7: line contains trailing whitespace
    
      package/package1/package1.mk:7: line contains trailing whitespace
      PACKAGE1_SITE = https://localhost
      package/package1/package1.mk:14: line contains trailing whitespace
      PACKAGE1_INSTALL_STAGING=NO
      package/package1/package1.mk:15: line contains trailing whitespace
      PACKAGE1_INSTALL_TARGET = YES< tab  >
      package/package1/package1.mk:55: line contains trailing whitespace
      < tab  >
      180 lines processed
      6 warnings generated

 support/scripts/checkpackagelib.py        | 35 +++++++++++++++++++++++++++++++
 support/scripts/checkpackagelib_config.py |  3 +++
 support/scripts/checkpackagelib_hash.py   |  3 +++
 support/scripts/checkpackagelib_mk.py     |  3 +++
 4 files changed, 44 insertions(+)

diff --git a/support/scripts/checkpackagelib.py b/support/scripts/checkpackagelib.py
index 1a4904183..280084575 100644
--- a/support/scripts/checkpackagelib.py
+++ b/support/scripts/checkpackagelib.py
@@ -3,6 +3,32 @@
 from checkpackagebase import _CheckFunction
 
 
+class ConsecutiveEmptyLines(_CheckFunction):
+    def before(self):
+        self.lastline = "non empty"
+
+    def check_line(self, lineno, text):
+        if text.strip() == "" == self.lastline.strip():
+            return ["{}:{}: consecutive empty lines"
+                    .format(self.filename, lineno)]
+        self.lastline = text
+
+
+class EmptyLastLine(_CheckFunction):
+    def before(self):
+        self.lastlineno = 0
+        self.lastline = "non empty"
+
+    def check_line(self, lineno, text):
+        self.lastlineno = lineno
+        self.lastline = text
+
+    def after(self):
+        if self.lastline.strip() == "":
+            return ["{}:{}: empty line at end of file"
+                    .format(self.filename, self.lastlineno)]
+
+
 class NewlineAtEof(_CheckFunction):
     def before(self):
         self.lastlineno = 0
@@ -17,3 +43,12 @@ class NewlineAtEof(_CheckFunction):
             return ["{}:{}: missing newline at end of file"
                     .format(self.filename, self.lastlineno),
                     self.lastline]
+
+
+class TrailingSpace(_CheckFunction):
+    def check_line(self, lineno, text):
+        line = text.rstrip("\r\n")
+        if line != line.rstrip():
+            return ["{}:{}: line contains trailing whitespace"
+                    .format(self.filename, lineno),
+                    text]
diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
index f546d173e..ee5981e64 100644
--- a/support/scripts/checkpackagelib_config.py
+++ b/support/scripts/checkpackagelib_config.py
@@ -4,4 +4,7 @@
 # checked by running "make menuconfig".
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import ConsecutiveEmptyLines
+from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
+from checkpackagelib import TrailingSpace
diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
index 8c0337fc9..1f268838f 100644
--- a/support/scripts/checkpackagelib_hash.py
+++ b/support/scripts/checkpackagelib_hash.py
@@ -4,4 +4,7 @@
 # "make package-dirclean package-source".
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import ConsecutiveEmptyLines
+from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
+from checkpackagelib import TrailingSpace
diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index 84eeef889..a37304b6d 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -5,4 +5,7 @@
 # packages enabled.
 
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
+from checkpackagelib import ConsecutiveEmptyLines
+from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
+from checkpackagelib import TrailingSpace
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 4/9] check-package: check *.hash files
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (2 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 5/9] check-package: check *.patch files Ricardo Martincoski
                     ` (5 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Check each hash entry (see [1]) and warn when:
- it does not have three fields;
- its type is unknown;
- its length does not match its type;
- the name of the file contains a directory component.

[1] http://nightly.buildroot.org/#adding-packages-hash

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
---
Changes v1 -> v2:
  - do not use regex when a simple 'in' is enough (Thomas DS);
  - use classes instead of functions to declare each check (Thomas DS);
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m0.684s
    user	0m0.656s
    sys	0m0.024s
    
    HashFilename:
     support/scripts/check-package --include-only HashFilename \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only HashFilename -vv package/*/*)
      package/package1/package1.hash:5: use filename without directory component (http://nightly.buildroot.org/#adding-packages-hash)
      sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
      180 lines processed
      1 warnings generated
    
    HashNumberOfFields:
     support/scripts/check-package --include-only HashNumberOfFields \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only HashNumberOfFields -vv package/*/*)
      package/package1/package1.hash:6: expected three fields (http://nightly.buildroot.org/#adding-packages-hash)
      sha256 1234567890123456789012345678901234567890123456789012345678901234
      180 lines processed
      1 warnings generated
    
    HashType:
     support/scripts/check-package --include-only HashType \
     $(find package -name '*.hash') 2>/dev/null | wc -l
      1
     (cd support/scripts/check-package-example && \
     ../check-package --include-only HashType -vv package/*/*)
      package/package1/package1.hash:3: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
      sha256 123456789 package1-1.0.tar.gz
      expected 64 hex digits
      package/package1/package1.hash:4: unexpected type of hash (http://nightly.buildroot.org/#adding-packages-hash)
      crc16< tab  >123456789< tab  >package1-1.0.tar.gz
      package/package1/package1.hash:5: hash size does not match type (http://nightly.buildroot.org/#adding-packages-hash)
      sha256< tab  >12345678901234567890123456789012345678901234567890123456789012345< tab  >dl/package1-1.0.tar.gz
      expected 64 hex digits
      180 lines processed
      3 warnings generated

 support/scripts/checkpackagelib_hash.py | 62 +++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/support/scripts/checkpackagelib_hash.py b/support/scripts/checkpackagelib_hash.py
index 1f268838f..47fe18756 100644
--- a/support/scripts/checkpackagelib_hash.py
+++ b/support/scripts/checkpackagelib_hash.py
@@ -3,8 +3,70 @@
 # functions don't need to check for things already checked by running
 # "make package-dirclean package-source".
 
+import re
+
+from checkpackagebase import _CheckFunction
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import ConsecutiveEmptyLines
 from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
 from checkpackagelib import TrailingSpace
+
+
+def _empty_line_or_comment(text):
+    return text.strip() == "" or text.startswith("#")
+
+
+class HashFilename(_CheckFunction):
+    def check_line(self, lineno, text):
+        if _empty_line_or_comment(text):
+            return
+
+        fields = text.split()
+        if len(fields) < 3:
+            return
+
+        if '/' in fields[2]:
+            return ["{}:{}: use filename without directory component"
+                    " ({}#adding-packages-hash)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+
+
+class HashNumberOfFields(_CheckFunction):
+    def check_line(self, lineno, text):
+        if _empty_line_or_comment(text):
+            return
+
+        fields = text.split()
+        if len(fields) != 3:
+            return ["{}:{}: expected three fields ({}#adding-packages-hash)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+
+
+class HashType(_CheckFunction):
+    len_of_hash = {"md5": 32, "sha1": 40, "sha224": 56, "sha256": 64,
+                   "sha384": 96, "sha512": 128}
+
+    def check_line(self, lineno, text):
+        if _empty_line_or_comment(text):
+            return
+
+        fields = text.split()
+        if len(fields) < 2:
+            return
+
+        htype, hexa = fields[:2]
+        if htype == "none":
+            return
+        if htype not in self.len_of_hash.keys():
+            return ["{}:{}: unexpected type of hash ({}#adding-packages-hash)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+        if not re.match("^[0-9A-Fa-f]{%s}$" % self.len_of_hash[htype], hexa):
+            return ["{}:{}: hash size does not match type "
+                    "({}#adding-packages-hash)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text,
+                    "expected {} hex digits".format(self.len_of_hash[htype])]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 5/9] check-package: check *.patch files
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (3 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 4/9] check-package: check *.hash files Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 6/9] check-package: check *.mk files Ricardo Martincoski
                     ` (4 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Warn when the name of the patch file does not start with number (apply
order), see [1].
Warn when the patch was generated using git format-patch without -N, see
[2].
Warn when the patch file has no SoB, see [3].

[1] http://nightly.buildroot.org/#_providing_patches
[2] http://patchwork.ozlabs.org/patch/704753/
[3] http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas De Schampheleire <patrickdepinguin@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Changes v1 -> v2:
  - do not warn for numbered subject if the patch was not generated with
    git (based on comments from Thomas DS and Thomas P);
  - use classes instead of functions to declare each check (Thomas DS);
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m0.935s
    user	0m0.904s
    sys	0m0.028s
    
    ApplyOrder:
     support/scripts/check-package --include-only ApplyOrder \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      16
     (cd support/scripts/check-package-example && \
     ../check-package --include-only ApplyOrder -vv package/*/*)
      package/package1/wrong-name.patch:0: use name <number>-<description>.patch (http://nightly.buildroot.org/#_providing_patches)
      180 lines processed
      1 warnings generated
    
    NumberedSubject:
     support/scripts/check-package --include-only NumberedSubject \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      139
     (cd support/scripts/check-package-example && \
     ../check-package --include-only NumberedSubject -vv package/*/*)
      package/package1/0001-do-something.patch:4: generate your patches with 'git format-patch -N'
      Subject: [PATCH 25/39] do something
      180 lines processed
      1 warnings generated
    
    Sob:
     support/scripts/check-package --include-only Sob \
     $(find package -name '*.patch') 2>/dev/null | wc -l
      143
     (cd support/scripts/check-package-example && \
     ../check-package --include-only Sob -vv package/*/*)
      package/package1/0001-do-something.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
      package/package1/wrong-name.patch:0: missing Signed-off-by in the header (http://nightly.buildroot.org/#_format_and_licensing_of_the_package_patches)
      180 lines processed
      2 warnings generated

 support/scripts/checkpackagelib_patch.py | 55 ++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/support/scripts/checkpackagelib_patch.py b/support/scripts/checkpackagelib_patch.py
index 131e42347..ee46efb70 100644
--- a/support/scripts/checkpackagelib_patch.py
+++ b/support/scripts/checkpackagelib_patch.py
@@ -3,5 +3,60 @@
 # functions don't need to check for things already checked by running
 # "make package-dirclean package-patch".
 
+import re
+
+from checkpackagebase import _CheckFunction
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import NewlineAtEof
+
+
+class ApplyOrder(_CheckFunction):
+    APPLY_ORDER = re.compile("/\d{1,4}-[^/]*$")
+
+    def before(self):
+        if not self.APPLY_ORDER.search(self.filename):
+            return ["{}:0: use name <number>-<description>.patch "
+                    "({}#_providing_patches)"
+                    .format(self.filename, self.url_to_manual)]
+
+
+class NumberedSubject(_CheckFunction):
+    NUMBERED_PATCH = re.compile("Subject:\s*\[PATCH\s*\d+/\d+\]")
+
+    def before(self):
+        self.git_patch = False
+        self.lineno = 0
+        self.text = None
+
+    def check_line(self, lineno, text):
+        if text.startswith("diff --git"):
+            self.git_patch = True
+            return
+        if self.NUMBERED_PATCH.search(text):
+            self.lineno = lineno
+            self.text = text
+
+    def after(self):
+        if self.git_patch and self.text:
+            return ["{}:{}: generate your patches with 'git format-patch -N'"
+                    .format(self.filename, self.lineno),
+                    self.text]
+
+
+class Sob(_CheckFunction):
+    SOB_ENTRY = re.compile("^Signed-off-by: .*$")
+
+    def before(self):
+        self.found = False
+
+    def check_line(self, lineno, text):
+        if self.found:
+            return
+        if self.SOB_ENTRY.search(text):
+            self.found = True
+
+    def after(self):
+        if not self.found:
+            return ["{}:0: missing Signed-off-by in the header "
+                    "({}#_format_and_licensing_of_the_package_patches)"
+                    .format(self.filename, self.url_to_manual)]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 6/9] check-package: check *.mk files
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (4 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 5/9] check-package: check *.patch files Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 7/9] docs/manual: size of tab in package description Ricardo Martincoski
                     ` (3 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Warn when there are obvious indentation errors:
- the number of expect tabs is not yet checked since it is more complex
  to achieve;
- the content inside define ... endef should be indented with tab(s),
  see [1];
- line just after a backslash should be indented with tab(s), see [2];
- other lines should not be indented, see [3];
- ignore empty lines and comments.
Warn when there is no well-formatted header in the file:
- 80 hashes at lines 1 and 5;
- 1 hash at lines 2 and 4;
- empty line at line 6;
- see [4];
- ignore files that only include other mk files.
Warn when there are more than one space before backslash, see [5].
Warn when there is a trailing backslash [6].
Warn for flags set to default value YES or NO, see [7], [8], [9].

[1] http://patchwork.ozlabs.org/patch/681429/
[2] http://patchwork.ozlabs.org/patch/681430/
[3] http://patchwork.ozlabs.org/patch/559209/
[4] http://nightly.buildroot.org/#writing-rules-mk
[5] http://patchwork.ozlabs.org/patch/649084/
[6] http://patchwork.ozlabs.org/patch/535550/
[7] http://patchwork.ozlabs.org/patch/704718/
[8] http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems
[9] http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Changes v1 -> v2:
  - use classes instead of functions to declare each check (Thomas DS);
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m1.865s
    user	0m1.836s
    sys	0m0.028s
    
    Indent:
     support/scripts/check-package --include-only Indent \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      24
     (cd support/scripts/check-package-example && \
     ../check-package --include-only Indent -vv package/*/*)
      package/package1/package1.mk:22: unexpected indent with tabs
      < tab  >PACKAGE1_INSTALL_STAGING = NO
      package/package1/package1.mk:27: expected indent with tabs
                             depend3
      package/package1/package1.mk:42: expected indent with tabs
              mkdir -p $(TARGET_DIR)/var/lib
      package/package1/package1.mk:51: expected indent with tabs
              $(PACKAGE1_INSTALL_SOMETHING_ELSE)
      180 lines processed
      4 warnings generated
    
    PackageHeader:
     support/scripts/check-package --include-only PackageHeader \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      22
     (cd support/scripts/check-package-example && \
     ../check-package --include-only PackageHeader -vv package/*/*)
      package/package1/package1.mk:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)
      ########################################
      ################################################################################
      package/package1/package1.mk:5: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)
      ########################################################################################################################
      ################################################################################
      package/package1/package1.mk:6: should be a blank line (http://nightly.buildroot.org/#writing-rules-mk)
      PACKAGE1_VERSION=1.0
      180 lines processed
      3 warnings generated
    
    SpaceBeforeBackslash:
     support/scripts/check-package --include-only SpaceBeforeBackslash \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      342
     (cd support/scripts/check-package-example && \
     ../check-package --include-only SpaceBeforeBackslash -vv package/*/*)
      package/package1/package1.mk:26: use only one space before backslash
      PACKAGE1_DEPENDENCIES = depend1 depend2  \
      package/package1/package1.mk:28: use only one space before backslash
      PACKAGE1_DEPENDENCIES += depend5< tab  >\
      180 lines processed
      2 warnings generated
    
    TrailingBackslash:
     support/scripts/check-package --include-only TrailingBackslash \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      20
     (cd support/scripts/check-package-example && \
     ../check-package --include-only TrailingBackslash -vv package/*/*)
      package/package1/package1.mk:29: remove trailing backslash
      < tab  >depend4 \
      180 lines processed
      1 warnings generated
    
    UselessFlag:
     support/scripts/check-package --include-only UselessFlag \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      0
     (cd support/scripts/check-package-example && \
     ../check-package --include-only UselessFlag -vv package/*/*)
      package/package1/package1.mk:14: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_STAGING=NO
      package/package1/package1.mk:15: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_TARGET = YES< tab  >
      package/package1/package1.mk:16: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
      PACKAGE1_INSTALL_IMAGES  =  NO
      package/package1/package1.mk:17: useless default value (http://nightly.buildroot.org/#_infrastructure_for_packages_with_specific_build_systems)
       PACKAGE1_INSTALL_REDISTRIBUTE = YES
      package/package1/package1.mk:18: useless default value (http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages)
      PACKAGE1_AUTORECONF = NO
      package/package1/package1.mk:19: useless default value (http://nightly.buildroot.org/#_infrastructure_for_autotools_based_packages)
      PACKAGE1_LIBTOOL_PATCH< tab  >=< tab  >YES
      180 lines processed
      6 warnings generated

 support/scripts/checkpackagelib_mk.py | 160 ++++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index a37304b6d..f66888d21 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -4,8 +4,168 @@
 # menu options using "make menuconfig" and by running "make" with appropriate
 # packages enabled.
 
+import re
+
+from checkpackagebase import _CheckFunction
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import ConsecutiveEmptyLines
 from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
 from checkpackagelib import TrailingSpace
+
+
+class Indent(_CheckFunction):
+    COMMENT = re.compile("^\s*#")
+    CONDITIONAL = re.compile("^\s*(ifeq|ifneq|endif)\s")
+    ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
+    END_DEFINE = re.compile("^\s*endef\s")
+    MAKEFILE_TARGET = re.compile("^[^# \t]+:\s")
+    START_DEFINE = re.compile("^\s*define\s")
+
+    def before(self):
+        self.define = False
+        self.backslash = False
+        self.makefile_target = False
+
+    def check_line(self, lineno, text):
+        if self.START_DEFINE.search(text):
+            self.define = True
+            return
+        if self.END_DEFINE.search(text):
+            self.define = False
+            return
+
+        expect_tabs = False
+        if self.define or self.backslash or self.makefile_target:
+            expect_tabs = True
+        if self.CONDITIONAL.search(text):
+            expect_tabs = False
+
+        # calculate for next line
+        if self.ENDS_WITH_BACKSLASH.search(text):
+            self.backslash = True
+        else:
+            self.backslash = False
+
+        if self.MAKEFILE_TARGET.search(text):
+            self.makefile_target = True
+            return
+        if text.strip() == "":
+            self.makefile_target = False
+            return
+
+        # comment can be indented or not inside define ... endef, so ignore it
+        if self.define and self.COMMENT.search(text):
+            return
+
+        if expect_tabs:
+            if not text.startswith("\t"):
+                return ["{}:{}: expected indent with tabs"
+                        .format(self.filename, lineno),
+                        text]
+        else:
+            if text.startswith("\t"):
+                return ["{}:{}: unexpected indent with tabs"
+                        .format(self.filename, lineno),
+                        text]
+
+
+class PackageHeader(_CheckFunction):
+    def before(self):
+        self.skip = False
+
+    def check_line(self, lineno, text):
+        if self.skip or lineno > 6:
+            return
+
+        if lineno in [1, 5]:
+            if lineno == 1 and text.startswith("include "):
+                self.skip = True
+                return
+            if text.rstrip() != "#" * 80:
+                return ["{}:{}: should be 80 hashes ({}#writing-rules-mk)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        text,
+                        "#" * 80]
+        elif lineno in [2, 4]:
+            if text.rstrip() != "#":
+                return ["{}:{}: should be 1 hash ({}#writing-rules-mk)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        text]
+        elif lineno == 6:
+            if text.rstrip() != "":
+                return ["{}:{}: should be a blank line ({}#writing-rules-mk)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        text]
+
+
+class SpaceBeforeBackslash(_CheckFunction):
+    TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH = re.compile(r"^.*(  |\t)\\$")
+
+    def check_line(self, lineno, text):
+        if self.TAB_OR_MULTIPLE_SPACES_BEFORE_BACKSLASH.match(text.rstrip()):
+            return ["{}:{}: use only one space before backslash"
+                    .format(self.filename, lineno),
+                    text]
+
+
+class TrailingBackslash(_CheckFunction):
+    ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
+
+    def before(self):
+        self.backslash = False
+
+    def check_line(self, lineno, text):
+        last_line_ends_in_backslash = self.backslash
+
+        # calculate for next line
+        if self.ENDS_WITH_BACKSLASH.search(text):
+            self.backslash = True
+            self.lastline = text
+            return
+        self.backslash = False
+
+        if last_line_ends_in_backslash and text.strip() == "":
+            return ["{}:{}: remove trailing backslash"
+                    .format(self.filename, lineno - 1),
+                    self.lastline]
+
+
+class UselessFlag(_CheckFunction):
+    DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
+        "_AUTORECONF\s*=\s*NO",
+        "_LIBTOOL_PATCH\s*=\s*YES"])))
+    DEFAULT_GENERIC_FLAG = re.compile("^.*{}".format("|".join([
+        "_INSTALL_IMAGES\s*=\s*NO",
+        "_INSTALL_REDISTRIBUTE\s*=\s*YES",
+        "_INSTALL_STAGING\s*=\s*NO",
+        "_INSTALL_TARGET\s*=\s*YES"])))
+    END_CONDITIONAL = re.compile("^\s*(endif)")
+    START_CONDITIONAL = re.compile("^\s*(ifeq|ifneq)")
+
+    def before(self):
+        self.conditional = 0
+
+    def check_line(self, lineno, text):
+        if self.START_CONDITIONAL.search(text):
+            self.conditional += 1
+            return
+        if self.END_CONDITIONAL.search(text):
+            self.conditional -= 1
+            return
+
+        # allow non-default conditionally overridden by default
+        if self.conditional > 0:
+            return
+
+        if self.DEFAULT_GENERIC_FLAG.search(text):
+            return ["{}:{}: useless default value ({}#"
+                    "_infrastructure_for_packages_with_specific_build_systems)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+
+        if self.DEFAULT_AUTOTOOLS_FLAG.search(text):
+            return ["{}:{}: useless default value "
+                    "({}#_infrastructure_for_autotools_based_packages)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 7/9] docs/manual: size of tab in package description
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (5 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 6/9] check-package: check *.mk files Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 8/9] check-package: check Config.* files Ricardo Martincoski
                     ` (2 subsequent siblings)
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Explicitly state that one tab counts for 8 columns in package
description, leaving 62 characters to the text itself.
Update the text and the example in the two places where the Config.in
format is described.
Also mention a newline is expected between the help text itself and the
upstream URL.

This blob can help developers to understand the expected formatting.
Also, it can be referenced by reviewers.

http://patchwork.ozlabs.org/patch/611289/
http://patchwork.ozlabs.org/patch/606866/
http://patchwork.ozlabs.org/patch/459960/

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Romain Naour <romain.naour@gmail.com>
---
Changes v1 -> v2:
  - add url to past reviews to the commit message (after comments from
    Romain);
---

Notes:
    The next patch in this series generates a warning for each line of help
    text that does not comply to:
    <tab><2 spaces><62 chars>

 docs/manual/adding-packages-directory.txt | 8 +++++---
 docs/manual/writing-rules.txt             | 6 ++++--
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/docs/manual/adding-packages-directory.txt b/docs/manual/adding-packages-directory.txt
index 5dba962bd..08f5d42f9 100644
--- a/docs/manual/adding-packages-directory.txt
+++ b/docs/manual/adding-packages-directory.txt
@@ -28,7 +28,8 @@ contain:
 config BR2_PACKAGE_LIBFOO
 	bool "libfoo"
 	help
-	  This is a comment that explains what libfoo is.
+	  This is a comment that explains what libfoo is. The help text
+	  should be wrapped.
 
 	  http://foosoftware.org/libfoo/
 ---------------------------
@@ -36,8 +37,9 @@ config BR2_PACKAGE_LIBFOO
 The +bool+ line, +help+ line and other metadata information about the
 configuration option must be indented with one tab. The help text
 itself should be indented with one tab and two spaces, lines should
-not be longer than 72 columns, and it must mention the upstream URL
-of the project.
+be wrapped to fit 72 columns, where tab counts for 8, so 62 characters
+in the text itself. The help text must mention the upstream URL of the
+project after an empty line.
 
 As a convention specific to Buildroot, the ordering of the attributes
 is as follows:
diff --git a/docs/manual/writing-rules.txt b/docs/manual/writing-rules.txt
index ec1ddb191..e2ad41ebc 100644
--- a/docs/manual/writing-rules.txt
+++ b/docs/manual/writing-rules.txt
@@ -29,7 +29,8 @@ config BR2_PACKAGE_LIBFOO
 	depends on BR2_PACKAGE_LIBBAZ
 	select BR2_PACKAGE_LIBBAR
 	help
-	  This is a comment that explains what libfoo is.
+	  This is a comment that explains what libfoo is. The help text
+	  should be wrapped.
 
 	  http://foosoftware.org/libfoo/
 ---------------------
@@ -40,7 +41,8 @@ config BR2_PACKAGE_LIBFOO
 * The help text itself should be indented with one tab and two
   spaces.
 
-* The help text should be wrapped to fit 72 columns.
+* The help text should be wrapped to fit 72 columns, where tab counts
+  for 8, so 62 characters in the text itself.
 
 The +Config.in+ files are the input for the configuration tool
 used in Buildroot, which is the regular _Kconfig_. For further
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 8/9] check-package: check Config.* files
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (6 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 7/9] docs/manual: size of tab in package description Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
  2017-04-08 14:21   ` [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style Thomas Petazzoni
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Warn when help text is larger than 72 columns, see [1].
Warn for wrongly indented attributes, see [1].
Warn when the convention of attributes order is not followed, see [2].

[1] http://nightly.buildroot.org/#writing-rules-config-in
[2] http://nightly.buildroot.org/#_config_files

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
---
Changes v1 -> v2:
  - update commit subject since now Config.* are tested;
  - use classes instead of functions to declare each check (Thomas DS);
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m2.196s
    user	0m2.116s
    sys	0m0.044s
    
    AttributesOrder:
     support/scripts/check-package --include-only AttributesOrder \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      369
     (cd support/scripts/check-package-example && \
     ../check-package --include-only AttributesOrder -vv package/*/*)
      package/package1/Config.in:3: attributes order: type, default, depends on, select, help (http://nightly.buildroot.org/#_config_files)
      < tab  >bool "pAcKaGe"
      package/package1/Config.in:6: attributes order: type, default, depends on, select, help (http://nightly.buildroot.org/#_config_files)
      < tab  >depends on BR2_USE_WCHAR
      package/package1/Config.something:6: attributes order: type, default, depends on, select, help (http://nightly.buildroot.org/#_config_files)
      < tab  >bool "Extension 1"
      180 lines processed
      3 warnings generated
    
    HelpText:
     support/scripts/check-package --include-only HelpText \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      987
     (cd support/scripts/check-package-example && \
     ../check-package --include-only HelpText -vv package/*/*)
      package/package1/Config.in:8: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  package1 is a bad stylized package. Its only purpose is to exemplify
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:9: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
                common style mistakes
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:10: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >some more help text but no url
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:23: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  Another paragraph. - But this time we cross the column 72 by 1.
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:24: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  > wrong_line_with_single_word
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      package/package1/Config.in:25: help text: <tab><2 spaces><62 chars> (http://nightly.buildroot.org/#writing-rules-config-in)
      < tab  >  http://www.example.com/ urls do not have spaces and this line is too long.
      < tab  >  123456789 123456789 123456789 123456789 123456789 123456789 12
      180 lines processed
      6 warnings generated
    
    Indent:
     support/scripts/check-package --include-only Indent \
     $(find package -name 'Config.*') 2>/dev/null | wc -l
      501
     (cd support/scripts/check-package-example && \
     ../check-package --include-only Indent -vv package/*/*)
      package/package1/Config.in:13: should not be indented
       config BR2_PACKAGE_PACKAGE1_OPTION
      package/package1/Config.in:14: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              bool "package1 option"
      package/package1/Config.in:15: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              depends on BR2_USE_MMU
      package/package1/Config.in:16: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              select BR2_PACKAGE_GETTEXT if BR2_NEEDS_GETTEXT_IF_LOCALE
      package/package1/Config.in:17: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              help
      package/package1/Config.in:37: continuation line should be indented using tabs
                      if BR2_aarch64
      package/package1/Config.in:44: should be indented with one tab (http://nightly.buildroot.org/#_config_files)
              depends on BR2_USE_MMU
      package/package1/package1.mk:22: unexpected indent with tabs
      < tab  >PACKAGE1_INSTALL_STAGING = NO
      package/package1/package1.mk:27: expected indent with tabs
                             depend3
      package/package1/package1.mk:42: expected indent with tabs
              mkdir -p $(TARGET_DIR)/var/lib
      package/package1/package1.mk:51: expected indent with tabs
              $(PACKAGE1_INSTALL_SOMETHING_ELSE)
      180 lines processed
      11 warnings generated

 support/scripts/checkpackagelib_config.py | 128 ++++++++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/support/scripts/checkpackagelib_config.py b/support/scripts/checkpackagelib_config.py
index ee5981e64..be52d9432 100644
--- a/support/scripts/checkpackagelib_config.py
+++ b/support/scripts/checkpackagelib_config.py
@@ -3,8 +3,136 @@
 # "bool", so below check functions don't need to check for things already
 # checked by running "make menuconfig".
 
+import re
+
+from checkpackagebase import _CheckFunction
 # Notice: ignore 'imported but unused' from pyflakes for check functions.
 from checkpackagelib import ConsecutiveEmptyLines
 from checkpackagelib import EmptyLastLine
 from checkpackagelib import NewlineAtEof
 from checkpackagelib import TrailingSpace
+
+
+def _empty_or_comment(text):
+    line = text.strip()
+    # ignore empty lines and comment lines indented or not
+    return line == "" or line.startswith("#")
+
+
+def _part_of_help_text(text):
+    return text.startswith("\t  ")
+
+
+# used in more than one check
+entries_that_should_not_be_indented = [
+    "choice", "comment", "config", "endchoice", "endif", "endmenu", "if",
+    "menu", "menuconfig", "source"]
+
+
+class AttributesOrder(_CheckFunction):
+    attributes_order_convention = {
+        "bool": 1, "prompt": 1, "string": 1, "default": 2, "depends": 3,
+        "select": 4, "help": 5}
+
+    def before(self):
+        self.state = 0
+
+    def check_line(self, lineno, text):
+        if _empty_or_comment(text) or _part_of_help_text(text):
+            return
+
+        attribute = text.split()[0]
+
+        if attribute in entries_that_should_not_be_indented:
+            self.state = 0
+            return
+        if attribute not in self.attributes_order_convention.keys():
+            return
+        new_state = self.attributes_order_convention[attribute]
+        wrong_order = self.state > new_state
+
+        # save to process next line
+        self.state = new_state
+
+        if wrong_order:
+            return ["{}:{}: attributes order: type, default, depends on,"
+                    " select, help ({}#_config_files)"
+                    .format(self.filename, lineno, self.url_to_manual),
+                    text]
+
+
+class HelpText(_CheckFunction):
+    HELP_TEXT_FORMAT = re.compile("^\t  .{,62}$")
+    URL_ONLY = re.compile("^(http|https|git)://\S*$")
+
+    def before(self):
+        self.help_text = False
+
+    def check_line(self, lineno, text):
+        if _empty_or_comment(text):
+            return
+
+        entry = text.split()[0]
+
+        if entry in entries_that_should_not_be_indented:
+            self.help_text = False
+            return
+        if text.strip() == "help":
+            self.help_text = True
+            return
+
+        if not self.help_text:
+            return
+
+        if self.HELP_TEXT_FORMAT.match(text.rstrip()):
+            return
+        if self.URL_ONLY.match(text.strip()):
+            return
+        return ["{}:{}: help text: <tab><2 spaces><62 chars>"
+                " ({}#writing-rules-config-in)"
+                .format(self.filename, lineno, self.url_to_manual),
+                text,
+                "\t  " + "123456789 " * 6 + "12"]
+
+
+class Indent(_CheckFunction):
+    ENDS_WITH_BACKSLASH = re.compile(r"^[^#].*\\$")
+    entries_that_should_be_indented = [
+        "bool", "default", "depends", "help", "prompt", "select", "string"]
+
+    def before(self):
+        self.backslash = False
+
+    def check_line(self, lineno, text):
+        if _empty_or_comment(text) or _part_of_help_text(text):
+            self.backslash = False
+            return
+
+        entry = text.split()[0]
+
+        last_line_ends_in_backslash = self.backslash
+
+        # calculate for next line
+        if self.ENDS_WITH_BACKSLASH.search(text):
+            self.backslash = True
+        else:
+            self.backslash = False
+
+        if last_line_ends_in_backslash:
+            if text.startswith("\t"):
+                return
+            return ["{}:{}: continuation line should be indented using tabs"
+                    .format(self.filename, lineno),
+                    text]
+
+        if entry in self.entries_that_should_be_indented:
+            if not text.startswith("\t{}".format(entry)):
+                return ["{}:{}: should be indented with one tab"
+                        " ({}#_config_files)"
+                        .format(self.filename, lineno, self.url_to_manual),
+                        text]
+        elif entry in entries_that_should_not_be_indented:
+            if not text.startswith(entry):
+                return ["{}:{}: should not be indented"
+                        .format(self.filename, lineno),
+                        text]
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 9/9] check-package: check *.mk for typo in variable
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (7 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 8/9] check-package: check Config.* files Ricardo Martincoski
@ 2017-02-19 22:17   ` Ricardo Martincoski
  2017-04-08 14:21   ` [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style Thomas Petazzoni
  9 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 22:17 UTC (permalink / raw)
  To: buildroot

Warn when a variable is defined in a .mk file and it don't start with
the package name.

This function generates false warnings and the maintenance of the
whitelist can be an extra burden, but it catches some typos really hard
to see:
- POPLER_CONF_OPTS [1]
- BALELD_LICENSE [2]
- DRDB_UTILS_DEPENDENCIES [3]
- PERL_LIBWWW_LICENSE_FILES [4]
- AVRDUDR_LICENSE_FILES [5]
- GST1_PLUGINS_ULGY_HAS_GPL_LICENSE [6]
- ON2_8170_LICENSE [7]
- LIBFDTI_CONF_OPTS [8][9]
- IPSEC_DEPENDENCIES [10]

[1] http://patchwork.ozlabs.org/patch/681533
[2] http://patchwork.ozlabs.org/patch/643293
[3] http://patchwork.ozlabs.org/patch/449589
[4] http://patchwork.ozlabs.org/patch/464545
[5] http://patchwork.ozlabs.org/patch/305060
[6] http://patchwork.ozlabs.org/patch/253089
[7] http://patchwork.ozlabs.org/patch/250523
[8] http://patchwork.ozlabs.org/patch/394125
[9] https://github.com/buildroot/buildroot/commit/fe7a4b524b72bcb448f7e723873d8244620cb2f1
[10] https://github.com/buildroot/buildroot/commit/dff1d590b2a0fadf58b6eed60029b2ecbab7c710

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Romain Naour <romain.naour@gmail.com>
---
Changes v1 -> v2:
  - avoid false positive for variable of virtual package the package
    under analysis provides (e.g. MYSQL_SOCKET) (Romain);
  - use classes instead of functions to declare each check (Thomas DS);
  - I kept the examples of typos it catches, but Peter Korsgaard already
    fixed the typos listed above;
---

Notes:
    $ time support/scripts/check-package $(find package -type f) >/dev/null 2>/dev/null
    
    real	0m3.205s
    user	0m3.140s
    sys	0m0.064s
    
    TypoInPackageVariable:
     support/scripts/check-package --include-only TypoInPackageVariable \
     $(find package -name '*.mk') 2>/dev/null | wc -l
      3
     (cd support/scripts/check-package-example && \
     ../check-package --include-only TypoInPackageVariable -vv package/*/*)
      package/package1/package1.mk:33: possible typo: LINUX_DEPENDENCIES -> *PACKAGE1*
      LINUX_DEPENDENCIES = messing with others
      package/package1/package1.mk:34: possible typo: PACKACE1_DEPENDENCIES -> *PACKAGE1*
      PACKACE1_DEPENDENCIES = typo
      package/package1/package1.mk:39: possible typo: NOT_PROVIDED4_LOCK -> *PACKAGE1*
      NOT_PROVIDED4_LOCK = lock4
      180 lines processed
      3 warnings generated

 support/scripts/checkpackagelib_mk.py | 52 +++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/support/scripts/checkpackagelib_mk.py b/support/scripts/checkpackagelib_mk.py
index f66888d21..0740a37f7 100644
--- a/support/scripts/checkpackagelib_mk.py
+++ b/support/scripts/checkpackagelib_mk.py
@@ -131,6 +131,58 @@ class TrailingBackslash(_CheckFunction):
                     self.lastline]
 
 
+class TypoInPackageVariable(_CheckFunction):
+    ALLOWED = re.compile("|".join([
+        "ACLOCAL_DIR",
+        "ACLOCAL_HOST_DIR",
+        "BR_CCACHE_INITIAL_SETUP",
+        "BR_NO_CHECK_HASH_FOR",
+        "LINUX_POST_PATCH_HOOKS",
+        "LINUX_TOOLS",
+        "LUA_RUN",
+        "MKFS_JFFS2",
+        "MKIMAGE_ARCH",
+        "PKG_CONFIG_HOST_BINARY",
+        "TARGET_FINALIZE_HOOKS",
+        "XTENSA_CORE_NAME"]))
+    PACKAGE_NAME = re.compile("/([^/]+)\.mk")
+    VARIABLE = re.compile("^([A-Z0-9_]+_[A-Z0-9_]+)\s*(\+|)=")
+
+    def before(self):
+        package = self.PACKAGE_NAME.search(self.filename).group(1)
+        package = package.replace("-", "_").upper()
+        # linux tools do not use LINUX_TOOL_ prefix for variables
+        package = package.replace("LINUX_TOOL_", "")
+        self.package = package
+        self.REGEX = re.compile("^(HOST_)?({}_[A-Z0-9_]+)".format(package))
+        self.FIND_VIRTUAL = re.compile(
+            "^{}_PROVIDES\s*(\+|)=\s*(.*)".format(package))
+        self.virtual = []
+
+    def check_line(self, lineno, text):
+        m = self.VARIABLE.search(text)
+        if m is None:
+            return
+
+        variable = m.group(1)
+
+        # allow to set variables for virtual package this package provides
+        v = self.FIND_VIRTUAL.search(text)
+        if v:
+            self.virtual += v.group(2).upper().split()
+            return
+        for virtual in self.virtual:
+            if variable.startswith("{}_".format(virtual)):
+                return
+
+        if self.ALLOWED.match(variable):
+            return
+        if self.REGEX.search(text) is None:
+            return ["{}:{}: possible typo: {} -> *{}*"
+                    .format(self.filename, lineno, variable, self.package),
+                    text]
+
+
 class UselessFlag(_CheckFunction):
     DEFAULT_AUTOTOOLS_FLAG = re.compile("^.*{}".format("|".join([
         "_AUTORECONF\s*=\s*NO",
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 2/9] support/scripts/check-package: new script
  2017-01-24 21:14   ` Thomas De Schampheleire
  2017-02-06 18:53     ` Thomas De Schampheleire
@ 2017-02-19 23:13     ` Ricardo Martincoski
  1 sibling, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 23:13 UTC (permalink / raw)
  To: buildroot

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

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 4/9] check-package: check *.hash files
  2017-01-24 21:18   ` Thomas De Schampheleire
@ 2017-02-19 23:16     ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 23:16 UTC (permalink / raw)
  To: buildroot

Thomas,

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

[snip]
>> +FILENAME_WITH_SLASH = re.compile("/")
[snip]
>> +    if FILENAME_WITH_SLASH.search(filename):
> 
> This is overkill. It is more simple and I expect more performant to just do:
> 
>     if '/' in filename:

Fixed in v2.

Regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH 5/9] check-package: check *.patch files
  2017-02-07  9:58     ` Thomas Petazzoni
@ 2017-02-19 23:41       ` Ricardo Martincoski
  0 siblings, 0 replies; 41+ messages in thread
From: Ricardo Martincoski @ 2017-02-19 23:41 UTC (permalink / raw)
  To: buildroot

Thomas De Schampheleire,
Thomas Petazzoni,

On Tue, Feb 07, 2017 at 07:58 AM, Thomas Petazzoni wrote:

> On Tue, 24 Jan 2017 22:21:31 +0100, Thomas De Schampheleire wrote:
> 
>> > +NUMBERED_PATCH = re.compile("Subject:\s*\[PATCH\s*\d+/\d+\]")  
>> 
>> Is this really a requirement? This numbering in the patch header is
>> unrelated to the number used in the filename. One can expect many
>> 'PATCH 1/1' in the same package directory. I would leave out this
>> test.
> 
> We normally require patches in package/<foo>/ to be generated with "git
> format-patch -N", so that their prefix is [PATCH] and not [PATCH x/y].
> 
> However, we do not require all patches to be Git formatted. It's kind
> of required when the upstream project uses Git as its version control
> system. But when the upstream project uses some other version control
> system, other patch formats are excepted, like hand-generated patches
> with "diff", or patches generated with "quilt". Not everyone wants to
> find with Mercurial to find out to generate a patch with this damn
> thing :-)

I fixed the code to generate a warning only when the patch was generated
using git (by testing if any line starts with 'diff --git').

I let to you to decide to keep or remove it.
Removing this check NumberedSubject when applying should be easy in v2.

Regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
                     ` (8 preceding siblings ...)
  2017-02-19 22:17   ` [Buildroot] [PATCH v2 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
@ 2017-04-08 14:21   ` Thomas Petazzoni
  2017-04-11 23:03     ` Ricardo Martincoski
  9 siblings, 1 reply; 41+ messages in thread
From: Thomas Petazzoni @ 2017-04-08 14:21 UTC (permalink / raw)
  To: buildroot

Hello,

On Sun, 19 Feb 2017 19:17:15 -0300, Ricardo Martincoski wrote:

> Ricardo Martincoski (9):
>   support/scripts/check-package: example
>   support/scripts/check-package: new script
>   check-package: check whitespace and empty lines
>   check-package: check *.hash files
>   check-package: check *.patch files
>   check-package: check *.mk files
>   docs/manual: size of tab in package description
>   check-package: check Config.* files
>   check-package: check *.mk for typo in variable

Since there was no feedback to your patch series since 1.5 months, I've
applied all of it, except PATCH 1/9. I'm not sure we want to have the
bad examples/test cases inside the Buildroot tree.

However, there are two things that bothered me a little bit:

 - The number of files added in support/scripts/. Should we have
   support/scripts/check-package as a script, and the rest in a
   subdirectory?

 - Between every function/method/class, you put two empty lines. The
   Buildroot coding style is generally to have only one empty line.

I haven't reviewed the details of the Python script for this version, I
briefly looked at it on the v1. There are possibly some improvements to
be made, but they can be done as followup patches. The tool is already
very useful as it is today, so I don't want to keep it longer out of
the tree.

Thanks a lot for this very useful contribution!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2017-04-08 14:21   ` [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style Thomas Petazzoni
@ 2017-04-11 23:03     ` Ricardo Martincoski
  2017-04-12  7:49       ` Thomas Petazzoni
  0 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2017-04-11 23:03 UTC (permalink / raw)
  To: buildroot

Thomas,

On Sat, Apr 08, 2017 at 11:21 AM, Thomas Petazzoni wrote:

> Since there was no feedback to your patch series since 1.5 months, I've
> applied all of it, except PATCH 1/9. I'm not sure we want to have the

Thanks.

> bad examples/test cases inside the Buildroot tree.

OK.

> However, there are two things that bothered me a little bit:
> 
>  - The number of files added in support/scripts/. Should we have
>    support/scripts/check-package as a script, and the rest in a
>    subdirectory?

Sure. I will send a followup patch.

I am inclined to change to this:
 support/scripts/
 ...
 |-- check-package
 |-- checkpackage
 |   |-- __init__.py
 |   |-- base.py
 |   |-- lib.py
 |   |-- lib_config.py
 |   |-- lib_hash.py
 |   |-- lib_mk.py
 |   |-- lib_patch.py
 |   `-- readme.txt
 ...

>  - Between every function/method/class, you put two empty lines. The
>    Buildroot coding style is generally to have only one empty line.

Sorry, the series got contaminated by my habit of using pep8 with default
options.

I will fix that in another followup patch.

Regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2017-04-11 23:03     ` Ricardo Martincoski
@ 2017-04-12  7:49       ` Thomas Petazzoni
  2017-04-13  3:03         ` Ricardo Martincoski
  0 siblings, 1 reply; 41+ messages in thread
From: Thomas Petazzoni @ 2017-04-12  7:49 UTC (permalink / raw)
  To: buildroot

Hello,

On Tue, 11 Apr 2017 20:03:06 -0300, Ricardo Martincoski wrote:

> > However, there are two things that bothered me a little bit:
> > 
> >  - The number of files added in support/scripts/. Should we have
> >    support/scripts/check-package as a script, and the rest in a
> >    subdirectory?  
> 
> Sure. I will send a followup patch.
> 
> I am inclined to change to this:
>  support/scripts/
>  ...
>  |-- check-package
>  |-- checkpackage
>  |   |-- __init__.py
>  |   |-- base.py
>  |   |-- lib.py
>  |   |-- lib_config.py
>  |   |-- lib_hash.py
>  |   |-- lib_mk.py
>  |   |-- lib_patch.py
>  |   `-- readme.txt
>  ...

Looks good to me. I'm wondering what is best between checkpackage and
check-packagelib for the folder, but I don't have a strong opinion here.

> >  - Between every function/method/class, you put two empty lines. The
> >    Buildroot coding style is generally to have only one empty line.  
> 
> Sorry, the series got contaminated by my habit of using pep8 with default
> options.

Ah, but then if PEP8 suggests this, perhaps we should follow it? It's
just that we don't do this anywhere else in Buildroot. Anyway, that's
really a minor detail.

Thanks again for this work!

Have you seen that http://autobuild.buildroot.net/stats/ has a new
column "Warnings", which indicates for each package the number of
check-package warnings reported?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2017-04-12  7:49       ` Thomas Petazzoni
@ 2017-04-13  3:03         ` Ricardo Martincoski
  2017-04-13  7:20           ` Thomas Petazzoni
  0 siblings, 1 reply; 41+ messages in thread
From: Ricardo Martincoski @ 2017-04-13  3:03 UTC (permalink / raw)
  To: buildroot

Thomas,

On Wed, Apr 12, 2017 at 04:49 AM, Thomas Petazzoni wrote:

> On Tue, 11 Apr 2017 20:03:06 -0300, Ricardo Martincoski wrote:
> 
>> I am inclined to change to this:
>>  support/scripts/
>>  ...
>>  |-- check-package
>>  |-- checkpackage
>>  |   |-- __init__.py
>>  |   |-- base.py
>>  |   |-- lib.py
>>  |   |-- lib_config.py
>>  |   |-- lib_hash.py
>>  |   |-- lib_mk.py
>>  |   |-- lib_patch.py
>>  |   `-- readme.txt
>>  ...
> 
> Looks good to me. I'm wondering what is best between checkpackage and
> check-packagelib for the folder, but I don't have a strong opinion here.

Maybe checkpackagelib?

The dash needs a bit more code:

import importlib
lib_config = importlib.import_module(".lib_config", "check-packagelib")

instead of

import checkpackagelib.lib_config

Also PEP 8 [1] recommends "[a-z]*" for folders (packages) and "[a-z_]*" for
files (modules).

>> >  - Between every function/method/class, you put two empty lines. The
>> >    Buildroot coding style is generally to have only one empty line.  
>> 
>> Sorry, the series got contaminated by my habit of using pep8 with default
>> options.
> 
> Ah, but then if PEP8 suggests this, perhaps we should follow it? It's
> just that we don't do this anywhere else in Buildroot. Anyway, that's
> really a minor detail.

Indeed a minor detail.
+1 on this, either the PEP 8 recommendation [2] or the pep8 tool [3].

My *personal* reasoning for using both is:
"Some people that wrote much more Python code than me already thought and
discussed about this to came up with this recommendation"
"I am lazy so I use the tool so I don't need to read the recommendation too
often"


In the long run, it would be good to have opinions from others.
How do you prefer handling this?
A) you guys discuss on IRC;
B) new thread on the mailing list;
C) add topic to next dev meeting;

Maybe here is a good starting point (authors and reviewers for past year):

find support/ -type f | xargs file | grep Python | sed -e 's,:.*,,g' \
 | tee /tmp/python-files
git shortlog -s 2016.05.. -- $(cat /tmp/python-files) \
 | sed -e 's,^\s*[0-9]*\s*,,g' | sort -u | tee /tmp/python-authors
git log 2016.05.. -- $(cat /tmp/python-files) \
 | grep -Ei '(reviewed|acked)-by:' | sed 's,.*[bB]y: ,,' | sort -u \
 | sed -e 's,\s[<(].*,,g' -e 's,",,g' | tee /tmp/python-reviewers
grep -f /tmp/python-reviewers -f /tmp/python-authors DEVELOPERS

> Have you seen that http://autobuild.buildroot.net/stats/ has a new
> column "Warnings", which indicates for each package the number of
> check-package warnings reported?

Nice! Thank you for this.

[1] https://www.python.org/dev/peps/pep-0008/#package-and-module-names
[2] https://www.python.org/dev/peps/pep-0008
[3] https://pypi.python.org/pypi/pep8

Regards,
Ricardo

^ permalink raw reply	[flat|nested] 41+ messages in thread

* [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style
  2017-04-13  3:03         ` Ricardo Martincoski
@ 2017-04-13  7:20           ` Thomas Petazzoni
  0 siblings, 0 replies; 41+ messages in thread
From: Thomas Petazzoni @ 2017-04-13  7:20 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 13 Apr 2017 00:03:25 -0300, Ricardo Martincoski wrote:

> > Looks good to me. I'm wondering what is best between checkpackage and
> > check-packagelib for the folder, but I don't have a strong opinion here.  
> 
> Maybe checkpackagelib?

Looks good to me.

> Indeed a minor detail.
> +1 on this, either the PEP 8 recommendation [2] or the pep8 tool [3].
> 
> My *personal* reasoning for using both is:
> "Some people that wrote much more Python code than me already thought and
> discussed about this to came up with this recommendation"
> "I am lazy so I use the tool so I don't need to read the recommendation too
> often"
> 
> 
> In the long run, it would be good to have opinions from others.
> How do you prefer handling this?
> A) you guys discuss on IRC;
> B) new thread on the mailing list;
> C) add topic to next dev meeting;

A new thread on the mailing list would be fine.

I think the most active Python-ist folks are Samuel Martin, Maxime
Hadjinlian and Thomas De Schampheleire. Make sure to include them in
the discussion.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 41+ messages in thread

end of thread, other threads:[~2017-04-13  7:20 UTC | newest]

Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-31  3:21 [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 1/9] support/scripts/check-package: example Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 2/9] support/scripts/check-package: new script Ricardo Martincoski
2017-01-24 21:14   ` Thomas De Schampheleire
2017-02-06 18:53     ` Thomas De Schampheleire
2017-02-07  0:17       ` Ricardo Martincoski
2017-02-19 23:13     ` Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 4/9] check-package: check *.hash files Ricardo Martincoski
2017-01-24 21:18   ` Thomas De Schampheleire
2017-02-19 23:16     ` Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 5/9] check-package: check *.patch files Ricardo Martincoski
2017-01-24 21:21   ` Thomas De Schampheleire
2017-02-07  9:58     ` Thomas Petazzoni
2017-02-19 23:41       ` Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 6/9] check-package: check *.mk files Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 7/9] docs/manual: size of tab in package description Ricardo Martincoski
2017-01-21 16:58   ` Romain Naour
2017-02-07  1:10     ` Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 8/9] check-package: check Config.in.* files Ricardo Martincoski
2016-12-31  3:21 ` [Buildroot] [PATCH 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
2017-01-21 17:19   ` Romain Naour
2017-02-07  0:33     ` Ricardo Martincoski
2017-02-07  9:03   ` Peter Korsgaard
2017-01-21 17:56 ` [Buildroot] [PATCH 0/9] A checkpackage script that verifies a package coding style Romain Naour
2017-02-07  0:52   ` Ricardo Martincoski
2017-02-19 22:17 ` [Buildroot] [PATCH v2 " Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 1/9] support/scripts/check-package: example Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 2/9] support/scripts/check-package: new script Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 3/9] check-package: check whitespace and empty lines Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 4/9] check-package: check *.hash files Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 5/9] check-package: check *.patch files Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 6/9] check-package: check *.mk files Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 7/9] docs/manual: size of tab in package description Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 8/9] check-package: check Config.* files Ricardo Martincoski
2017-02-19 22:17   ` [Buildroot] [PATCH v2 9/9] check-package: check *.mk for typo in variable Ricardo Martincoski
2017-04-08 14:21   ` [Buildroot] [PATCH v2 0/9] A checkpackage script that verifies a package coding style Thomas Petazzoni
2017-04-11 23:03     ` Ricardo Martincoski
2017-04-12  7:49       ` Thomas Petazzoni
2017-04-13  3:03         ` Ricardo Martincoski
2017-04-13  7:20           ` Thomas Petazzoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.