Differential D11339 Diff 31370 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 pathlib
7​import re
8​import subprocess
9​import tempfile
10​from typing import Dict, List, Union
11​
12​from helperslib import Packages
13​
14​import logging
15​logging.basicConfig(level=logging.DEBUG)
16​
17​# Parse the command line arguments we've been given
18​parser = argparse.ArgumentParser(description='Utility to create abi checker tarballs.')
19​parser.add_argument('--buildlog', type=str, required=True)
20​parser.add_argument('--environment', type=str, required=True)
21​arguments = parser.parse_args()
22​
23​def cmake_parser(lines: List) -> Dict:
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​ """A small cmake parser, if you search for a better solution think about using
25​ a propper one based on ply.
26​ see https://salsa.debian.org/qt-kde-team/pkg-kde-jenkins/blob/master/hooks/prepare/cmake_update_deps
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​
28​ But in our case we are only interessed in two keywords and do not need many features.
29​ we return a dictonary with keywords and targets.
30​ set(VAR "123")
31​ -> variables["VAR"]="123"
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​ set_target_properties(TARGET PROPERTIES PROP1 A B PROP2 C D)
33​ -> targets = {
34​ "PROP1":["A","B"],
35​ "PROP2":["C","D"],
36​ }
bcooksleyUnsubmitted
Not Done

Same as above.

bcooksley: Same as above.
37​ """
38​
39​ variables = {} # type: Dict[str,str]
40​ targets = defaultdict(lambda:defaultdict(list)) # type: Dict[str, Dict[str, List[str]]]
41​
42​ ret = {
43​ "variable": variables,
44​ "targets": targets,
45​ }
46​
47​ def parse_set(args: str) -> None:
48​ """process set lines and updates the variables directory:
bcooksleyUnsubmitted
Done

Coding style: same issue as for line 25.

bcooksley: Coding style: same issue as for line 25.
49​ set(VAR 1.2.3) -> args = ["VAR", "1.2.3"]
50​ and we set variable["VAR"] = "1.2.3"
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​ """
52​ _args = args.split()
53​ if len(_args) == 2:
54​ name, value = _args
55​ variables[name] = value
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​
57​ def parse_set_target_properties(args: str) -> None:
58​ """process set_target_properties cmake lines and update the targets directory
59​ all argiments of set_target_properties are given in the args parameter as list.
60​ as cmake using keyword val1 val2 we need to save the keyword so long we detect
bcooksleyUnsubmitted
Done

Coding style: brackets.

bcooksley: Coding style: brackets.
61​ a next keyword.
62​
63​ args[0] is the target we want to update
64​ args[1] must be PROPERTIES
65​ """
66​ name, properties, *values = args.split()
67​ target = targets[name]
68​ if not properties == "PROPERTIES":
69​ logging.warning("unknown line: %s"%(args))
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​
71​ # Known set_target_properties keywords
72​ keywords = [
73​ "IMPORTED_LINK_DEPENDENT_LIBRARIES_DEBUG",
74​ "IMPORTED_LOCATION_DEBUG",
75​ "IMPORTED_SONAME_DEBUG",
76​ "INTERFACE_INCLUDE_DIRECTORIES",
77​ "INTERFACE_LINK_LIBRARIES",
78​ "INTERFACE_COMPILE_OPTIONS",
79​ ]
80​
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​ tmpKeyword = None
82​ for arg in values:
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​ if arg in keywords:
84​ tmpKeyword = target[arg]
85​ continue
86​ tmpKeyword.append(arg)
87​
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​ #Keywords we want to react on
89​ keywords = {
90​ "set": parse_set,
91​ "set_target_properties": parse_set_target_properties,
92​ }
bcooksleyUnsubmitted
Done

Coding style: descriptive variables please

bcooksley: Coding style: descriptive variables please
93​
94​ RELINE = re.compile("^\s*(?P<keyword>[^(]+)\s*\(\s*(?P<args>.*)\s*\)\s*$")
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​ for line in lines:
96​ m = RELINE.match(line)
97​ if m and m.group('keyword') in keywords:
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​ keywords[m.group('keyword')](m.group('args'))
99​
100​ return ret
101​
102​
103​class Library:
104​ def __init__(self, name: str) -> None:
105​
106​ # name of the library
107​ self.name = name # type: str
bcooksleyUnsubmitted
Done

Coding style: braces

