Add a script to create abi.tar.gz for all the libs.

Authored by knauss on Mar 14 2018, 8:18 PM.

Diff Detail

R857 CI System Tooling
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.
knauss requested review of this revision.Mar 14 2018, 8:18 PM
knauss created this revision.

This looks okay for the most part, mostly just coding style fixes.
Only big question is the one on splitting out the code into a module in helperslib/ as I made in one comment. Thoughts about that?


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


Coding style: please see for how braces for maps/arrays are usually formatted.


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)


Same as above.


Coding style: same issue as for line 25.


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


Coding style: brackets.


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


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


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?


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


Coding style: descriptive variables please


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


Coding style: braces


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


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


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


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

knauss updated this revision to Diff 29982.Mar 20 2018, 9:57 AM
knauss marked 14 inline comments as done.
  • add documentation & fixups
knauss added inline comments.Mar 20 2018, 9:58 AM

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.


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.


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

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


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

lbeltrame added inline comments.

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.


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).


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.


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

knauss updated this revision to Diff 31272.Apr 4 2018, 10:20 AM

some more cleanups

knauss updated this revision to Diff 31279.Apr 4 2018, 10:31 AM

cleanups and things mentioned by luca.

knauss marked 4 inline comments as done.

see also D11915 for the metadata extesion.


use path.write_text()


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

knauss marked 2 inline comments as done.Apr 5 2018, 7:41 AM
knauss updated this revision to Diff 31370.Apr 5 2018, 10:48 AM

some small fixes and merge.

This revision was not accepted when it landed; it landed in state Needs Review.Jun 6 2018, 9:29 AM
This revision was automatically updated to reflect the committed changes.