Differential D11339 Diff 31272 helpers/create-abi-bump.py

Changeset View

Standalone View

View Options

helpers/create-abi-bump.py

  • This file was added.
1​#!/usr/bin/python3
2​
3​import argparse
4​from collections import defaultdict
5​import os
6​import re
7​import subprocess
8​import tempfile
9​
10​from helperslib import Packages
11​
12​import logging
13​logging.basicConfig(level=logging.DEBUG)
14​
15​# Parse the command line arguments we've been given
16​parser = argparse.ArgumentParser(description='Utility to create abi checker tarballs.')
17​parser.add_argument('--buildlog', type=str, required=True)
18​parser.add_argument('--environment', type=str, required=True)
19​arguments = parser.parse_args()
20​
21​def cmake_parser(lines):
22​ ret = {
23​ "variables":{},
bcooksleyUnsubmitted
Not Done

Coding style issue: elsewhere i've used descriptive variable names which indicate what it's contents should be.
Can you change this to match?

bcooksley: Coding style issue: elsewhere i've used descriptive variable names which indicate what it's…
knaussAuthorUnsubmitted
Not Done

well match is mostly used for the return value of an re.match. ret is a very clear name -as it is the return variable.

knauss: well match is mostly used for the return value of an re.match. ret is a very clear name -as it…
24​ "targets":defaultdict(lambda:defaultdict(list))
25​ }
26​ variables = ret['variables']
bcooksleyUnsubmitted
Done

Coding style: please see gather-jobs.py for how braces for maps/arrays are usually formatted.

bcooksley: Coding style: please see gather-jobs.py for how braces for maps/arrays are usually formatted.
27​ targets = ret['targets']
28​
29​ def parse_set(args):
30​ args = args.split()
31​ if len(args) == 2:
bcooksleyUnsubmitted
Not Done

Could you add some comments, indicating what this function expects to receive and what it provides as a result?
Also, should these functions be split off to the module level? (in case somewhere else needs to do something similar later on)

bcooksley: Could you add some comments, indicating what this function expects to receive and what it…
knaussAuthorUnsubmitted
Not Done

no module level do not make sense, as those inner functions use local variables targets/variables. Additional this function depends on the part at line 56.

knauss: no module level do not make sense, as those inner functions use local variables…
32​ variables[args[0]] = args[1]
33​
34​ def parse_set_target_properties(args):
35​ args = args.split()
36​ target = targets[args[0]]
bcooksleyUnsubmitted
Not Done

Same as above.

bcooksley: Same as above.
37​ if not args[1] == "PROPERTIES":
38​ logging.warning("unknown line: %s"%(args))
39​
40​
41​ keywords=["IMPORTED_LINK_DEPENDENT_LIBRARIES_DEBUG",
42​ "IMPORTED_LOCATION_DEBUG",
43​ "IMPORTED_SONAME_DEBUG",
44​ "INTERFACE_INCLUDE_DIRECTORIES",
45​ "INTERFACE_LINK_LIBRARIES",
46​ "INTERFACE_COMPILE_OPTIONS",
47​ ]
48​
bcooksleyUnsubmitted
Done

Coding style: same issue as for line 25.

bcooksley: Coding style: same issue as for line 25.
49​ t = None
50​ for arg in args[2:]:
bcooksleyUnsubmitted
Done

Can we have some descriptive variables here as well please, along with some comments on what we're trying to achieve?

bcooksley: Can we have some descriptive variables here as well please, along with some comments on what…
51​ if arg in keywords:
52​ t=target[arg]
53​ continue
54​ t.append(arg)
55​
lbeltrameUnsubmitted
Done

I know it's mostly "nitpicking" but it would be best to assing meaningful names to args[0] and [1] where it makes sense:

if len(_args) == 2:
   variable, value = _args
   variables[variable] = value

and so on.