bcooksley: Coding style: braces
108​
109​ # The raw cmake Parser output, available for debug porpuse
110​ # see cmake_parser function for the return value
111​ self.__parser_output = None # type: Union[Dict, None]
112​
113​ # version of the library
114​ # created/documented within runCMake function
115​
116​ # targets the targets of the libary ( existing so files)
117​ # created/documented within runCMake function
118​
119​
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​ def __repr__(self) -> str:
121​ return "<Library \"{self.name}\">".format(self=self) # replace with f-String in python 3.6
122​
123​ def runCMake(self) -> None:
124​ """Create a CMakeLists.txt to detect the headers, version and library path"""
125​ with tempfile.TemporaryDirectory() as d:
126​
127​ # Create a CMakeLists.txt that depends on the requested library
128​ cmakeFile = (pathlib.Path(d)/"CMakeLists.txt")
129​ cmakeFile.write_text("find_package({self.name} CONFIG REQUIRED)\n".format(self=self)) # replace with f-String in python 3.6
130​
131​ proc = subprocess.Popen(['cmake', '.', '--trace-expand'], cwd=d, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, check=True)
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​
133​ # search only for lines that are the output of the specifc cmake files
134​ self.__mlines = [] # type: List[str]
135​ # cmake prefixes outout with the name of the file, filter only lines with interessting files
136​ retarget=re.compile('.*/{self.name}(Targets[^/]*|Config[^/]*)\.cmake\(\d+\):\s*(.*)$'.format(self=self)) # replace with f-String in python 3.6
137​ for line in proc.stderr:
138​ theLine = line.decode("utf-8")
139​ m = retarget.match(theLine)
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​ if m:
141​ mline = m.group(2)
142​ self.__mlines.append(mline)
143​
144​ self.__parser_output = cmake_parser(self.__mlines)
bcooksleyUnsubmitted
Done

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

bcooksley: Coding style: can the variable be made more descriptive please?
145​
146​ # version of the library
147​ self.version = self.__parser_output["variables"]["PACKAGE_VERSION"] # type: str
148​
149​ # targets the targets of the libary ( existing so files)
150​ # a dict with keys, SONAME = the SONAME of the lib
151​ # path = path of the library
152​ # include_dirs = the header files for the library
153​ self.targets = {} # type: Dict
154​
155​ def inclDirs(args: List[str]) -> List[str]:
156​ """ cmake using ";" to seperate different paths
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…
157​ split the paths and make a unique list of all paths (do not add paths multiple times)
158​ """
159​ d = [] # type: List[str]
160​ for arg in args:
161​ d += arg.split(";")
162​ return d
163​
164​ for t,value in self.__parser_output["targets"].items():
165​ target = {
166​ "SONAME": re.search("\.([\d]*)$",value["IMPORTED_SONAME_DEBUG"][0]).group(1),
167​ "path": value["IMPORTED_LOCATION_DEBUG"][0],
168​ "include_dirs": inclDirs(value["INTERFACE_INCLUDE_DIRECTORIES"]),
169​ }
170​ self.targets[t]=target
171​
172​ def createABIDump(self) -> None:
173​ """run abi-compliance-checker (acc) to create a ABIDump tar gz
174​
175​ First we need to construct a input file for acc, see xml variable.
176​ After that we can run acc with the constructed file.
177​ """
178​ if not self.__parser_output:
179​ self.runCMake()
180​
181​ version = self.version
182​ headers = [] # type: List[str]
183​ libs = [] # type: List[str]
184​ for target in self.targets.values():
185​ for i in target['include_dirs']:
186​ # ignore general folders, as there are no lib specific headers are placed
187​ if i == '/usr/include' or i.endswith("/KF5"):
188​ continue
189​ if not i in headers:
190​ headers.append(i)
191​ if not target['path'] in libs:
192​ libs.append(target['path'])
193​
194​ xml = """
195​<version>{version}</version>
196​<headers>
197​{headers}
198​</headers>
199​<libs>
200​{libs}
201​</libs>
202​""".format(version=version, headers="\n".join(headers), libs="\n".join(libs)) # replace with f-String in Python 3.6
203​ with open("{version}.xml".format(version=version),"w") as f: # replace with f-String in python 3.6
204​ f.write(xml)
205​
206​ # acc is compatible for C/C++ as Qt using C++11 and -fPic we need to set the gcc settings explitly
207​ subprocess.check_call(["abi-compliance-checker", "-gcc-options", "-std=c++11 -fPIC", "-l", self.name, "--dump",f.name])
208​
209​
210​# search in buildlog for the Installing/Up-to-date lines where we installnig the <name>Config.cmake files.
211​# with this we get a complete List of installed libraries.
212​
213​#List of all libraries
214​libs = []
215​reline = re.compile("^-- (Installing|Up-to-date): .*/([^/]*)Config\.cmake$")
216​with open(arguments.buildlog) as f:
217​ for line in f.readlines():
218​ m = reline.match(line)
219​ if m:
220​ lib = Library(m.group(2))
221​ libs.append(lib)
222​
223​# Initialize the archive manager
224​ourArchive = Packages.Archive(arguments.environment, 'ABIReference', usingCache = False)
225​
226​# Determine which SCM revision we are storing
227​# This will be embedded into the package metadata which might help someone doing some debugging
228​# GIT_COMMIT is set by Jenkins Git plugin, so we can rely on that for most of our builds
229​scmRevision = ''
230​if os.getenv('GIT_COMMIT') != '':
231​ scmRevision = os.getenv('GIT_COMMIT')
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…
232​
233​for lib in libs:
234​ lib.createABIDump()
235​
236​ fileName = "abi_dumps/{name}/{name}_{version}.abi.tar.gz".format(name=lib.name,version=lib.version) # can replaced with f-String in python 3.6
237​ scmRevision = max([t['SONAME'] for t in lib.targets.values()]) # a more hackish way, to save the SONAME in the metadata
238​ packageName = "{name}_{scmRevision}".format(name=lib.name, scmRevision=scmRevision)
239​ ourArchive.storePackage(packageName, fileName, scmRevision)