Move settings for abi-complience-checker to own yaml file.
ClosedPublic

Authored by knauss on Dec 19 2018, 12:32 AM.

Details

Summary

Some repos like akonadi-search need special settings for creating a
dump.

Diff Detail

Repository
R857 CI System Tooling
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
knauss requested review of this revision.Dec 19 2018, 12:32 AM
knauss created this revision.
bcooksley added inline comments.Dec 19 2018, 7:47 AM
helpers/create-abi-dump.py
233–236

We should probably document all the supported options somewhere central.
I'd suggest this is done as comments at the top of the config file.

276–277

Shouldn't this comment be updated to reflect the new arguments being passed to abi-compliance-checker?

303

Spelling: abi-complience-checker -> abi-compliance-checker

304

Code style: yaml.load( open(localMetadataPath) )

helpers/helperslib/Settings.py
5 ↗(On Diff #47813)

Could we follow the pattern from BuildSpecs here (which is similar, except it uses separate files per project/platform/etc)

Also, i'm wondering if this module should be called something different to follow the convention of BuildSpecs (although admittedly this could be used for other things as well), perhaps something like ToolingSettings?

10 ↗(On Diff #47813)

Unless 'default' means something special to a YAML indexer, won't this code fail to pick up the defaults?

24 ↗(On Diff #47813)

Won't this lead to platform keys being imported into the configuration/settings dict we return?

knauss updated this revision to Diff 47822.Dec 19 2018, 8:50 AM
knauss marked 5 inline comments as done.

updated.

knauss added inline comments.Dec 19 2018, 8:50 AM
helpers/helperslib/Settings.py
5 ↗(On Diff #47813)

can you rephrase this? I don't get it what do you want. Is it just renaming the function? or the filename?

hopefully this solotion is general enough to be used for more things.

10 ↗(On Diff #47813)

this works just fine. see line18 here we add default to the beginning, so the dict is initialized with the values of default entry. tested it for akonadi ( default values should been returned) and akonadi-search (special values are returned)

24 ↗(On Diff #47813)

yepp this is a side-effect, but it is just, that the dict has more entries.
Where do I get a list of platforms to strip those keys out afterwards? In helpers/check-platform.py the dict has this None:MacOSX entry (just add a MacOSX:MacOSX entry) - otherwise it is called allPlatforms :D should we move this to helperslib so we can access it from both places?

bcooksley added inline comments.Dec 20 2018, 8:19 AM
helpers/helperslib/Settings.py
5 ↗(On Diff #47813)

The first paragraph was referring to the function name. Usually we've followed a pattern of Class.load( file ) or equivalent. Settings.settings( ) doesn't really describe what it does (which the pattern of BuildSpecs does)

For the second paragraph, I was referring to the filename (which python treats as the module name).

Based on the code, I agree it should be usable for other things.

10 ↗(On Diff #47813)

I see, okay. I didn't see that at first as it was under the sorting all entries comment.
Could you move the comment beside it to it's own line above entries.insert() to make this clearer?

24 ↗(On Diff #47813)

That map in check-platforms is more to map things to an operating system.
In theory platform names can be any string and there isn't any rules around what is valid/invalid.

I'd suggest perhaps that the YAML gets structured something like this to solve this:

"kde/pim/akonadi-search":
  general:
    gcc_options:
     - "-std=c++11"
     - "-fPIC"
     - "-DQT_NO_KEYWORDS"
  "SUSEQt5.10":   # or overwrite the setting for one specific platform
    gcc_options:
     - "-bla"
knauss updated this revision to Diff 47883.Dec 20 2018, 10:29 AM

Rework complete ToolingSettings with a Loader class.

knauss marked 3 inline comments as done.Dec 20 2018, 10:32 AM
knauss added inline comments.
helpers/helperslib/Settings.py
10 ↗(On Diff #47813)

well I used a more elegant solution and used "*" entry in yaml file, than we don't need a special entry.

bcooksley accepted this revision.Dec 21 2018, 3:24 AM

Perfect, that works for me and makes it quite clear what's going on.

This revision is now accepted and ready to land.Dec 21 2018, 3:24 AM
This revision was automatically updated to reflect the committed changes.