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