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 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 = { | ||||
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… | |||||
23 | "variables":{}, | ||||
24 | "targets":defaultdict(lambda:defaultdict(list)) | ||||
25 | } | ||||
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. | |||||
26 | variables = ret['variables'] | ||||
27 | targets = ret['targets'] | ||||
28 | | ||||
29 | def parse_set(args): | ||||
30 | args = args.split() | ||||
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… | |||||
31 | if len(args) == 2: | ||||
32 | variables[args[0]] = args[1] | ||||
33 | | ||||
34 | def parse_set_target_properties(args): | ||||
35 | args = args.split() | ||||
bcooksley: Same as above. | |||||
36 | target = targets[args[0]] | ||||
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 | ] | ||||
bcooksley: Coding style: same issue as for line 25. | |||||
48 | | ||||
49 | t = None | ||||
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… | |||||
50 | for arg in args[2:]: | ||||
51 | if arg in keywords: | ||||
52 | t=target[arg] | ||||
53 | continue | ||||
54 | t.append(arg) | ||||
55 | | ||||
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 | } | ||||
bcooksley: Coding style: brackets. | |||||
60 | RELINE = re.compile("^\s*(?P<keyword>[^(]+)\s*\(\s*(?P<args>.*)\s*\)\s*$") | ||||
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 | | ||||
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… | |||||
69 | class Library: | ||||
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: | ||||
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() | |||||
80 | f.write("find_package({self.name} CONFIG REQUIRED)\n".format(self=self)) | ||||
81 | proc = subprocess.Popen(['cmake', '.', '--trace-expand'], cwd=d, stdout=subprocess.DEVNULL, stderr=subprocess.PIPE) | ||||
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… | |||||
82 | self.mlines = [] | ||||
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) | ||||
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… | |||||
87 | if m: | ||||
88 | mline = m.group(2) | ||||
89 | self.mlines.append(mline) | ||||
90 | | ||||
91 | self.p = cmake_parser(self.mlines) | ||||
bcooksley: Coding style: descriptive variables please | |||||
92 | | ||||
93 | self.version = self.p["variables"]["PACKAGE_VERSION"] | ||||
94 | self.targets = {} | ||||
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): | ||||
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… | |||||
97 | d = [] | ||||
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 | } | ||||
bcooksley: Coding style: braces | |||||
107 | self.targets[t]=target | ||||
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"): | ||||
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… | |||||
119 | continue | ||||
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} | ||||
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 | </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 = [] | ||||
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… | |||||
139 | | ||||
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) | ||||
bcooksley: Coding style: can the variable be made more descriptive please? | |||||
144 | if m: | ||||
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()])) | ||||
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… | |||||
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… |
Coding style issue: elsewhere i've used descriptive variable names which indicate what it's contents should be.
Can you change this to match?