[Buildroot] [PATCH 0/5] Fix bug 11271 and add test for check-package

[Buildroot] [PATCH 0/5] Fix bug 11271 and add test for check-package

[Ricardo Martincoski]

Hello,

This series fixes bug 11271 and adds a test case that catches the bug.

Patch 1 fixes the usage of check-package when called from the same directory
than the files to be tested in a br2-external tree.

Patch 2 improve the testing infra by ensuring it can be started from any path,
not only the Buildroot top directory.
As a consequence the test infra will always test the repo it belongs to.

Patch 3 creates as test case for check-package, calling the script in the same
repo against in-tree files and against files in a br2-external.

Patch 4 improves usability of check-package in a br2-external tree by ignoring
external.mk since it is special: it usually contains raw makefile targets and
therefore cannot be tested by the script, but it is part of the br2-external
structure, so it is likely someone expects it to be checkable by an in-tree
script.

Patch 5 improves the test case created by patch 3 to test the scenario covered
by patch 4.

On below tests I used another series just to make my life easier:
http://patchwork.ozlabs.org/project/buildroot/list/?series=72927

1) to check patch 2 works and don't bring regressions:

A full run with only patch 2 applied:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290055

Another full run with only patch 2 applied, this time cd'ing to /tmp and
starting the tests from there:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290166

Without any patches of this series, cd'ing to /tmp and starting the tests from
there:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290220

2) to check patch 3 tests what patch 1 fixes:

With only patches 2 and 3 applied: failed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116015533

With patches 1, 2 and 3 applied: passed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116015627

3) to check patch 5 tests what patch 4 fixes:

With only patches 1, 2, 3 and 5 applied: failed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116015767

With all patches applied: passed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116015909

Regards,
Ricardo

Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>

Ricardo Martincoski (5):
  check-package: fix check of file in current dir with -b
  support/testing: allow run-tests to be called from anywhere
  support/testing: add test for check-package
  check-package: ignore external.mk
  support/testing: test check-package ignores external.mk

 .gitlab-ci.yml                                |   1 +
 support/testing/infra/__init__.py             |   8 +-
 support/testing/infra/builder.py              |   2 +-
 support/testing/tests/utils/__init__.py       |   0
 .../tests/utils/br2-external/Config.in        |   1 +
 .../tests/utils/br2-external/external.desc    |   1 +
 .../tests/utils/br2-external/external.mk      |   2 +
 .../br2-external/package/external/external.mk |   1 +
 .../testing/tests/utils/test_check_package.py | 171 ++++++++++++++++++
 utils/check-package                           |   5 +-
 10 files changed, 189 insertions(+), 3 deletions(-)
 create mode 100644 support/testing/tests/utils/__init__.py
 create mode 100644 support/testing/tests/utils/br2-external/Config.in
 create mode 100644 support/testing/tests/utils/br2-external/external.desc
 create mode 100644 support/testing/tests/utils/br2-external/external.mk
 create mode 100644 support/testing/tests/utils/br2-external/package/external/external.mk
 create mode 100644 support/testing/tests/utils/test_check_package.py

-- 
2.17.1

[Buildroot] [PATCH 1/5] check-package: fix check of file in current dir with -b

[Ricardo Martincoski]

One of the possible usages of check-package is to first cd to the
directory that contains the files to test (e.g. a package directory) and
then call the script passing the files in the current dir.
It already works when used for intree files, but for files in a
br2-external it throws an exception because some check functions (from
utils/checkpackagelib/lib_*.py) do need the name of the file being
processed and assume there will be a slash before the name.

Notice RemoveDefaultPackageSourceVariable and TypoInPackageVariable lead
to an exception in this case, but ApplyOrder instead generates a false
warning.

Instead of fixing any check function that could lead to different
behaviour when the script is called with -b for a file in the current
directory, always use the absolute path of the file being checked when
-b was passed. A small drawback is that the warning messages will occupy
more columns in this case, but this approach is more future-proof as new
check functions may also need to depend on the name of the file being
processed.

Fixes bug #11271.

Reported-by: Vitaliy Lotorev <lotorev@gmail.com>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 utils/check-package | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/utils/check-package b/utils/check-package
index 3dbc28b0a2..aa6993dabd 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -148,7 +148,7 @@ def __main__():
         # move current dir so the script find the files
         os.chdir(base_dir)
     else:
-        files_to_check = flags.files
+        files_to_check = [os.path.abspath(f) for f in flags.files]
 
     if len(files_to_check) == 0:
         print("No files to check style")
-- 
2.17.1

[Buildroot] [PATCH 2/5] support/testing: allow run-tests to be called from anywhere

[Ricardo Martincoski]

Currently run-tests must be called from the Buildroot top directory.

Derive the top directory from the script path, so run-tests can be called from
any path.
As a consequence the test infra will always test the repo it belongs to.

Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>
---
The suggestion happened one year ago, but I just remembered it now that
I need the base dir to be used by a test case.
Search for "derive the top" in
http://patchwork.ozlabs.org/patch/814544/
---
 support/testing/infra/__init__.py | 8 +++++++-
 support/testing/infra/builder.py  | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/support/testing/infra/__init__.py b/support/testing/infra/__init__.py
index e229e90852..1d4d18bbe9 100644
--- a/support/testing/infra/__init__.py
+++ b/support/testing/infra/__init__.py
@@ -6,6 +6,7 @@ import subprocess
 from urllib2 import urlopen, HTTPError, URLError
 
 ARTIFACTS_URL = "http://autobuild.buildroot.net/artefacts/"
+BASE_DIR = os.path.realpath(os.path.join(os.path.dirname(__file__), "../../.."))
 
 
 def open_log_file(builddir, stage, logtofile=True):
@@ -21,8 +22,13 @@ def open_log_file(builddir, stage, logtofile=True):
     return fhandle
 
 
+def basepath(relpath=""):
+    """Return the absolute path for a file or directory relative to the Buildroot top directory."""
+    return os.path.join(BASE_DIR, relpath)
+
+
 def filepath(relpath):
-    return os.path.join(os.getcwd(), "support/testing", relpath)
+    return os.path.join(BASE_DIR, "support/testing", relpath)
 
 
 def download(dldir, filename):
diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
index fc318fe26e..26fa6be7ae 100644
--- a/support/testing/infra/builder.py
+++ b/support/testing/infra/builder.py
@@ -29,7 +29,7 @@ class Builder(object):
                "O={}".format(self.builddir),
                "olddefconfig"]
         ret = subprocess.call(cmd, stdout=self.logfile, stderr=self.logfile,
-                              env=env)
+                              cwd=infra.basepath(), env=env)
         if ret != 0:
             raise SystemError("Cannot olddefconfig")
 
