Differential D11339 Diff 35670 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​import os
3​import re
4​import logging
5​import pathlib
6​import argparse
7​import tempfile
8​import subprocess
9​from collections import defaultdict
10​from typing import Dict, List, Union
11​
12​from helperslib import Packages, EnvironmentHandler
13​
14​# Make sure logging is ready to go
15​logging.basicConfig(level=logging.DEBUG)
16​
17​def cmake_parser(lines: List) -> Dict:
18​ """A small cmake parser, if you search for a better solution think about using
19​ a proper one based on ply.
20​ see https://salsa.debian.org/qt-kde-team/pkg-kde-jenkins/blob/master/hooks/prepare/cmake_update_deps
21​
22​ But in our case we are only interested in two keywords and do not need many features.
23​ we return a dictonary with keywords and targets.
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​ set(VAR "123")
25​ -> variables["VAR"]="123"
26​ set_target_properties(TARGET PROPERTIES PROP1 A B PROP2 C D)
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 = {
28​ "PROP1":["A","B"],
29​ "PROP2":["C","D"],
30​ }
31​ """
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​
33​ variables = {} # type: Dict[str,str]
34​ targets = defaultdict(lambda:defaultdict(list)) # type: Dict[str, Dict[str, List[str]]]
35​
36​ ret = {
bcooksleyUnsubmitted
Not Done

Same as above.

bcooksley: Same as above.
37​ "variables": variables,
38​ "targets": targets,
39​ }
40​
41​ def parse_set(args: str) -> None:
42​ """process set lines and updates the variables directory:
43​ set(VAR 1.2.3) -> args = ["VAR", "1.2.3"]
44​ and we set variable["VAR"] = "1.2.3"
45​ """
46​ _args = args.split()
47​ if len(_args) == 2:
48​ name, value = _args
bcooksleyUnsubmitted
Done

Coding style: same issue as for line 25.

bcooksley: Coding style: same issue as for line 25.
49​ variables[name] = value
50​
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​ def parse_set_target_properties(args: str) -> None:
52​ """process set_target_properties cmake lines and update the targets directory
53​ all argiments of set_target_properties are given in the args parameter as list.
54​ as cmake using keyword val1 val2 we need to save the keyword so long we detect
55​ a next keyword.
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​ args[0] is the target we want to update
58​ args[1] must be PROPERTIES
59​ """
60​ name, properties, *values = args.split()
bcooksleyUnsubmitted
Done

Coding style: brackets.

