diff mbox series

[4/4] utils/getdeveloperlib.py: use relative paths for files

Message ID 20201119145354.1175043-4-thomas.petazzoni@bootlin.com
State New
Headers show
Series [1/4] support/scripts/pkg-stats: import cve module only when needed | expand

Commit Message

Thomas Petazzoni Nov. 19, 2020, 2:53 p.m. UTC
Using absolute paths within getdeveloperlib isn't very sensible, it
makes a lot more sense to handle everything as relative paths from the
top-level Buildroot source directory.

parse_developers() is changed to no longer take the base path as
argument: it is automatically calculated based on the location of
utils/getdeveloperlib.py. Then, the rest of the logic is adjusted to
use relative paths, and prepend them with the base "brpath" when
needed.

This commit allows pkg-stats to report correct developers information
even when executed from an out of tree directory.

Before this patch:

$ ~/buildroot/support/scripts/pkg-stats -p ipmitool --json out.json
$ cat out.json | jq '.packages.ipmitool.developers'
[]

$ cat out.json | jq '.defconfigs.stm32f469_disco'
{
  "name": "stm32f469_disco",
  "path": "configs/stm32f469_disco_defconfig",
  "developers": []
}

After this patch:

$ ~/buildroot/support/scripts/pkg-stats -p ipmitool --json out.json
$ cat out.json | jq '.packages.ipmitool.developers'
[
  "Floris Bos <bos@je-eigen-domein.nl>",
  "Heiko Thiery <heiko.thiery@gmail.com>"
]
$ cat out.json | jq '.defconfigs.stm32f469_disco'
{
  "name": "stm32f469_disco",
  "path": "configs/stm32f469_disco_defconfig",
  "developers": [
    "Christophe Priouzeau <christophe.priouzeau@st.com>"
  ]
}

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/scripts/pkg-stats |  2 +-
 utils/get-developers      |  5 -----
 utils/getdeveloperlib.py  | 26 ++++++++++++--------------
 3 files changed, 13 insertions(+), 20 deletions(-)

Comments

Heiko Thiery Nov. 20, 2020, 10:03 a.m. UTC | #1
Hi Thomas,

Am Do., 19. Nov. 2020 um 15:54 Uhr schrieb Thomas Petazzoni
<thomas.petazzoni@bootlin.com>:
>
> Using absolute paths within getdeveloperlib isn't very sensible, it
> makes a lot more sense to handle everything as relative paths from the
> top-level Buildroot source directory.
>
> parse_developers() is changed to no longer take the base path as
> argument: it is automatically calculated based on the location of
> utils/getdeveloperlib.py. Then, the rest of the logic is adjusted to
> use relative paths, and prepend them with the base "brpath" when
> needed.
>
> This commit allows pkg-stats to report correct developers information
> even when executed from an out of tree directory.

Just tried this but either I do not completely understand what it
should do or there is an issue.

I do as follows:

--- >8 ---
# mkdir b
# cd b
# make ../buildroot/ defconfig
# ../buildroot/support/scripts/pkg-stats -c --json out.json
make: *** Keine Regel, um „show-info“ zu erstellen.  Schluss.
Traceback (most recent call last):
  File "../buildroot/support/scripts/pkg-stats", line 1008, in <module>
    __main__()
  File "../buildroot/support/scripts/pkg-stats", line 962, in __main__
    package_list = get_config_packages()
  File "../buildroot/support/scripts/pkg-stats", line 337, in
get_config_packages
    js = json.loads(subprocess.check_output(cmd))
  File "/usr/lib/python3.7/subprocess.py", line 395, in check_output
    **kwargs).stdout
  File "/usr/lib/python3.7/subprocess.py", line 487, in run
    output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make',
'--no-print-directory', 'show-info']' returned non-zero exit status 2.

--- >8 ---

>
> Before this patch:
>
> $ ~/buildroot/support/scripts/pkg-stats -p ipmitool --json out.json
> $ cat out.json | jq '.packages.ipmitool.developers'
> []
>
> $ cat out.json | jq '.defconfigs.stm32f469_disco'
> {
>   "name": "stm32f469_disco",
>   "path": "configs/stm32f469_disco_defconfig",
>   "developers": []
> }
>
> After this patch:
>
> $ ~/buildroot/support/scripts/pkg-stats -p ipmitool --json out.json
> $ cat out.json | jq '.packages.ipmitool.developers'
> [
>   "Floris Bos <bos@je-eigen-domein.nl>",
>   "Heiko Thiery <heiko.thiery@gmail.com>"
> ]
> $ cat out.json | jq '.defconfigs.stm32f469_disco'
> {
>   "name": "stm32f469_disco",
>   "path": "configs/stm32f469_disco_defconfig",
>   "developers": [
>     "Christophe Priouzeau <christophe.priouzeau@st.com>"
>   ]
> }