-- 
2.17.1

[Buildroot] [PATCH 3/5] support/testing: add test for check-package

[Ricardo Martincoski]

Check the basic usage for check-package.

It can be called using either absolute path, relative path or from PATH.
Files to be checked can be passed with either absolute path or relative
path (also including files in the current directory).

Also check it ignores some special files when checking intree files,
i.e. package/pkg-generic.mk, while still generating warnings for out-of-tree
files when called with -b.
In order to allow the later, add an empty line to the Config.in in the
br2-external being tested so the script does generate a warning.

Catches bug #11271.

More tests can be added later, for example compatibility to Python 3.

Suggested-by: Arnout Vandecappelle <arnout@mind.be>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
NOTE: please don't remove the empty line on
support/testing/tests/utils/br2-external/Config.in
as it was intended.
---
 .gitlab-ci.yml                                |   1 +
 support/testing/tests/utils/__init__.py       |   0
 .../tests/utils/br2-external/Config.in        |   1 +
 .../tests/utils/br2-external/external.desc    |   1 +
 .../tests/utils/br2-external/external.mk      |   0
 .../testing/tests/utils/test_check_package.py | 148 ++++++++++++++++++
 6 files changed, 151 insertions(+)
 create mode 100644 support/testing/tests/utils/__init__.py
 create mode 100644 support/testing/tests/utils/br2-external/Config.in
 create mode 100644 support/testing/tests/utils/br2-external/external.desc
 create mode 100644 support/testing/tests/utils/br2-external/external.mk
 create mode 100644 support/testing/tests/utils/test_check_package.py

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a5fbdcb0d8..0eeabbdfcf 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -336,3 +336,4 @@ tests.toolchain.test_external.TestExternalToolchainLinaroArm: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv4: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv5: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv7: *runtime_test
+tests.utils.test_check_package.TestCheckPackageBasicUsage: *runtime_test
diff --git a/support/testing/tests/utils/__init__.py b/support/testing/tests/utils/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/support/testing/tests/utils/br2-external/Config.in b/support/testing/tests/utils/br2-external/Config.in
new file mode 100644
index 0000000000..8b13789179
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/Config.in
@@ -0,0 +1 @@
+
diff --git a/support/testing/tests/utils/br2-external/external.desc b/support/testing/tests/utils/br2-external/external.desc
new file mode 100644
index 0000000000..e89c3560ab
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/external.desc
@@ -0,0 +1 @@
+name: CHECK_PACKAGE
diff --git a/support/testing/tests/utils/br2-external/external.mk b/support/testing/tests/utils/br2-external/external.mk
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/support/testing/tests/utils/test_check_package.py b/support/testing/tests/utils/test_check_package.py
new file mode 100644
index 0000000000..85f2e280a8
--- /dev/null
+++ b/support/testing/tests/utils/test_check_package.py
@@ -0,0 +1,148 @@
+"""Test cases for utils/check-package.
+
+It does not inherit from infra.basetest.BRTest and therefore does not generate a logfile.
+Only when the tests fail there will be output to the console.
+The make target ('make check-package') is already used by the job 'check-package' and won't be tested here.
+"""
+import os
+import subprocess
+import unittest
+
+import infra
+
+
+def call_script(args, env, cwd):
+    """Call a script and return stdout and stderr as lists."""
+    out, err = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, cwd=cwd).communicate()
+    return out.splitlines(), err.splitlines()
+
+
+class TestCheckPackage(unittest.TestCase):
+    """Base class for all test cases for check-package.
+
+    It inherits from TestCase so its methods can use unittest assertions but it does not declare a test_run method in order to be
+    ignored by nose2 when the test cases are discovered.
+    """
+
+    WITH_EMPTY_PATH = {}
+    WITH_UTILS_IN_PATH = {"PATH": infra.basepath("utils") + ":" + os.environ["PATH"]}
+    relative = [
+        # base_script           base_file               rel_script               rel_file                rel_cwd
+        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "package/atop/atop.mk", ""],
+        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "./package/atop/atop.mk", ""],
+        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "atop.mk", "package/atop"],
+        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "./atop.mk", "package/atop"],
+        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "atop/atop.mk", "package"],
+        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "./atop/atop.mk", "package"],
+        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "package/atop/Config.in", ""],
+        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "./package/atop/Config.in", ""],
+        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "Config.in", "package/atop"],
+        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "./Config.in", "package/atop"],
+        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "atop/Config.in", "package"],
+        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "./atop/Config.in", "package"]]
+
+    def assert_file_was_processed(self, stderr):
+        """Infer from check-package stderr if at least one file was processed and fail otherwise."""
+        self.assertIn("lines processed", stderr[0], stderr)
+        processed = int(stderr[0].split()[0])
+        self.assertGreater(processed, 0)
+
+    def assert_file_was_ignored(self, stderr):
+        """Infer from check-package stderr if no file was processed and fail otherwise."""
+        self.assertIn("lines processed", stderr[0], stderr)
+        processed = int(stderr[0].split()[0])
+        self.assertEqual(processed, 0)
+
+    def assert_warnings_generated_for_file(self, stderr):
+        """Infer from check-package stderr if at least one warning was generatd and fail otherwise."""
+        self.assertIn("warnings generated", stderr[1], stderr)
+        generated = int(stderr[1].split()[0])
+        self.assertGreater(generated, 0)
+
+
+class TestCheckPackageBasicUsage(TestCheckPackage):
+    """Test the various ways the script can be called.
+
+    The script can be called either using relative path, absolute path or from PATH.
+    The files to be checked can be passed as arguments using either relative path or absolute path.
+    When in in-tree mode (without -b) some in-tree files and also all out-of-tree files are ignored.
+    When in out-tree mode (with -b) the script does generate warnings.
+    """
+
+    def test_run(self):
+        """Test the various ways the script can be called in a simple top to bottom sequence."""
+        # an intree file can be checked by the script called from relative path, absolute path and from PATH
+        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
+            abs_script = infra.basepath(base_script)
+            abs_file = infra.basepath(base_file)
+            cwd = infra.basepath(rel_cwd)
+
+            _, m = call_script([rel_script, rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([rel_script, abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+        # some intree files are ignored
+        _, m = call_script(["./utils/check-package", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["./utils/check-package", "-b", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+
+        # an out-of-tree file can be checked by the script called from relative path, absolute path and from PATH
+        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
+            abs_script = infra.basepath(base_script)
+            abs_file = infra.basepath(base_file)
+            cwd = infra.basepath(rel_cwd)
+
+            _, m = call_script([rel_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([rel_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+        # out-of-tree files are are ignored without -b but can generate warnings with -b
+        abs_path = infra.filepath("tests/utils/br2-external")
+        rel_file = "Config.in"
+        abs_file = os.path.join(abs_path, rel_file)
+
+        _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        w, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
+
+        w, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
-- 
2.17.1

[Buildroot] [PATCH 4/5] check-package: ignore external.mk

[Ricardo Martincoski]

The external.mk file in a br2-external usually contains raw makefile
targets. This file is common code and not a package recipe so it should
not be tested against the code-style of a package .mk file.

When using this script to check files in a br2-external tree, usually
the user is responsible for not passing files that check-package do not
understand. But external.mk is special because it is part of the
br2-external structure, so it is likely someone expects it to be
checkable by an in-tree script.

Instead of adding another blob to the manual, just ignore this file.
Only do that when a br2-external is being tested (so with option -b
passed to the script) and also check that it is on the root path of the
br2-external to allow someone to have a package called external.

Reported on bug #11271.

Reported-by: Vitaliy Lotorev <lotorev@gmail.com>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 utils/check-package | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils/check-package b/utils/check-package
index aa6993dabd..ebd82cba95 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -71,6 +71,9 @@ def get_lib_from_filename(fname):
             return None
         if DO_NOT_CHECK_INTREE.match(fname):
             return None
+    else:
+        if os.path.basename(fname) == "external.mk" and os.path.exists(fname[:-2] + "desc"):
+            return None
     if CONFIG_IN_FILENAME.search(fname):
         return checkpackagelib.lib_config
     if fname.endswith(".hash"):
-- 
2.17.1

[Buildroot] [PATCH 5/5] support/testing: test check-package ignores external.mk

[Ricardo Martincoski]

Check external.mk is ignored only when in the root path of a
br2-external.

Add a file called external.mk as a fixture to be used by the test case.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
 .../tests/utils/br2-external/external.mk      |  2 ++
 .../br2-external/package/external/external.mk |  1 +
 .../testing/tests/utils/test_check_package.py | 25 ++++++++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 support/testing/tests/utils/br2-external/package/external/external.mk

diff --git a/support/testing/tests/utils/br2-external/external.mk b/support/testing/tests/utils/br2-external/external.mk
index e69de29bb2..470c01a382 100644
--- a/support/testing/tests/utils/br2-external/external.mk
+++ b/support/testing/tests/utils/br2-external/external.mk
@@ -0,0 +1,2 @@
+custom-target:
+	@echo "do nothing"
diff --git a/support/testing/tests/utils/br2-external/package/external/external.mk b/support/testing/tests/utils/br2-external/package/external/external.mk
new file mode 100644
index 0000000000..74fb6540ab
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/package/external/external.mk
@@ -0,0 +1 @@
+# wrong
diff --git a/support/testing/tests/utils/test_check_package.py b/support/testing/tests/utils/test_check_package.py
index 85f2e280a8..301aae51ac 100644
--- a/support/testing/tests/utils/test_check_package.py
+++ b/support/testing/tests/utils/test_check_package.py
@@ -66,7 +66,7 @@ class TestCheckPackageBasicUsage(TestCheckPackage):
     The script can be called either using relative path, absolute path or from PATH.
     The files to be checked can be passed as arguments using either relative path or absolute path.
     When in in-tree mode (without -b) some in-tree files and also all out-of-tree files are ignored.
-    When in out-tree mode (with -b) the script does generate warnings.
+    When in out-tree mode (with -b) the script does generate warnings but ignores external.mk.
     """
 
     def test_run(self):
@@ -146,3 +146,26 @@ class TestCheckPackageBasicUsage(TestCheckPackage):
         self.assert_file_was_processed(m)
         self.assert_warnings_generated_for_file(m)
         self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
+
+        # external.mk is ignored only when in the root path of a br2-external
+        rel_file = "external.mk"
+        abs_file = os.path.join(abs_path, rel_file)
+
+        _, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        rel_file = "package/external/external.mk"
+        abs_file = os.path.join(abs_path, rel_file)
+
+        w, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)".format(abs_file), w)
+
+        w, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)".format(abs_file), w)
-- 
2.17.1

[Buildroot] [PATCH 1/5] check-package: fix check of file in current dir with -b

[Ricardo Martincoski]

Hello,

On Sat, Nov 03, 2018 at 01:56 AM, Ricardo Martincoski wrote:

> One of the possible usages of check-package is to first cd to the
> directory that contains the files to test (e.g. a package directory) and
> then call the script passing the files in the current dir.
> It already works when used for intree files, but for files in a
> br2-external it throws an exception because some check functions (from
> utils/checkpackagelib/lib_*.py) do need the name of the file being
> processed and assume there will be a slash before the name.
> 
> Notice RemoveDefaultPackageSourceVariable and TypoInPackageVariable lead
> to an exception in this case, but ApplyOrder instead generates a false
> warning.
> 
> Instead of fixing any check function that could lead to different
> behaviour when the script is called with -b for a file in the current
> directory, always use the absolute path of the file being checked when
> -b was passed. A small drawback is that the warning messages will occupy
> more columns in this case, but this approach is more future-proof as new
> check functions may also need to depend on the name of the file being
> processed.

I will rework this patch and resend, based on the feedback in [1].
I will mark v1 of the series as Changes Requested.

[1] http://lists.busybox.net/pipermail/buildroot/2018-November/234966.html


Regards,
Ricardo

[Buildroot] [PATCH v2 0/5] Fix bug 11271 and add test for check-package

[Ricardo Martincoski]

Hello,

This series fixes bug 11271 and adds a test case that catches the bug.

Patch 1 fixes the usage of check-package when called from the same directory
than the files to be tested in a br2-external tree.

Patch 2 improves the testing infra by ensuring it can be started from any path,
not only the Buildroot top directory.
As a consequence the test infra will always test the repo it belongs to.

Patch 3 creates a test case for check-package, calling the script in the same
repo against in-tree files and against files in a br2-external.

Patch 4 improves usability of check-package in a br2-external tree by ignoring
external.mk since it is special: it usually contains raw makefile targets and
therefore cannot be tested by the script, but it is part of the br2-external
structure, so it is likely someone expects it to be checkable by an in-tree
script.

Patch 5 improves the test case created by patch 3 to test the scenario covered
by patch 4.

On below tests I used another series just to make my life easier:
http://patchwork.ozlabs.org/project/buildroot/list/?series=72927

1) to check patch 2 works and don't bring regressions: (I reused the urls for v1
   because patch 2 did not change since v1 and also the master branch had no
   change on support/testing since I sent v1)

A full run with only patch 2 applied:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290055

Another full run with only patch 2 applied, this time cd'ing to /tmp and
starting the tests from there:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290166

Without any patches of this series, cd'ing to /tmp and starting the tests from
there:
https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290220

2) to check patch 3 tests what patch 1 fixes:

With only patches 2 and 3 applied: failed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174566

With patches 1, 2 and 3 applied: passed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174639

3) to check patch 5 tests what patch 4 fixes:

With only patches 1, 2, 3 and 5 applied: failed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174711

With all patches applied: passed
https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174756

Changes v1 -> v2:
  - do not absolutize paths on output (Vitaliy Lotorev)
  - update expectation on the test case after changing patch 1: when a warning
    is generated for a file from a br2-external and the file was specified in
    the arguments as a relative path, the relative path is used in the warning
    message

Regards,
Ricardo

Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Vitaliy Lotorev <lotorev@gmail.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>

Ricardo Martincoski (5):
  check-package: fix check of file in current dir with -b
  support/testing: allow run-tests to be called from anywhere
  support/testing: add test for check-package
  check-package: ignore external.mk
  support/testing: test check-package ignores external.mk

 .gitlab-ci.yml                                |   1 +
 support/testing/infra/__init__.py             |   8 +-
 support/testing/infra/builder.py              |   2 +-
 support/testing/tests/utils/__init__.py       |   0
 .../tests/utils/br2-external/Config.in        |   1 +
 .../tests/utils/br2-external/external.desc    |   1 +
 .../tests/utils/br2-external/external.mk      |   2 +
 .../br2-external/package/external/external.mk |   1 +
 .../testing/tests/utils/test_check_package.py | 171 ++++++++++++++++++
 utils/check-package                           |   3 +
 utils/checkpackagelib/lib_mk.py               |   7 +-
 utils/checkpackagelib/lib_patch.py            |   5 +-
 12 files changed, 194 insertions(+), 8 deletions(-)
 create mode 100644 support/testing/tests/utils/__init__.py
 create mode 100644 support/testing/tests/utils/br2-external/Config.in
 create mode 100644 support/testing/tests/utils/br2-external/external.desc
 create mode 100644 support/testing/tests/utils/br2-external/external.mk
 create mode 100644 support/testing/tests/utils/br2-external/package/external/external.mk
 create mode 100644 support/testing/tests/utils/test_check_package.py

-- 
2.17.1

[Buildroot] [PATCH v2 1/5] check-package: fix check of file in current dir with -b

[Ricardo Martincoski]

One of the possible usages of check-package is to first cd to the
directory that contains the files to test (e.g. a package directory) and
then call the script passing the files in the current dir.
It already works when used for intree files, but for files in a
br2-external it throws an exception because some check functions (from
utils/checkpackagelib/lib_*.py) do need the name of the file being
processed and assume there will be a slash before the name.

Fix all check functions that assume that the full filename being checked
contains a slash. Do not use regexps to extract the filename, use
os.path functions instead.

Notice RemoveDefaultPackageSourceVariable and TypoInPackageVariable lead
to an exception in this case, but ApplyOrder instead generates a false
warning.

Fixes bug #11271.

Reported-by: Vitaliy Lotorev <lotorev@gmail.com>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Vitaliy Lotorev <lotorev@gmail.com>
---
Changes v1 -> v2:
  - do not absolutize paths on output (Vitaliy Lotorev)
---
 utils/checkpackagelib/lib_mk.py    | 7 +++----
 utils/checkpackagelib/lib_patch.py | 5 +++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/utils/checkpackagelib/lib_mk.py b/utils/checkpackagelib/lib_mk.py
index 0e430a2f12..f2f2a244be 100644
--- a/utils/checkpackagelib/lib_mk.py
+++ b/utils/checkpackagelib/lib_mk.py
@@ -4,6 +4,7 @@
 # menu options using "make menuconfig" and by running "make" with appropriate
 # packages enabled.
 
+import os
 import re
 
 from base import _CheckFunction
@@ -100,10 +101,9 @@ class PackageHeader(_CheckFunction):
 
 class RemoveDefaultPackageSourceVariable(_CheckFunction):
     packages_that_may_contain_default_source = ["binutils", "gcc", "gdb"]
-    PACKAGE_NAME = re.compile("/([^/]+)\.mk")
 
     def before(self):
-        package = self.PACKAGE_NAME.search(self.filename).group(1)
+        package, _ = os.path.splitext(os.path.basename(self.filename))
         package_upper = package.replace("-", "_").upper()
         self.package = package
         self.FIND_SOURCE = re.compile(
@@ -173,11 +173,10 @@ class TypoInPackageVariable(_CheckFunction):
         "TARGET_FINALIZE_HOOKS",
         "TARGETS_ROOTFS",
         "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, _ = os.path.splitext(os.path.basename(self.filename))
         package = package.replace("-", "_").upper()
         # linux tools do not use LINUX_TOOL_ prefix for variables
         package = package.replace("LINUX_TOOL_", "")
diff --git a/utils/checkpackagelib/lib_patch.py b/utils/checkpackagelib/lib_patch.py
index 555621afa1..bd0f24938b 100644
--- a/utils/checkpackagelib/lib_patch.py
+++ b/utils/checkpackagelib/lib_patch.py
@@ -3,6 +3,7 @@
 # functions don't need to check for things already checked by running
 # "make package-dirclean package-patch".
 
+import os
 import re
 
 from base import _CheckFunction
@@ -10,10 +11,10 @@ from lib import NewlineAtEof           # noqa: F401
 
 
 class ApplyOrder(_CheckFunction):
-    APPLY_ORDER = re.compile("/\d{1,4}-[^/]*$")
+    APPLY_ORDER = re.compile("\d{1,4}-[^/]*$")
 
     def before(self):
-        if not self.APPLY_ORDER.search(self.filename):
+        if not self.APPLY_ORDER.match(os.path.basename(self.filename)):
             return ["{}:0: use name <number>-<description>.patch "
                     "({}#_providing_patches)"
                     .format(self.filename, self.url_to_manual)]
-- 
2.17.1

[Buildroot] [PATCH v2 2/5] support/testing: allow run-tests to be called from anywhere

[Ricardo Martincoski]

Currently run-tests must be called from the Buildroot top directory.

Derive the top directory from the script path, so run-tests can be called from
any path.
As a consequence the test infra will always test the repo it belongs to.

Suggested-by: Yann E. MORIN <yann.morin.1998@free.fr>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Cc: Yann E. MORIN <yann.morin.1998@free.fr>
---
The suggestion happened one year ago, but I just remembered it now that
I need the base dir to be used by a test case.
Search for "derive the top" in
http://patchwork.ozlabs.org/patch/814544/

Changes v1 -> v2:
  - no changes
---
 support/testing/infra/__init__.py | 8 +++++++-
 support/testing/infra/builder.py  | 2 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/support/testing/infra/__init__.py b/support/testing/infra/__init__.py
index e229e90852..1d4d18bbe9 100644
--- a/support/testing/infra/__init__.py
+++ b/support/testing/infra/__init__.py
@@ -6,6 +6,7 @@ import subprocess
 from urllib2 import urlopen, HTTPError, URLError
 
 ARTIFACTS_URL = "http://autobuild.buildroot.net/artefacts/"
+BASE_DIR = os.path.realpath(os.path.join(os.path.dirname(__file__), "../../.."))
 
 
 def open_log_file(builddir, stage, logtofile=True):
@@ -21,8 +22,13 @@ def open_log_file(builddir, stage, logtofile=True):
     return fhandle
 
 
+def basepath(relpath=""):
+    """Return the absolute path for a file or directory relative to the Buildroot top directory."""
+    return os.path.join(BASE_DIR, relpath)
+
+
 def filepath(relpath):
-    return os.path.join(os.getcwd(), "support/testing", relpath)
+    return os.path.join(BASE_DIR, "support/testing", relpath)
 
 
 def download(dldir, filename):
diff --git a/support/testing/infra/builder.py b/support/testing/infra/builder.py
index fc318fe26e..26fa6be7ae 100644
--- a/support/testing/infra/builder.py
+++ b/support/testing/infra/builder.py
@@ -29,7 +29,7 @@ class Builder(object):
                "O={}".format(self.builddir),
                "olddefconfig"]
         ret = subprocess.call(cmd, stdout=self.logfile, stderr=self.logfile,
-                              env=env)
+                              cwd=infra.basepath(), env=env)
         if ret != 0:
             raise SystemError("Cannot olddefconfig")
 
-- 
2.17.1

[Buildroot] [PATCH v2 3/5] support/testing: add test for check-package

[Ricardo Martincoski]

Check the basic usage for check-package.

It can be called using either absolute path, relative path or from PATH.
Files to be checked can be passed with either absolute path or relative
path (also including files in the current directory).

Also check it ignores some special files when checking intree files,
i.e. package/pkg-generic.mk, while still generating warnings for out-of-tree
files when called with -b.
In order to allow the later, add an empty line to the Config.in in the
br2-external being tested so the script does generate a warning.

Catches bug #11271.

More tests can be added later, for example compatibility to Python 3.

Suggested-by: Arnout Vandecappelle <arnout@mind.be>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
NOTE: please don't remove the empty line on
support/testing/tests/utils/br2-external/Config.in
as it was intended.

Changes v1 -> v2:
  - update expectation after changing patch 1: when a warning is
    generated for a file from a br2-external and the file was specified
    in the arguments as a relative path, the relative path is used in
    the warning message
---
 .gitlab-ci.yml                                |   1 +
 support/testing/tests/utils/__init__.py       |   0
 .../tests/utils/br2-external/Config.in        |   1 +
 .../tests/utils/br2-external/external.desc    |   1 +
 .../tests/utils/br2-external/external.mk      |   0
 .../testing/tests/utils/test_check_package.py | 148 ++++++++++++++++++
 6 files changed, 151 insertions(+)
 create mode 100644 support/testing/tests/utils/__init__.py
 create mode 100644 support/testing/tests/utils/br2-external/Config.in
 create mode 100644 support/testing/tests/utils/br2-external/external.desc
 create mode 100644 support/testing/tests/utils/br2-external/external.mk
 create mode 100644 support/testing/tests/utils/test_check_package.py

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index a5fbdcb0d8..0eeabbdfcf 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -336,3 +336,4 @@ tests.toolchain.test_external.TestExternalToolchainLinaroArm: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv4: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv5: *runtime_test
 tests.toolchain.test_external.TestExternalToolchainSourceryArmv7: *runtime_test
+tests.utils.test_check_package.TestCheckPackageBasicUsage: *runtime_test
diff --git a/support/testing/tests/utils/__init__.py b/support/testing/tests/utils/__init__.py
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/support/testing/tests/utils/br2-external/Config.in b/support/testing/tests/utils/br2-external/Config.in
new file mode 100644
index 0000000000..8b13789179
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/Config.in
@@ -0,0 +1 @@
+
diff --git a/support/testing/tests/utils/br2-external/external.desc b/support/testing/tests/utils/br2-external/external.desc
new file mode 100644
index 0000000000..e89c3560ab
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/external.desc
@@ -0,0 +1 @@
+name: CHECK_PACKAGE
diff --git a/support/testing/tests/utils/br2-external/external.mk b/support/testing/tests/utils/br2-external/external.mk
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/support/testing/tests/utils/test_check_package.py b/support/testing/tests/utils/test_check_package.py
new file mode 100644
index 0000000000..35c2adcf8a
--- /dev/null
+++ b/support/testing/tests/utils/test_check_package.py
@@ -0,0 +1,148 @@
+"""Test cases for utils/check-package.
+
+It does not inherit from infra.basetest.BRTest and therefore does not generate a logfile.
+Only when the tests fail there will be output to the console.
+The make target ('make check-package') is already used by the job 'check-package' and won't be tested here.
+"""
+import os
+import subprocess
+import unittest
+
+import infra
+
+
+def call_script(args, env, cwd):
+    """Call a script and return stdout and stderr as lists."""
+    out, err = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, cwd=cwd).communicate()
+    return out.splitlines(), err.splitlines()
+
+
+class TestCheckPackage(unittest.TestCase):
+    """Base class for all test cases for check-package.
+
+    It inherits from TestCase so its methods can use unittest assertions but it does not declare a test_run method in order to be
+    ignored by nose2 when the test cases are discovered.
+    """
+
+    WITH_EMPTY_PATH = {}
+    WITH_UTILS_IN_PATH = {"PATH": infra.basepath("utils") + ":" + os.environ["PATH"]}
+    relative = [
+        # base_script           base_file               rel_script               rel_file                rel_cwd
+        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "package/atop/atop.mk", ""],
+        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "./package/atop/atop.mk", ""],
+        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "atop.mk", "package/atop"],
+        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "./atop.mk", "package/atop"],
+        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "atop/atop.mk", "package"],
+        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "./atop/atop.mk", "package"],
+        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "package/atop/Config.in", ""],
+        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "./package/atop/Config.in", ""],
+        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "Config.in", "package/atop"],
+        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "./Config.in", "package/atop"],
+        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "atop/Config.in", "package"],
+        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "./atop/Config.in", "package"]]
+
+    def assert_file_was_processed(self, stderr):
+        """Infer from check-package stderr if at least one file was processed and fail otherwise."""
+        self.assertIn("lines processed", stderr[0], stderr)
+        processed = int(stderr[0].split()[0])
+        self.assertGreater(processed, 0)
+
+    def assert_file_was_ignored(self, stderr):
+        """Infer from check-package stderr if no file was processed and fail otherwise."""
+        self.assertIn("lines processed", stderr[0], stderr)
+        processed = int(stderr[0].split()[0])
+        self.assertEqual(processed, 0)
+
+    def assert_warnings_generated_for_file(self, stderr):
+        """Infer from check-package stderr if at least one warning was generatd and fail otherwise."""
+        self.assertIn("warnings generated", stderr[1], stderr)
+        generated = int(stderr[1].split()[0])
+        self.assertGreater(generated, 0)
+
+
+class TestCheckPackageBasicUsage(TestCheckPackage):
+    """Test the various ways the script can be called.
+
+    The script can be called either using relative path, absolute path or from PATH.
+    The files to be checked can be passed as arguments using either relative path or absolute path.
+    When in in-tree mode (without -b) some in-tree files and also all out-of-tree files are ignored.
+    When in out-tree mode (with -b) the script does generate warnings.
+    """
+
+    def test_run(self):
+        """Test the various ways the script can be called in a simple top to bottom sequence."""
+        # an intree file can be checked by the script called from relative path, absolute path and from PATH
+        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
+            abs_script = infra.basepath(base_script)
+            abs_file = infra.basepath(base_file)
+            cwd = infra.basepath(rel_cwd)
+
+            _, m = call_script([rel_script, rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([rel_script, abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+        # some intree files are ignored
+        _, m = call_script(["./utils/check-package", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["./utils/check-package", "-b", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+
+        # an out-of-tree file can be checked by the script called from relative path, absolute path and from PATH
+        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
+            abs_script = infra.basepath(base_script)
+            abs_file = infra.basepath(base_file)
+            cwd = infra.basepath(rel_cwd)
+
+            _, m = call_script([rel_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([rel_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script([abs_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+            _, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, cwd)
+            self.assert_file_was_processed(m)
+
+        # out-of-tree files are are ignored without -b but can generate warnings with -b
+        abs_path = infra.filepath("tests/utils/br2-external")
+        rel_file = "Config.in"
+        abs_file = os.path.join(abs_path, rel_file)
+
+        _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        w, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: empty line at end of file".format(rel_file), w)
+
+        w, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
-- 
2.17.1

[Buildroot] [PATCH v2 4/5] check-package: ignore external.mk

[Ricardo Martincoski]

The external.mk file in a br2-external usually contains raw makefile
targets. This file is common code and not a package recipe so it should
not be tested against the code-style of a package .mk file.

When using this script to check files in a br2-external tree, usually
the user is responsible for not passing files that check-package do not
understand. But external.mk is special because it is part of the
br2-external structure, so it is likely someone expects it to be
checkable by an in-tree script.

Instead of adding another blob to the manual, just ignore this file.
Only do that when a br2-external is being tested (so with option -b
passed to the script) and also check that it is on the root path of the
br2-external to allow someone to have a package called external.

Reported on bug #11271.

Reported-by: Vitaliy Lotorev <lotorev@gmail.com>
Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
Changes v1 -> v2:
  - no changes
---
 utils/check-package | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/utils/check-package b/utils/check-package
index 3dbc28b0a2..3969ec3ec7 100755
--- a/utils/check-package
+++ b/utils/check-package
@@ -71,6 +71,9 @@ def get_lib_from_filename(fname):
             return None
         if DO_NOT_CHECK_INTREE.match(fname):
             return None
+    else:
+        if os.path.basename(fname) == "external.mk" and os.path.exists(fname[:-2] + "desc"):
+            return None
     if CONFIG_IN_FILENAME.search(fname):
         return checkpackagelib.lib_config
     if fname.endswith(".hash"):
-- 
2.17.1

[Buildroot] [PATCH v2 5/5] support/testing: test check-package ignores external.mk

[Ricardo Martincoski]

Check external.mk is ignored only when in the root path of a
br2-external.

Add a file called external.mk as a fixture to be used by the test case.

Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com>
Cc: Arnout Vandecappelle <arnout@mind.be>
---
Changes v1 -> v2:
  - update expectation after changing patch 1: when a warning is
    generated for a file from a br2-external and the file was specified
    in the arguments as a relative path, the relative path is used in
    the warning message
  - make the test stronger by cd'ing to br2-external/package/external
    before testing check-package against the newly added external.mk
---
 .../tests/utils/br2-external/external.mk      |  2 ++
 .../br2-external/package/external/external.mk |  1 +
 .../testing/tests/utils/test_check_package.py | 25 ++++++++++++++++++-
 3 files changed, 27 insertions(+), 1 deletion(-)
 create mode 100644 support/testing/tests/utils/br2-external/package/external/external.mk

diff --git a/support/testing/tests/utils/br2-external/external.mk b/support/testing/tests/utils/br2-external/external.mk
index e69de29bb2..470c01a382 100644
--- a/support/testing/tests/utils/br2-external/external.mk
+++ b/support/testing/tests/utils/br2-external/external.mk
@@ -0,0 +1,2 @@
+custom-target:
+	@echo "do nothing"
diff --git a/support/testing/tests/utils/br2-external/package/external/external.mk b/support/testing/tests/utils/br2-external/package/external/external.mk
new file mode 100644
index 0000000000..74fb6540ab
--- /dev/null
+++ b/support/testing/tests/utils/br2-external/package/external/external.mk
@@ -0,0 +1 @@
+# wrong
diff --git a/support/testing/tests/utils/test_check_package.py b/support/testing/tests/utils/test_check_package.py
index 35c2adcf8a..75a3e3a41b 100644
--- a/support/testing/tests/utils/test_check_package.py
+++ b/support/testing/tests/utils/test_check_package.py
@@ -66,7 +66,7 @@ class TestCheckPackageBasicUsage(TestCheckPackage):
     The script can be called either using relative path, absolute path or from PATH.
     The files to be checked can be passed as arguments using either relative path or absolute path.
     When in in-tree mode (without -b) some in-tree files and also all out-of-tree files are ignored.
-    When in out-tree mode (with -b) the script does generate warnings.
+    When in out-tree mode (with -b) the script does generate warnings but ignores external.mk.
     """
 
     def test_run(self):
@@ -146,3 +146,26 @@ class TestCheckPackageBasicUsage(TestCheckPackage):
         self.assert_file_was_processed(m)
         self.assert_warnings_generated_for_file(m)
         self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
+
+        # external.mk is ignored only when in the root path of a br2-external
+        rel_file = "external.mk"
+        abs_file = os.path.join(abs_path, rel_file)
+
+        _, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_ignored(m)
+
+        _, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_ignored(m)
+
+        abs_path = infra.filepath("tests/utils/br2-external/package/external")
+        abs_file = os.path.join(abs_path, rel_file)
+
+        w, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)".format(rel_file), w)
+
+        w, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
+        self.assert_file_was_processed(m)
+        self.assert_warnings_generated_for_file(m)
+        self.assertIn("{}:1: should be 80 hashes (http://nightly.buildroot.org/#writing-rules-mk)".format(abs_file), w)
-- 
2.17.1

[Buildroot] [PATCH v2 0/5] Fix bug 11271 and add test for check-package

[Arnout Vandecappelle]

 Hi Ricardo,
On 04/11/2018 05:12, Ricardo Martincoski wrote:
> Hello,
> 
> This series fixes bug 11271 and adds a test case that catches the bug.

 Sorry it took so long to apply this series...


> Patch 1 fixes the usage of check-package when called from the same directory
> than the files to be tested in a br2-external tree.
> 
> Patch 2 improves the testing infra by ensuring it can be started from any path,
> not only the Buildroot top directory.
> As a consequence the test infra will always test the repo it belongs to.
> 
> Patch 3 creates a test case for check-package, calling the script in the same
> repo against in-tree files and against files in a br2-external.

 I had a few concerns about this one, but I just fixed it up and applied.

> 
> Patch 4 improves usability of check-package in a br2-external tree by ignoring
> external.mk since it is special: it usually contains raw makefile targets and
> therefore cannot be tested by the script, but it is part of the br2-external
> structure, so it is likely someone expects it to be checkable by an in-tree
> script.
> 
> Patch 5 improves the test case created by patch 3 to test the scenario covered
> by patch 4.

 Series applies to master, thanks!

 Regards,
 Arnout

> 
> On below tests I used another series just to make my life easier:
> http://patchwork.ozlabs.org/project/buildroot/list/?series=72927
> 
> 1) to check patch 2 works and don't bring regressions: (I reused the urls for v1
>    because patch 2 did not change since v1 and also the master branch had no
>    change on support/testing since I sent v1)
> 
> A full run with only patch 2 applied:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290055
> 
> Another full run with only patch 2 applied, this time cd'ing to /tmp and
> starting the tests from there:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290166
> 
> Without any patches of this series, cd'ing to /tmp and starting the tests from
> there:
> https://gitlab.com/RicardoMartincoski/buildroot/pipelines/35290220
> 
> 2) to check patch 3 tests what patch 1 fixes:
> 
> With only patches 2 and 3 applied: failed
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174566
> 
> With patches 1, 2 and 3 applied: passed
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174639
> 
> 3) to check patch 5 tests what patch 4 fixes:
> 
> With only patches 1, 2, 3 and 5 applied: failed
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174711
> 
> With all patches applied: passed
> https://gitlab.com/RicardoMartincoski/buildroot/-/jobs/116174756
> 
> Changes v1 -> v2:
>   - do not absolutize paths on output (Vitaliy Lotorev)
>   - update expectation on the test case after changing patch 1: when a warning
>     is generated for a file from a br2-external and the file was specified in
>     the arguments as a relative path, the relative path is used in the warning
>     message
> 
> Regards,
> Ricardo
> 
> Cc: Arnout Vandecappelle <arnout@mind.be>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> Cc: Vitaliy Lotorev <lotorev@gmail.com>
> Cc: Yann E. MORIN <yann.morin.1998@free.fr>
> 
> Ricardo Martincoski (5):
>   check-package: fix check of file in current dir with -b
>   support/testing: allow run-tests to be called from anywhere
>   support/testing: add test for check-package
>   check-package: ignore external.mk
>   support/testing: test check-package ignores external.mk
> 
>  .gitlab-ci.yml                                |   1 +
>  support/testing/infra/__init__.py             |   8 +-
>  support/testing/infra/builder.py              |   2 +-
>  support/testing/tests/utils/__init__.py       |   0
>  .../tests/utils/br2-external/Config.in        |   1 +
>  .../tests/utils/br2-external/external.desc    |   1 +
>  .../tests/utils/br2-external/external.mk      |   2 +
>  .../br2-external/package/external/external.mk |   1 +
>  .../testing/tests/utils/test_check_package.py | 171 ++++++++++++++++++
>  utils/check-package                           |   3 +
>  utils/checkpackagelib/lib_mk.py               |   7 +-
>  utils/checkpackagelib/lib_patch.py            |   5 +-
>  12 files changed, 194 insertions(+), 8 deletions(-)
>  create mode 100644 support/testing/tests/utils/__init__.py
>  create mode 100644 support/testing/tests/utils/br2-external/Config.in
>  create mode 100644 support/testing/tests/utils/br2-external/external.desc
>  create mode 100644 support/testing/tests/utils/br2-external/external.mk
>  create mode 100644 support/testing/tests/utils/br2-external/package/external/external.mk
>  create mode 100644 support/testing/tests/utils/test_check_package.py
> 

[Buildroot] [PATCH 3/5] support/testing: add test for check-package

[Arnout Vandecappelle]

 Hi Ricardo,

On 03/11/2018 05:56, Ricardo Martincoski wrote:
> Check the basic usage for check-package.
> 
> It can be called using either absolute path, relative path or from PATH.
> Files to be checked can be passed with either absolute path or relative
> path (also including files in the current directory).
> 
> Also check it ignores some special files when checking intree files,
> i.e. package/pkg-generic.mk, while still generating warnings for out-of-tree
> files when called with -b.
> In order to allow the later, add an empty line to the Config.in in the
> br2-external being tested so the script does generate a warning.
> 
> Catches bug #11271.
> 
> More tests can be added later, for example compatibility to Python 3.
[snip]
> diff --git a/support/testing/tests/utils/test_check_package.py b/support/testing/tests/utils/test_check_package.py
> new file mode 100644
> index 0000000000..85f2e280a8
> --- /dev/null
> +++ b/support/testing/tests/utils/test_check_package.py
> @@ -0,0 +1,148 @@
> +"""Test cases for utils/check-package.
> +
> +It does not inherit from infra.basetest.BRTest and therefore does not generate a logfile.

 Lines should be wrapped at 80 columns. We override that limit in the gitlab-CI
test because sometimes the code is uglier by wrapping exactly at 80, but the
.flake8 file does set the limit at 80.

 So I've rewrapped the entire file where appropriate. I've done the same with
the following two commits.

> +Only when the tests fail there will be output to the console.
> +The make target ('make check-package') is already used by the job 'check-package' and won't be tested here.
> +"""
> +import os
> +import subprocess
> +import unittest
> +
> +import infra
> +
> +
> +def call_script(args, env, cwd):
> +    """Call a script and return stdout and stderr as lists."""
> +    out, err = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE, env=env, cwd=cwd).communicate()
> +    return out.splitlines(), err.splitlines()
> +
> +
> +class TestCheckPackage(unittest.TestCase):
> +    """Base class for all test cases for check-package.

 Since there is only one derived class, and it doesn't seem worth to make
additional classes even if more tests are added, I've merged this with
TestCheckPackageBasicUsage.

> +
> +    It inherits from TestCase so its methods can use unittest assertions but it does not declare a test_run method in order to be
> +    ignored by nose2 when the test cases are discovered.
> +    """
> +
> +    WITH_EMPTY_PATH = {}
> +    WITH_UTILS_IN_PATH = {"PATH": infra.basepath("utils") + ":" + os.environ["PATH"]}
> +    relative = [
> +        # base_script           base_file               rel_script               rel_file                rel_cwd
> +        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "package/atop/atop.mk", ""],
> +        ["utils/check-package", "package/atop/atop.mk", "./utils/check-package", "./package/atop/atop.mk", ""],
> +        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "atop.mk", "package/atop"],
> +        ["utils/check-package", "package/atop/atop.mk", "../../utils/check-package", "./atop.mk", "package/atop"],
> +        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "atop/atop.mk", "package"],
> +        ["utils/check-package", "package/atop/atop.mk", "../utils/check-package", "./atop/atop.mk", "package"],
> +        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "package/atop/Config.in", ""],
> +        ["utils/check-package", "package/atop/Config.in", "./utils/check-package", "./package/atop/Config.in", ""],
> +        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "Config.in", "package/atop"],
> +        ["utils/check-package", "package/atop/Config.in", "../../utils/check-package", "./Config.in", "package/atop"],
> +        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "atop/Config.in", "package"],
> +        ["utils/check-package", "package/atop/Config.in", "../utils/check-package", "./atop/Config.in", "package"]]

 I have the feeling that it would be sufficient to just have rel_cwd in this
