[next,1/5] support/scripts/pkg-stats-new: rewrite in Python

This commit adds a new version of the pkg-stats script, rewritten in
Python. It is for now implemented in a separate file called,
pkg-stats-new, in order to make the diff easily readable. A future
commit will rename it to pkg-stats.

Compared to the existing shell-based pkg-stats script, the
functionality and output is basically the same. The main difference is
that the output no longer goes to stdout, but to the file passed as
argument using the -o option. This allows stdout to be used for more
debugging related information.

The way the script works is that a first function get_pkglist()
creates a dict associating package names with an instance of a
Package() object, containing basic information about the package. Then
a number of other functions (add_infra_info, add_pkg_make_info,
add_hash_info, add_patch_count, add_check_package_warnings) will
calculate additional information about packages, and fill in fields in
the Package objects.

calculate_stats() then calculates global statistics (how packages have
license information, how packages have a hash file, etc.). Finally,
dump_html() produces the HTML output, using a number of sub-functions.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 support/scripts/pkg-stats-new | 424 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 424 insertions(+)
 create mode 100755 support/scripts/pkg-stats-new

Hello,

On Thu, Feb 15, 2018 at 08:03 PM, Thomas Petazzoni wrote:
[snip]
> +++ b/support/scripts/pkg-stats-new
> @@ -0,0 +1,424 @@
> +#!/usr/bin/env python

You removed your copyright. Well, that is your choice.

[snip]
> +import sys

This is unused.
Can you run flake8 and fix this and the other 32 warnings?

> +
> +class Package:
> +    def __init__(self, name):
> +        self.name = name
> +        self.path = None
path could be passed to constructor:
    def __init__(self, name, path):
        self.name = name
        self.path = path