bcooksley: Coding style: brackets.
61​ target = targets[name]
62​ if not properties == "PROPERTIES":
63​ logging.warning("unknown line: %s"%(args))
64​
65​ # Known set_target_properties keywords
66​ keywords = [
67​ "IMPORTED_LINK_DEPENDENT_LIBRARIES_DEBUG",
68​ "IMPORTED_LOCATION_DEBUG",
69​ "IMPORTED_SONAME_DEBUG",
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​ "INTERFACE_INCLUDE_DIRECTORIES",
71​ "INTERFACE_LINK_LIBRARIES",
72​ "INTERFACE_COMPILE_OPTIONS",
73​ "INTERFACE_COMPILE_DEFINITIONS",
74​ ]
75​
76​ tmpKeyword = None
77​ for arg in values:
78​ if arg in keywords:
79​ tmpKeyword = target[arg]
80​ continue
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.append(arg)
82​
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​ #Keywords we want to react on
84​ keywords = {
85​ "set": parse_set,
86​ "set_target_properties": parse_set_target_properties,
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​
89​ RELINE = re.compile("^\s*(?P<keyword>[^(]+)\s*\(\s*(?P<args>.*)\s*\)\s*$")
90​ for line in lines:
91​ m = RELINE.match(line)
92​ if m and m.group('keyword') in keywords:
bcooksleyUnsubmitted
Done

Coding style: descriptive variables please

bcooksley: Coding style: descriptive variables please
93​ keywords[m.group('keyword')](m.group('args'))
94​
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​ return ret
96​
97​# Wrapper class to represent a library we have found
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​# This class stores information on the library in question and assists in extracting information concerning it
99​class Library:
100​ # Make sure we initialize everything we are going to need
101​ def __init__(self, name: str) -> None:
102​ # name of the library
103​ self.name = name # type: str
104​
105​ # The raw cmake Parser output, available for debugging purposes
106​ # see cmake_parser function for the return value
107​ self.__parser_output = None # type: Union[Dict, None]
bcooksleyUnsubmitted
Done

Coding style: braces

bcooksley: Coding style: braces
108​
109​ # Provide a helpful description of the object (to ease debugging)
110​ def __repr__(self) -> str:
111​ return "<Library \"{self.name}\">".format(self=self) # replace with f-String in python 3.6
112​
113​ # Execute CMake to gather the information we need
114​ def runCMake(self, runtimeEnvironment) -> None:
115​ """Create a CMakeLists.txt to detect the headers, version and library path"""
116​
117​ # Prepare to gather the information we need
118​ self.__mlines = [] # type: List[str]
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​ # To avoid contaminating the directory we are being run in, make sure we are in a temporary directory
121​ # This will also allow us to succeed if we are run from the build direcotry
122​ with tempfile.TemporaryDirectory() as d:
123​ # Create an appropriate CMakeLists.txt which searches for the library in question
124​ cmakeFile = (pathlib.Path(d)/"CMakeLists.txt")
125​ cmakeFile.write_text("find_package({self.name} CONFIG REQUIRED)\n".format(self=self)) # replace with f-String in python 3.6
126​
127​ # Now run CMake and ask it to process the CMakeLists.txt file we just generated
128​ # We want it to run in trace mode so we can examine the log to extract the information we need
129​ proc = subprocess.Popen(['cmake', '.', '--trace-expand'], cwd=d, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE, env=runtimeEnvironment)
130​
131​ # cmake prefixes outout with the name of the file, filter only lines with interessting files
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​ retarget = re.compile( '.*/{self.name}(Targets[^/]*|Config[^/]*)\.cmake\(\d+\):\s*(.*)$'.format(self=self) ) # replace with f-String in python 3.6
133​
134​ # Start processing the output of CMake, one line at a time
135​ for line in proc.stderr:
136​ # Make sure it is UTF-8 formatted
137​ theLine = line.decode("utf-8")
138​
139​ # Did we find a CMake line we were interested in?
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​ m = retarget.match(theLine)
141​ if m:
142​ # Extract the information from that and store it for further processing
143​ mline = m.group(2)
144​ self.__mlines.append(mline)
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​ # Process the information we've now gathered
147​ self.__parser_output = cmake_parser(self.__mlines)
148​ self.parser_output = self.__parser_output
149​ self.mlines = self.__mlines
150​
151​ # Extract the version number of the library for easier use
152​ self.version = self.__parser_output["variables"]["PACKAGE_VERSION"] # type: str
153​
154​ # targets the targets of the libary ( existing so files)
155​ # a dict with keys, SONAME = the SONAME of the lib
156​ # path = path of the library
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​ # include_dirs = the header files for the library
158​ self.targets = {} # type: Dict
159​
160​ # Helper Function to parse CMake formatted include directory lists
161​ def parseIncludeDirs(args: List[str]) -> List[str]:
162​ """ cmake using ";" to seperate different paths
163​ split the paths and make a unique list of all paths (do not add paths multiple times)
164​ """
165​ d = [] # type: List[str]
166​ for arg in args:
167​ d += arg.split(";")
168​ return d
169​
170​ # Process the various targets our parser found
171​ for t,value in self.__parser_output["targets"].items():
172​ # Particularly, we want to extract:
173​ # Library names (sonames)
174​ # The path to the CMake library package
175​ # Any include directories specified by the CMake library package
176​ target = {
177​ "SONAME": re.search("\.([\d]*)$",value["IMPORTED_SONAME_DEBUG"][0]).group(1),
178​ "path": value["IMPORTED_LOCATION_DEBUG"][0],
179​ "include_dirs": parseIncludeDirs(value["INTERFACE_INCLUDE_DIRECTORIES"]),
180​ }
181​ self.targets[t]=target
182​
183​ def createABIDump(self, runtimeEnvironment=None) -> None:
184​ """run abi-compliance-checker (acc) to create a ABIDump tar gz
185​
186​ First we need to construct a input file for acc, see xml variable.
187​ After that we can run acc with the constructed file.
188​ """
189​
190​ # Make sure we have a valid runtime environment for CMake and abi-compliance-checker
191​ if runtimeEnvironment is None:
192​ runtimeEnvironment = os.environ
193​
194​ # If we haven't yet run CMake, do so
195​ # Otherwise we won't have anything to give to abi-compliance-checker
196​ if not self.__parser_output:
197​ self.runCMake(runtimeEnvironment)
198​
199​ # Start preparations to run abi-compliance-checker
200​ # Gather the information we'll need to write the XML configuration file it uses
201​ version = self.version
202​ headers = [] # type: List[str]
203​ libs = [] # type: List[str]
204​
205​ # From the target information we previously collected...
206​ # Grab the list of libraries and include headers for abi-compliance-checker
207​ for target in self.targets.values():
208​ # Check each include directory to see if we need to add it....
209​ for i in target['include_dirs']:
210​ # ignore general folders, as there are no lib specific headers are placed
211​ if i == '/usr/include' or i.endswith("/KF5"):
212​ continue
213​
214​ # Otherwise, if we don't already have it - add it to the list!
215​ if not i in headers:
216​ headers.append(i)
217​
218​ # If the library path isn't in the list, then we should add it to the list
219​ if not target['path'] in libs:
220​ libs.append(target['path'])
221​
222​ # Now we can go ahead and generate the XML file for abi-compliance-checker
223​ xml = """
224​ <version>{version}</version>
225​ <headers>
226​ {headers}
227​ </headers>
228​ <libs>
229​ {libs}
230​ </libs>
231​ """.format(version=version, headers="\n".join(headers), libs="\n".join(libs)) # replace with f-String in Python 3.6
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​ # Write the generated XML out to a file to pass to abi-compliance-checker
234​ # We will give this to abi-compliance-checker using it's --dump parameter
235​ with open("{version}.xml".format(version=version),"w") as f: # replace with f-String in python 3.6
236​ f.write(xml)
237​
238​ # acc is compatible for C/C++ as Qt using C++11 and -fPic we need to set the gcc settings explitly
239​ subprocess.check_call(["abi-compliance-checker", "-gcc-options", "-std=c++11 -fPIC", "-l", self.name, "--dump", f.name], env=runtimeEnvironment)
240​
241​# Parse the command line arguments we've been given
242​parser = argparse.ArgumentParser(description='Utility to create abi checker tarballs.')
243​parser.add_argument('--buildLog', type=str, required=True)
244​parser.add_argument('--environment', type=str, required=True)
245​parser.add_argument('--usingInstall', type=str, required=True)
246​arguments = parser.parse_args()
247​
248​# Make sure we have an environment ready for executing commands
249​buildEnvironment = EnvironmentHandler.generateFor( installPrefix=arguments.usingInstall )
250​
251​# Get ready to start searching for libraries
252​foundLibraries = []
253​
254​# Search in the build log for the Installing/Up-to-date lines where we install the <name>Config.cmake files.
255​# From this we get a complete List of installed libraries.
256​cmakeConfig = re.compile("^-- (Installing|Up-to-date): .*/([^/]*)Config\.cmake$")
257​with open(arguments.buildLog, encoding='utf-8') as log:
258​ for line in log.readlines():
259​ match = cmakeConfig.match(line)
260​ if match:
261​ foundLibrary = Library( match.group(2) )
262​ foundLibraries.append(foundLibrary)
263​
264​# Initialize the archive manager
265​ourArchive = Packages.Archive(arguments.environment, 'ABIReference', usingCache = False)
266​
267​# Now we generate the ABI dumps for every library we have found
268​for library in foundLibraries:
269​ # Create the ABI Dump for this library
270​ library.createABIDump( runtimeEnvironment=buildEnvironment )
271​
272​ # Determine where the ABI Dump archive is located
273​ # This location is controlled by abi-compliance-checker, but follows a predictable pattern
274​ fileName = "abi_dumps/{name}/{name}_{version}.abi.tar.gz".format(name=library.name,version=library.version) # can replaced with f-String in python 3.6
275​
276​ # Determine the internal version of the library we have found
277​ # This is based off the CMake package metadata we read in above
278​ scmRevision = max([t['SONAME'] for t in library.targets.values()]) # a more hackish way, to save the SONAME in the metadata
279​
280​ # Create a name for this entry in the Package archive and store it there
281​ packageName = "{name}_{scmRevision}".format(name=lib.name, scmRevision=scmRevision)
282​ ourArchive.storePackage(packageName, fileName, scmRevision)