table and derive the rest from that (and using a loop for the .mk and
Config.in). But it's not worth spending time on that, so I've kept it as is.


> +
> +    def assert_file_was_processed(self, stderr):
> +        """Infer from check-package stderr if at least one file was processed and fail otherwise."""
> +        self.assertIn("lines processed", stderr[0], stderr)
> +        processed = int(stderr[0].split()[0])
> +        self.assertGreater(processed, 0)
> +
> +    def assert_file_was_ignored(self, stderr):
> +        """Infer from check-package stderr if no file was processed and fail otherwise."""
> +        self.assertIn("lines processed", stderr[0], stderr)
> +        processed = int(stderr[0].split()[0])
> +        self.assertEqual(processed, 0)
> +
> +    def assert_warnings_generated_for_file(self, stderr):
> +        """Infer from check-package stderr if at least one warning was generatd and fail otherwise."""
> +        self.assertIn("warnings generated", stderr[1], stderr)
> +        generated = int(stderr[1].split()[0])
> +        self.assertGreater(generated, 0)
> +
> +
> +class TestCheckPackageBasicUsage(TestCheckPackage):
> +    """Test the various ways the script can be called.
> +
> +    The script can be called either using relative path, absolute path or from PATH.
> +    The files to be checked can be passed as arguments using either relative path or absolute path.
> +    When in in-tree mode (without -b) some in-tree files and also all out-of-tree files are ignored.
> +    When in out-tree mode (with -b) the script does generate warnings.
> +    """
> +
> +    def test_run(self):
> +        """Test the various ways the script can be called in a simple top to bottom sequence."""
> +        # an intree file can be checked by the script called from relative path, absolute path and from PATH
> +        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
> +            abs_script = infra.basepath(base_script)
> +            abs_file = infra.basepath(base_file)
> +            cwd = infra.basepath(rel_cwd)
> +
> +            _, m = call_script([rel_script, rel_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([abs_script, rel_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([rel_script, abs_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([abs_script, abs_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +        # some intree files are ignored
> +        _, m = call_script(["./utils/check-package", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
> +        self.assert_file_was_ignored(m)
> +
> +        _, m = call_script(["./utils/check-package", "-b", "package/pkg-generic.mk"], self.WITH_EMPTY_PATH, infra.basepath())
> +        self.assert_file_was_processed(m)
> +
> +        # an out-of-tree file can be checked by the script called from relative path, absolute path and from PATH
> +        for base_script, base_file, rel_script, rel_file, rel_cwd in self.relative:
> +            abs_script = infra.basepath(base_script)
> +            abs_file = infra.basepath(base_file)
> +            cwd = infra.basepath(rel_cwd)
> +
> +            _, m = call_script([rel_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([abs_script, "-b", rel_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([rel_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script([abs_script, "-b", abs_file], self.WITH_EMPTY_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +            _, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, cwd)
> +            self.assert_file_was_processed(m)
> +
> +        # out-of-tree files are are ignored without -b but can generate warnings with -b
> +        abs_path = infra.filepath("tests/utils/br2-external")

 It might be nice to also test out-of-tree files that are *really* out of tree,
but that's a bit more work...

 Regards,
 Arnout

> +        rel_file = "Config.in"
> +        abs_file = os.path.join(abs_path, rel_file)
> +
> +        _, m = call_script(["check-package", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
> +        self.assert_file_was_ignored(m)
> +
> +        _, m = call_script(["check-package", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
> +        self.assert_file_was_ignored(m)
> +
> +        w, m = call_script(["check-package", "-b", rel_file], self.WITH_UTILS_IN_PATH, abs_path)
> +        self.assert_file_was_processed(m)
> +        self.assert_warnings_generated_for_file(m)
> +        self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
> +
> +        w, m = call_script(["check-package", "-b", abs_file], self.WITH_UTILS_IN_PATH, infra.basepath())
> +        self.assert_file_was_processed(m)
> +        self.assert_warnings_generated_for_file(m)
> +        self.assertIn("{}:1: empty line at end of file".format(abs_file), w)
>