lbeltrame: I know it's mostly "nitpicking" but it would be best to assing meaningful names to args[0] and…
56​ keywords={
57​ "set": parse_set,
58​ "set_target_properties": parse_set_target_properties,
59​ }
60​ RELINE = re.compile("^\s*(?P<keyword>[^(]+)\s*\(\s*(?P<args>.*)\s*\)\s*$")
bcooksleyUnsubmitted
Done

Coding style: brackets.

bcooksley: Coding style: brackets.
61​ for line in lines:
62​ m = RELINE.match(line)
63​ if m and m.group('keyword') in keywords:
64​ keywords[m.group('keyword')](m.group('args'))
65​
66​ return ret
67​
68​
69​class Library:
bcooksleyUnsubmitted
Not Done

Should this class, and perhaps the above function be split off into a module into helperslib/?

bcooksley: Should this class, and perhaps the above function be split off into a module into helperslib/?
knaussAuthorUnsubmitted
Not Done

I don't think so as this is clearly not a clean way to parse cmake - as it is only a dirty hackish way to parse. A better start for a good parser would be https://salsa.debian.org/qt-kde-team/pkg-kde-jenkins/blob/master/hooks/prepare/cmake_update_deps

but only if we plan to parse more than this simple two keywords.

knauss: I don't think so as this is clearly not a clean way to parse cmake - as it is only a dirty…
70​ def __init__(self, name):
71​ self.name = name
72​ self.p = None
73​
74​ def __repr__(self):
75​ return "<Library \"{self.name}\">".format(self=self)
76​
77​ def runCMake(self):
78​ with tempfile.TemporaryDirectory() as d:
79​ with open(d+"/CMakeLists.txt","w") as f:
80​ f.write("find_package({self.name} CONFIG REQUIRED)\n".format(self=self))
bcooksleyUnsubmitted
Done

Please use os.path.join() to ensure all edgecases around paths being merged are handled.

bcooksley: Please use os.path.join() to ensure all edgecases around paths being merged are handled.
lbeltrameUnsubmitted
Done

Since pathlib is used below, I would argue to use Path instead, it's much nicer and better than the os.path functions (at the cost of some overhead).

lbeltrame: Since `pathlib` is used below, I would argue to use `Path` instead, it's much nicer and better…
knaussAuthorUnsubmitted
Not Done

use path.write_text()

knauss: use path.write_text()
81​ proc = subprocess.Popen(['cmake', '.', '--trace-expand'], cwd=d, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE)
82​ self.mlines = []
bcooksleyUnsubmitted
Done

Coding style: as we've moved on to the next unit of work there should be a newline to separate the writing of the CMakeLists.txt file and the invocation of CMake.
Can you also please add some comments describing what we're populating the CMakeLists file with and our objective there?

bcooksley: Coding style: as we've moved on to the next unit of work there should be a newline to separate…
83​ self.libs=defaultdict(dict)
84​ for line in proc.stderr:
85​ theLine = line.decode("utf-8")
86​ m = re.match('.*/{self.name}(Targets[^/]*|Config[^/]*)\.cmake\(\d+\):\s*(.*)$'.format(self=self), theLine)
87​ if m:
bcooksleyUnsubmitted
Done

Coding style / readability: the formatted version of the regular expression should be stored in a variable then that should be run through re.match

bcooksley: Coding style / readability: the formatted version of the regular expression should be stored in…
88​ mline = m.group(2)
89​ self.mlines.append(mline)
90​
91​ self.p = cmake_parser(self.mlines)
92​
bcooksleyUnsubmitted
Done

Coding style: descriptive variables please

bcooksley: Coding style: descriptive variables please
93​ self.version = self.p["variables"]["PACKAGE_VERSION"]
94​ self.targets = {}
lbeltrameUnsubmitted
Not Done

Can we compile this just once at the start of the module? I don't think it's going to be much of a problem.