In-tree this change works as expected.

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Tested-by: Heiko Thiery <heiko.thiery@gmail.com>

> ---
>  support/scripts/pkg-stats |  2 +-
>  utils/get-developers      |  5 -----
>  utils/getdeveloperlib.py  | 26 ++++++++++++--------------
>  3 files changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
> index e472b67784..f6460cf6f2 100755
> --- a/support/scripts/pkg-stats
> +++ b/support/scripts/pkg-stats
> @@ -969,7 +969,7 @@ def __main__():
>      print("Build package list ...")
>      packages = get_pkglist(args.npackages, package_list)
>      print("Getting developers ...")
> -    developers = parse_developers(brpath)
> +    developers = parse_developers()
>      print("Build defconfig list ...")
>      defconfigs = get_defconfig_list()
>      for d in defconfigs:
> diff --git a/utils/get-developers b/utils/get-developers
> index 20272ed60b..e027c26562 100755
> --- a/utils/get-developers
> +++ b/utils/get-developers
> @@ -45,10 +45,6 @@ def __main__():
>          print("No action specified")
>          return
>
> -    # getdeveloperlib expects to be executed from the toplevel buildroot
> -    # directory, which is one level up from this script
> -    os.chdir(os.path.join(os.path.dirname(os.path.realpath(__file__)), '..'))
> -
>      devs = getdeveloperlib.parse_developers()
>      if devs is None:
>          sys.exit(1)
> @@ -75,7 +71,6 @@ def __main__():
>
>      # Handle the files action
>      if args.files is not None:
> -        args.files = [os.path.abspath(f) for f in args.files]
>          for dev in devs:
>              for f in args.files:
>                  if dev.hasfile(f):
> diff --git a/utils/getdeveloperlib.py b/utils/getdeveloperlib.py
> index f57f41887b..d7a90457ed 100644
> --- a/utils/getdeveloperlib.py
> +++ b/utils/getdeveloperlib.py
> @@ -6,6 +6,8 @@ import subprocess
>  import sys
>  import unittest
>
> +brpath = os.path.normpath(os.path.join(os.path.dirname(__file__), ".."))
> +
>  #
>  # Patch parsing functions
>  #
> @@ -94,14 +96,14 @@ def get_all_test_cases(suite):
>          yield (suite.__module__, suite.__class__.__name__)
>
>
> -def list_unittests(path):
> +def list_unittests():
>      """Use the unittest module to retreive all test cases from a given
>      directory"""
>      loader = unittest.TestLoader()
> -    suite = loader.discover(path)
> +    suite = loader.discover(os.path.join(brpath, "support", "testing"))
>      tests = {}
>      for module, test in get_all_test_cases(suite):
> -        module_path = os.path.join(path, *module.split('.'))
> +        module_path = os.path.join("support", "testing", *module.split('.'))
>          tests.setdefault(module_path, []).append('%s.%s' % (module, test))
>      return tests
>
> @@ -124,9 +126,7 @@ class Developer:
>          self.defconfigs = parse_developer_defconfigs(files)
>
>      def hasfile(self, f):
> -        f = os.path.abspath(f)
>          for fs in self.files:
> -            fs = os.path.abspath(fs)
>              if f.startswith(fs):
>                  return True
>          return False
> @@ -158,7 +158,7 @@ def parse_developer_packages(fnames):
>      patterns, and return a list of those packages."""
>      packages = set()
>      for fname in fnames:
> -        for root, dirs, files in os.walk(fname):
> +        for root, dirs, files in os.walk(os.path.join(brpath, fname)):
>              for f in files:
>                  path = os.path.join(root, f)
>                  if fname_get_package_infra(path):
> @@ -223,7 +223,7 @@ def parse_developer_runtime_tests(fnames):
>      # List all files recursively
>      for fname in fnames:
>          if os.path.isdir(fname):
> -            for root, _dirs, files in os.walk(fname):
> +            for root, _dirs, files in os.walk(os.path.join(brpath, fname)):
>                  all_files += [os.path.join(root, f) for f in files]
>          else:
>              all_files.append(fname)
> @@ -237,15 +237,13 @@ def parse_developer_runtime_tests(fnames):
>      return runtimes
>
>
> -def parse_developers(basepath=None):
> +def parse_developers():
>      """Parse the DEVELOPERS file and return a list of Developer objects."""
>      developers = []
>      linen = 0
> -    if basepath is None:
> -        basepath = os.getcwd()
>      global unittests
> -    unittests = list_unittests(os.path.join(basepath, 'support/testing'))
> -    with open(os.path.join(basepath, "DEVELOPERS"), "r") as f:
> +    unittests = list_unittests()
> +    with open(os.path.join(brpath, "DEVELOPERS"), "r") as f:
>          files = []
>          name = None
>          for line in f:
> @@ -259,11 +257,11 @@ def parse_developers(basepath=None):
>                  name = line[2:].strip()
>              elif line.startswith("F:"):
>                  fname = line[2:].strip()
> -                dev_files = glob.glob(os.path.join(basepath, fname))
> +                dev_files = glob.glob(os.path.join(brpath, fname))
>                  if len(dev_files) == 0:
>                      print("WARNING: '%s' doesn't match any file" % fname,
>                            file=sys.stderr)
> -                files += dev_files
> +                files += [os.path.relpath(f, brpath) for f in dev_files]
>              elif line == "":
>                  if not name:
>                      continue

Thank you.

--
Heiko
Thomas Petazzoni Nov. 20, 2020, 2:14 p.m. UTC | #2
Hello Heiko,

Thanks for the review and feedback!

On Fri, 20 Nov 2020 11:03:45 +0100
Heiko Thiery <heiko.thiery@gmail.com> wrote:

> I do as follows:
> 
> --- >8 ---  
> # mkdir b
> # cd b
> # make ../buildroot/ defconfig
> # ../buildroot/support/scripts/pkg-stats -c --json out.json
> make: *** Keine Regel, um „show-info“ zu erstellen.  Schluss.

The current folder must be a Builroot output directory:

$ mkdir foobar/
$ cd foobar/
$ make -C /path/to/buildroot O=$(pwd) blabla_defconfig
$ make pkg-stats
or
$ /path/to/buildroot/support/scripts/pkg-stats -c --json out.json

Or this should also work (but not tested)

$ cd /path/to/buildroot
$ make O=/output blabla_defconfig
$ make O=/output pkg-stats

In any case: "pkg-stats -c" runs "make show-info", so it needs to be
from a place where you can run Buildroot make targets.

Does this clarifies the situation ?

Thomas
Heiko Thiery Nov. 20, 2020, 2:26 p.m. UTC | #3
Hi Thomas,

Am Fr., 20. Nov. 2020 um 15:14 Uhr schrieb Thomas Petazzoni
<thomas.petazzoni@bootlin.com>:
>
> Hello Heiko,
>
> Thanks for the review and feedback!
>
> On Fri, 20 Nov 2020 11:03:45 +0100
> Heiko Thiery <heiko.thiery@gmail.com> wrote:
>
> > I do as follows:
> >
> > --- >8 ---
> > # mkdir b
> > # cd b
> > # make ../buildroot/ defconfig
> > # ../buildroot/support/scripts/pkg-stats -c --json out.json
> > make: *** Keine Regel, um „show-info“ zu erstellen.  Schluss.
>
> The current folder must be a Builroot output directory:
>
> $ mkdir foobar/
> $ cd foobar/
> $ make -C /path/to/buildroot O=$(pwd) blabla_defconfig
> $ make pkg-stats
> or
> $ /path/to/buildroot/support/scripts/pkg-stats -c --json out.json
>
> Or this should also work (but not tested)
>
> $ cd /path/to/buildroot
> $ make O=/output blabla_defconfig
> $ make O=/output pkg-stats
>
> In any case: "pkg-stats -c" runs "make show-info", so it needs to be
> from a place where you can run Buildroot make targets.
>
> Does this clarifies the situation ?

Oh yes .. that was my fault. I completely forgot to add the O=$PWD argument.

Forget my previous mail ;-)
diff mbox series

Patch

diff --git a/support/scripts/pkg-stats b/support/scripts/pkg-stats
index e472b67784..f6460cf6f2 100755
--- a/support/scripts/pkg-stats
+++ b/support/scripts/pkg-stats
@@ -969,7 +969,7 @@  def __main__():
     print("Build package list ...")
     packages = get_pkglist(args.npackages, package_list)
     print("Getting developers ...")
-    developers = parse_developers(brpath)
+    developers = parse_developers()
     print("Build defconfig list ...")
     defconfigs = get_defconfig_list()
     for d in defconfigs:
diff --git a/utils/get-developers b/utils/get-developers
index 20272ed60b..e027c26562 100755
--- a/utils/get-developers
+++ b/utils/get-developers
@@ -45,10 +45,6 @@  def __main__():
         print("No action specified")
         return
 
-    # getdeveloperlib expects to be executed from the toplevel buildroot
-    # directory, which is one level up from this script
-    os.chdir(os.path.join(os.path.dirname(os.path.realpath(__file__)), '..'))
-
     devs = getdeveloperlib.parse_developers()
     if devs is None:
         sys.exit(1)
@@ -75,7 +71,6 @@  def __main__():
 
     # Handle the files action
     if args.files is not None:
-        args.files = [os.path.abspath(f) for f in args.files]
         for dev in devs:
             for f in args.files:
                 if dev.hasfile(f):
diff --git a/utils/getdeveloperlib.py b/utils/getdeveloperlib.py
index f57f41887b..d7a90457ed 100644
--- a/utils/getdeveloperlib.py
+++ b/utils/getdeveloperlib.py
@@ -6,6 +6,8 @@  import subprocess
 import sys
 import unittest
 
+brpath = os.path.normpath(os.path.join(os.path.dirname(__file__), ".."))
+
 #
 # Patch parsing functions
 #
@@ -94,14 +96,14 @@  def get_all_test_cases(suite):
         yield (suite.__module__, suite.__class__.__name__)
 
 
-def list_unittests(path):
+def list_unittests():
     """Use the unittest module to retreive all test cases from a given
     directory"""
     loader = unittest.TestLoader()
-    suite = loader.discover(path)
+    suite = loader.discover(os.path.join(brpath, "support", "testing"))
     tests = {}
     for module, test in get_all_test_cases(suite):
-        module_path = os.path.join(path, *module.split('.'))
+        module_path = os.path.join("support", "testing", *module.split('.'))
         tests.setdefault(module_path, []).append('%s.%s' % (module, test))
     return tests
 
@@ -124,9 +126,7 @@  class Developer:
         self.defconfigs = parse_developer_defconfigs(files)
 
     def hasfile(self, f):
-        f = os.path.abspath(f)
         for fs in self.files:
-            fs = os.path.abspath(fs)
             if f.startswith(fs):
                 return True
         return False
@@ -158,7 +158,7 @@  def parse_developer_packages(fnames):
     patterns, and return a list of those packages."""
     packages = set()
     for fname in fnames:
-        for root, dirs, files in os.walk(fname):
+        for root, dirs, files in os.walk(os.path.join(brpath, fname)):
             for f in files:
                 path = os.path.join(root, f)
                 if fname_get_package_infra(path):
@@ -223,7 +223,7 @@  def parse_developer_runtime_tests(fnames):
     # List all files recursively
     for fname in fnames:
         if os.path.isdir(fname):
-            for root, _dirs, files in os.walk(fname):
+            for root, _dirs, files in os.walk(os.path.join(brpath, fname)):
                 all_files += [os.path.join(root, f) for f in files]
         else:
             all_files.append(fname)
@@ -237,15 +237,13 @@  def parse_developer_runtime_tests(fnames):
     return runtimes
 
 
-def parse_developers(basepath=None):
+def parse_developers():
     """Parse the DEVELOPERS file and return a list of Developer objects."""
     developers = []
     linen = 0
-    if basepath is None:
-        basepath = os.getcwd()
     global unittests
-    unittests = list_unittests(os.path.join(basepath, 'support/testing'))
-    with open(os.path.join(basepath, "DEVELOPERS"), "r") as f:
+    unittests = list_unittests()
+    with open(os.path.join(brpath, "DEVELOPERS"), "r") as f:
         files = []
         name = None
         for line in f:
@@ -259,11 +257,11 @@  def parse_developers(basepath=None):
                 name = line[2:].strip()
             elif line.startswith("F:"):
                 fname = line[2:].strip()
-                dev_files = glob.glob(os.path.join(basepath, fname))
+                dev_files = glob.glob(os.path.join(brpath, fname))
                 if len(dev_files) == 0:
                     print("WARNING: '%s' doesn't match any file" % fname,
                           file=sys.stderr)
-                files += dev_files
+                files += [os.path.relpath(f, brpath) for f in dev_files]
             elif line == "":
                 if not name:
                     continue