Changeset View
Standalone View
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: | ||||
bcooksley: Coding style issue: elsewhere i've used descriptive variable names which indicate what it's… | |||||
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 | ||||
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" | ||||
Could you add some comments, indicating what this function expects to receive and what it provides as a result? bcooksley: Could you add some comments, indicating what this function expects to receive and what it… | |||||
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 | } | ||||
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: | ||||
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" | ||||
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 | ||||
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 | ||||
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)) | ||||
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/? | |||||
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 | | ||||
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. | |||||
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… | |||||
knauss: use path.write_text() | |||||
81 | tmpKeyword = None | ||||
82 | for arg in values: | ||||
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. 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 | | ||||
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 | } | ||||
bcooksley: Coding style: descriptive variables please | |||||
93 | | ||||
94 | RELINE = re.compile("^\s*(?P<keyword>[^(]+)\s*\(\s*(?P<args>.*)\s*\)\s*$") | ||||
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: | ||||
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 | ||||
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 | | ||||
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) | ||||
lbeltrame: What if this fails for some reason? Or is this guaranteed never to? | |||||
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) | ||||
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) | ||||
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 | ||||
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') | ||||
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) |
Coding style issue: elsewhere i've used descriptive variable names which indicate what it's contents should be.
Can you change this to match?