lbeltrame: Can we compile this just once at the start of the module? I don't think it's going to be much…
95​
96​ def inclDirs(args):
97​ d = []
bcooksleyUnsubmitted
Done

As above: should this function perhaps be separated out for ease of readability and future reuse?

bcooksley: As above: should this function perhaps be separated out for ease of readability and future…
98​ for arg in args:
99​ d += arg.split(";")
100​ return d
101​
102​ for t,value in self.p["targets"].items():
103​ target={"SONAME": re.search("\.([\d]*)$",value["IMPORTED_SONAME_DEBUG"][0]).group(1),
104​ "path": value["IMPORTED_LOCATION_DEBUG"][0],
105​ "include_dirs": inclDirs(value["INTERFACE_INCLUDE_DIRECTORIES"]),
106​ }
107​ self.targets[t]=target
bcooksleyUnsubmitted
Done

Coding style: braces

bcooksley: Coding style: braces
108​
109​ def createABIDump(self):
110​ if not self.p:
111​ self.runCMake()
112​
113​ version = self.version
114​ headers = []
115​ libs = []
116​ for target in self.targets.values():
117​ for i in target['include_dirs']:
118​ if i == '/usr/include' or i.endswith("/KF5"):
119​ continue
bcooksleyUnsubmitted
Done

Can we get a comment describing why we're ignoring system prefixes and anything relating to KDE Frameworks?

bcooksley: Can we get a comment describing why we're ignoring system prefixes and anything relating to KDE…
120​ if not i in headers:
121​ headers.append(i)
122​ if not target['path'] in libs:
123​ libs.append(target['path'])
124​
125​ xml = """
126​<version>{version}</version>
127​<headers>
128​{headers}
129​</headers>
130​<libs>
131​{libs}
lbeltrameUnsubmitted
Done

What if this fails for some reason? Or is this guaranteed never to?

lbeltrame: What if this fails for some reason? Or is this guaranteed never to?
knaussAuthorUnsubmitted
Not Done

i added check=True, so it fails with a python error.

knauss: i added check=True, so it fails with a python error.
132​</libs>
133​""".format(version=version, headers="\n".join(headers), libs="\n".join(libs))
134​ with open("{version}.xml".format(version=version),"w") as f:
135​ f.write(xml)
136​ subprocess.call(["abi-compliance-checker", "-gcc-options", "-std=c++11 -fPIC", "-l", self.name, "--dump",f.name])
137​
138​libs = []
139​
bcooksleyUnsubmitted
Done

Could we get some comments describing what we're trying to achieve and how we're going to do that please?

bcooksley: Could we get some comments describing what we're trying to achieve and how we're going to do…
140​RELINE = re.compile("^-- (Installing|Up-to-date): .*/([^/]*)Config\.cmake$")
141​with open(arguments.buildlog) as f:
142​ for line in f.readlines():
143​ m = RELINE.match(line)
144​ if m:
bcooksleyUnsubmitted
Done

Coding style: can the variable be made more descriptive please?

bcooksley: Coding style: can the variable be made more descriptive please?
145​ lib = Library(m.group(2))
146​ libs.append(lib)
147​
148​
149​
150​# Initialize the archive manager
151​ourArchive = Packages.Archive(arguments.environment, 'ABIReference', usingCache = False)
152​
153​for lib in libs:
154​ lib.createABIDump()
155​ ourArchive.storePackage(lib.name, "abi_dumps/{name}/{name}_{version}.abi.tar.gz".format(name=lib.name,version=lib.version), max([t['SONAME'] for t in lib.targets.values()]))
bcooksleyUnsubmitted
Done

Coding style: please split the generation of the filename and revision identifier out to their own lines for readability.

bcooksley: Coding style: please split the generation of the filename and revision identifier out to their…
knaussAuthorUnsubmitted
Done

Don't know if we should implement a propper way to save for data in the metadata file in package.

knauss: Don't know if we should implement a propper way to save for data in the metadata file in…