[snip]
> +def get_pkglist():
> +    WALK_USEFUL_SUBDIRS = ["boot", "linux", "package", "toolchain"]
> +    WALK_EXCLUDES = [ "boot/common.mk",
> +                      "linux/linux-ext-.*.mk",

Here is an improvement not mentioned in the commit log:
linux/linux-ext-aufs.mk (and any new extension) is now ignored.

[snip]
> +            p = Package(pkgname)
> +            p.path = pkgpath
path could be passed to constructor:
            p = Package(pkgname, pkgpath)

[snip]
> +def get_pkg_infra_info(pkgpath):
> +    infras = list()
> +    with open(pkgpath, 'r') as f:
> +        lines = f.readlines()
> +        for l in lines:
> +            match = INFRA_RE.match(l)
> +            if not match:
> +                continue
> +            infra = match.group(1)
> +            if infra.startswith("host-"):
> +                infras.append(("host", infra[5:]))
> +            else:
> +                infras.append(("target", infra))
> +    return infras
Many methods like this one could become part of the class:
class Package:
    def get_pkg_infra_info(self):
        infras = list()
        with open(self.path, 'r') as f:
            lines = f.readlines()
            for l in lines:
                match = INFRA_RE.match(l)
                if not match:
                    continue
                infra = match.group(1)
                if infra.startswith("host-"):
                    infras.append(("host", infra[5:]))
                else:
                    infras.append(("target", infra))
        self.infras = infras

But that's a considerable effort with small gain.
The script can always be refactored later.

[snip]
> +html_header = """
[snip]
> +td.version-good {
> +  background: #d2ffc4;
> +}
> +td.version-needs-update {
> +  background: #ff9a69;
> +}
> +td.version-unknown {
> + background: #ffd870;
> +}

These 3 do not belong to this patch.

> +</style>
> +<title>Statistics of Buildroot packages</title>
> +</head>
> +
> +<a href=\"#results\">Results</a><br/>

This link is broken.

[snip]
> +html_footer = """
> +</table>

dump_html
 - calls dump_html_stats that ends in f.write("</table>")
 - then calls f.write(html_footer)
so there will be an unneeded tag in the html.

[snip]
> +def dump_html_pkg(f, pkg):

This method generates a line 700kB long in the html.
I guess it was not your plan, based in the fact there are spaces to indent each
tag passed to write().
So you could append a newline to each write:

> +    f.write(" <tr>")
    f.write(" <tr>\n")

It makes the diff between 2 outputs easier (for debugging the script).

> +    f.write("  <td>%s</td>" % pkg.path[2:])
[snip]
> +def dump_html_all_pkgs(f, packages):
[snip]
> +    for name, pkg in sorted(packages.iteritems()):

This is sorting by the name of the .mk file, not the path, so just after the
page is loaded, the order is:
package/4th/4th.mk
...
package/aespipe/aespipe.mk
boot/afboot-stm32/afboot-stm32.mk

which is weird.

The use of the name as key (duplicating the info already inside the Package
object) also seems to serves only the purpose of sorting.
By adding 2 methods to the class, sort by path becomes easy:

@@ class Package:
+    def __eq__(self, other):
+        return self.path == other.path
+
+    def __lt__(self, other):
+        return self.path < other.path
@@ def get_pkglist():
-    packages = dict()
+    packages = list()
@@ def get_pkglist():
-            packages[pkgname] = p
+            packages.append(p)
@@ def add_pkg_make_info(packages):
-    for name, pkg in packages.iteritems():
-        var = pkgname_to_pkgvar(name)
+    for pkg in packages:
+        var = pkgname_to_pkgvar(pkg.name)
@@ many methods:
-    for name, pkg in packages.iteritems():
+    for pkg in packages:
@@ def dump_html_all_pkgs(f, packages):
-    for name, pkg in sorted(packages.iteritems()):
+    for pkg in sorted(packages):

> +        dump_html_pkg(f, pkg)
> +    f.write("</table>")
> +
> +def dump_html_stats(f, stats):
> +    f.write("<table>")

In this method the absence of newline for each write is less problematic. The
line in the output html is 1.5kB long.
Not a problem for the browser to render of course, but it seems good to have
the newlines when debugging the script.

[snip]
> +def dump_html(packages, stats, output):
> +    with open(output, 'w') as f:
> +        f.write(html_header)
> +        dump_html_all_pkgs(f, packages)
> +        dump_html_stats(f, stats)

In the previous version of the script, after the stats table, the timestamp for
the creation of the html and the sha1 are displayed.
This seems useful to keep, to debug the server side because one can see quickly
if the script is running in the expected schedule and in the correct sha1.
For the timestamp, maybe this is enough: str(datetime.datetime.utcnow())

> +        f.write(html_footer)
[snip]

Regards,
Ricardo

Hello,

First of all, thanks a lot for your review. Definitely very nice to
have someone look at this script in detail.

On Sun, 18 Feb 2018 23:48:29 -0300, Ricardo Martincoski wrote:

> > +++ b/support/scripts/pkg-stats-new
> > @@ -0,0 +1,424 @@
> > +#!/usr/bin/env python  
> 
> You removed your copyright. Well, that is your choice.

I just started with an empty script, and it remained this way, I'll add
one.

> [snip]
> > +import sys  
> 
> This is unused.
> Can you run flake8 and fix this and the other 32 warnings?

Will do.

> > +class Package:
> > +    def __init__(self, name):
> > +        self.name = name
> > +        self.path = None  
> path could be passed to constructor:
>     def __init__(self, name, path):
>         self.name = name
>         self.path = path

ACK.

> [snip]
> > +def get_pkglist():
> > +    WALK_USEFUL_SUBDIRS = ["boot", "linux", "package", "toolchain"]
> > +    WALK_EXCLUDES = [ "boot/common.mk",
> > +                      "linux/linux-ext-.*.mk",  
> 
> Here is an improvement not mentioned in the commit log:
> linux/linux-ext-aufs.mk (and any new extension) is now ignored.

Indeed, will fix.

> > +def get_pkg_infra_info(pkgpath):
> > +    infras = list()
> > +    with open(pkgpath, 'r') as f:
> > +        lines = f.readlines()
> > +        for l in lines:
> > +            match = INFRA_RE.match(l)
> > +            if not match:
> > +                continue
> > +            infra = match.group(1)
> > +            if infra.startswith("host-"):
> > +                infras.append(("host", infra[5:]))
> > +            else:
> > +                infras.append(("target", infra))
> > +    return infras  
> Many methods like this one could become part of the class:
> class Package:
>     def get_pkg_infra_info(self):
>         infras = list()
>         with open(self.path, 'r') as f:
>             lines = f.readlines()
>             for l in lines:
>                 match = INFRA_RE.match(l)
>                 if not match:
>                     continue
>                 infra = match.group(1)
>                 if infra.startswith("host-"):
>                     infras.append(("host", infra[5:]))
>                 else:
>                     infras.append(("target", infra))
>         self.infras = infras
> 
> But that's a considerable effort with small gain.
> The script can always be refactored later.

I don't entirely agree: some of those functions don't apply to one
package object, they apply to the whole set of package objects.
Therefore, semantically, there are not methods of the Package() class,
as they don't manipulate just that object.

The main problem is with getting the license, license files and version
information. Indeed, this cannot be done as a method of a single
package, without slowing down the script significantly. Right now, we
are doing a single "make VARS=%_LICENSE printvars" invocation, and
parsing the output. This gives us the full list of licenses for all
packages, in just one make invocation. If we do this once per-package,
we pay the cost of running make + parsing time of all Buildroot
makefiles for the 2000+ packages, which isn't reasonable. One option
here would have to simply cache this value somewhere, and have the
method just use the cached value. Do you have a suggestion to handle
this in a beautiful way ?

> These 3 do not belong to this patch.

ACK.

> > +</style>
> > +<title>Statistics of Buildroot packages</title>
> > +</head>
> > +
> > +<a href=\"#results\">Results</a><br/>  
> 
> This link is broken.

I'll fix.

> [snip]
> > +html_footer = """
> > +</table>  
> 
> dump_html
>  - calls dump_html_stats that ends in f.write("</table>")
>  - then calls f.write(html_footer)
> so there will be an unneeded tag in the html.

Will fix.

> [snip]
> > +def dump_html_pkg(f, pkg):  
> 
> This method generates a line 700kB long in the html.
> I guess it was not your plan, based in the fact there are spaces to indent each
> tag passed to write().
> So you could append a newline to each write:

Yes, absolutely. I started by using print()s, but then changed to
writing to a file, and forgot to add the \n.

> > +    f.write("  <td>%s</td>" % pkg.path[2:])  
> [snip]
> > +def dump_html_all_pkgs(f, packages):  
> [snip]
> > +    for name, pkg in sorted(packages.iteritems()):  
> 
> This is sorting by the name of the .mk file, not the path, so just after the
> page is loaded, the order is:
> package/4th/4th.mk
> ...
> package/aespipe/aespipe.mk
> boot/afboot-stm32/afboot-stm32.mk
> 
> which is weird.

Right.

> The use of the name as key (duplicating the info already inside the Package
> object) also seems to serves only the purpose of sorting.
> By adding 2 methods to the class, sort by path becomes easy:
> 
> @@ class Package:
> +    def __eq__(self, other):
> +        return self.path == other.path
> +
> +    def __lt__(self, other):
> +        return self.path < other.path
> @@ def get_pkglist():
> -    packages = dict()
> +    packages = list()
> @@ def get_pkglist():
> -            packages[pkgname] = p
> +            packages.append(p)
> @@ def add_pkg_make_info(packages):
> -    for name, pkg in packages.iteritems():
> -        var = pkgname_to_pkgvar(name)
> +    for pkg in packages:
> +        var = pkgname_to_pkgvar(pkg.name)
> @@ many methods:
> -    for name, pkg in packages.iteritems():
> +    for pkg in packages:
> @@ def dump_html_all_pkgs(f, packages):
> -    for name, pkg in sorted(packages.iteritems()):
> +    for pkg in sorted(packages):

I'll have a look at the code you propose here. I understand __eq__ and
__lt__, but not (yet) the other changes.

> > +        dump_html_pkg(f, pkg)
> > +    f.write("</table>")
> > +
> > +def dump_html_stats(f, stats):
> > +    f.write("<table>")  
> 
> In this method the absence of newline for each write is less problematic. The
> line in the output html is 1.5kB long.
> Not a problem for the browser to render of course, but it seems good to have
> the newlines when debugging the script.

Sure.

> [snip]
> > +def dump_html(packages, stats, output):
> > +    with open(output, 'w') as f:
> > +        f.write(html_header)
> > +        dump_html_all_pkgs(f, packages)
> > +        dump_html_stats(f, stats)  
> 
> In the previous version of the script, after the stats table, the timestamp for
> the creation of the html and the sha1 are displayed.
> This seems useful to keep, to debug the server side because one can see quickly
> if the script is running in the expected schedule and in the correct sha1.
> For the timestamp, maybe this is enough: str(datetime.datetime.utcnow())

Totally correct, I'll re-add that.

Thanks!

Thomas

Hello,

On Mon, Feb 19, 2018 at 05:57 PM, Thomas Petazzoni wrote:

[snip]
>> > +def get_pkg_infra_info(pkgpath):
>> > +    infras = list()
>> > +    with open(pkgpath, 'r') as f:
>> > +        lines = f.readlines()
>> > +        for l in lines:
>> > +            match = INFRA_RE.match(l)
>> > +            if not match:
>> > +                continue
>> > +            infra = match.group(1)
>> > +            if infra.startswith("host-"):
>> > +                infras.append(("host", infra[5:]))
>> > +            else:
>> > +                infras.append(("target", infra))
>> > +    return infras  
>> Many methods like this one could become part of the class:
>> class Package:
>>     def get_pkg_infra_info(self):
>>         infras = list()
>>         with open(self.path, 'r') as f:
>>             lines = f.readlines()
>>             for l in lines:
>>                 match = INFRA_RE.match(l)
>>                 if not match:
>>                     continue
>>                 infra = match.group(1)
>>                 if infra.startswith("host-"):
>>                     infras.append(("host", infra[5:]))
>>                 else:
>>                     infras.append(("target", infra))
>>         self.infras = infras
>> 
>> But that's a considerable effort with small gain.
>> The script can always be refactored later.
> 
> I don't entirely agree: some of those functions don't apply to one
> package object, they apply to the whole set of package objects.
> Therefore, semantically, there are not methods of the Package() class,
> as they don't manipulate just that object.
> 
> The main problem is with getting the license, license files and version
> information. Indeed, this cannot be done as a method of a single
> package, without slowing down the script significantly. Right now, we
> are doing a single "make VARS=%_LICENSE printvars" invocation, and
> parsing the output. This gives us the full list of licenses for all
> packages, in just one make invocation. If we do this once per-package,
> we pay the cost of running make + parsing time of all Buildroot
> makefiles for the 2000+ packages, which isn't reasonable. One option
> here would have to simply cache this value somewhere, and have the
> method just use the cached value. Do you have a suggestion to handle
> this in a beautiful way ?

Unfortunately no.
Let's keep it as is.

Regards,
Ricardo