Refactor KConfigXT
ClosedPublic

Authored by tcanabrava on Dec 23 2019, 11:48 PM.

Details

Summary

The current KConfigXT compiler is in a sad state:
It's a massive file with loads of global variables that handle state, the generator is done within the main() function and it seems to have grown organically. There are no classes to separate logic / state / generation, what exists is code that generates code from a xml / ini pair, but it's hard to even discover what a bit of code is doing. The code istyle is C++ / Java from the nineties, which is not bad per see but it also uses quite a few things that are going to be deprecated in Qt 6 so I'm also taking the time make the code more streamlined with newer code style (no iterators, lambdas, auto usage, etc).

The code that generates the files simplly pushes strings to a text stream, and it's hard to figure out when something starts or something ends: for instance, the code that generates the Constructor has more than sixty lines of code englobing some nested if - for - if - for constructs.

Currently the code is "done" - there's one bug that I still need to find & fix regarding Translations, but the rest seems sane.
The current testcode generates incorrect *whitespaces* regarding the old code (there's some parts that I feel that it's important to fix before merging, but overall, the whitespace changes are not bad and easier to handle, old code had a hand-counted amount of spaces before each line, new code has a function whitespace() that adds the current-and-correct amount of whitespaces based on indentation level that you start by startScope() and ends with endScope(). rest of the code still needs to be ported to it.

I plan to fix the testcases whitespace by manually adding them, I'v fougth with the code for a while and added a few hacks there but I don't want to make the code hackish again.

New code is not perfect by any means, but is a good step in the right direction.

This code tries to Separate the compiler code into many different files / classes to be more obvious what's happening, and each class also has many helper methods to minimize copypaste.

  • CodeGenerator: Has base code for the header and source files that can be shared
  • HeaderGenerator: Logic for generating the header file
  • SourceGenerator: Logic for generating the source file
  • KcfgParser: Logic for parsing the kcfg file and extracting the information from the Xml file
  • CommonStructs: a header that contains the structs that are currently used everywhere.
  • KConfigParameters: (was CfgConfig - ConfigConfig, wat) - Has information passed via the kcfgc file
  • kcfg_compiler - will be renamed to main - start the other classes and generates the files.

This code here currently has the begining of this separation, with the CodeGenerator and the HeaderGenerator in a ~good~ state, but unfinished.

Test Plan
  • Run the test cases,
  • Compare the diffs generated by the testcases and fix in the code the errors / differences
  • Run and compare real kde source with the new and old generators to look for errors

Diff Detail

Repository
R237 KConfig
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
dfaure added inline comments.Jan 13 2020, 8:57 AM
autotests/kconfig_compiler/kconfigcompiler_test.cpp
182

I meant QVERIFY2(diffFile.open(...), ...).
No need to make a separate call to isOpen().

src/kconfig_compiler/KCFGXmlParser.h
47 ↗(On Diff #73345)

typo ("something")

src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
3

Is this really entirely new code?

Please propagate the copyright of the file you copied this from.

I can agree that .h files are brand new, but not .cpp files

src/kconfig_compiler/KConfigCommonStructs.h
15

Oh. I thought it meant "remove SignalArguments members from Param".

Can you at least clarify the comment by saying "TODO merge Param and SignalArguments" or (in front of SignalArguments) "TODO use Param instead"?

src/kconfig_compiler/KConfigHeaderGenerator.h
79

I'm no expert but I'm sure there's an option for this somewhere....

src/kconfig_compiler/KConfigSourceGenerator.cpp
60

There are still some qDebugs, please do a patch-wide search.

225

no space after '!'

392

newline before '{'

src/kconfig_compiler/kconfig_compiler.cpp
772

^

This revision now requires changes to proceed.Jan 13 2020, 8:57 AM
tcanabrava marked 10 inline comments as done.
  • Simplify List creation
  • Fail with the diff file name
  • Simplify Code
  • Fix Typo
  • Remove SignalArgument for Param
  • Nitpicks and Include fixes

Took the time to nuke the SignalArguments and use Param instead, easier than I initially tougth.

autotests/kconfig_compiler/kconfigcompiler_test.cpp
182

ups :)

tcanabrava updated this revision to Diff 73557.Jan 14 2020, 6:53 PM
  • Add missing copyright holders
tcanabrava marked an inline comment as done.Jan 15 2020, 10:17 AM
tcanabrava added inline comments.
autotests/kconfig_compiler/kconfigcompiler_test.cpp
129

This is done now within the appendFileDiff function.

dfaure accepted this revision.Jan 16 2020, 8:51 AM
This revision is now accepted and ready to land.Jan 16 2020, 8:51 AM
This revision was automatically updated to reflect the committed changes.

This morning it was found e.g. on opensuse build server (but reproduced by me locally) that this change broke KMyMoney & KDevelop, who now start to fail e.g. with (using VERBOSE=1 make)

[ 38%] Generating kcfg_custombuildsystemconfig.h, kcfg_custombuildsystemconfig.cpp
cd /home/koder/Kode/kdegit/build/kf5/extragear/kdevelop/kdevelop/plugins/custom-buildsystem && /home/koder/System/opt/kf5/lib64/libexec/kf5/kconfig_compiler_kf5 /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/custom-buildsystem/kcfg_custombuildsystemconfig.kcfg /home/koder/Kode/kdegit/kf5/extragear/kdevelop/kdevelop/plugins/custom-buildsystem/kcfg_custombuildsystemconfig.kcfgc -d /home/koder/Kode/kdegit/build/kf5/extragear/kdevelop/kdevelop/plugins/custom-buildsystem/
No entries.
make[2]: *** [plugins/custom-buildsystem/CMakeFiles/kdevcustombuildsystem.dir/build.make:71: plugins/custom-buildsystem/kcfg_custombuildsystemconfig.h] Fehler 1

See https://phabricator.kde.org/source/kdevelop/browse/master/plugins/custom-buildsystem/ for the respecive kfg files.

No time to investigate myself, but given this breaks existing codebase it should be quickly investigated or reverted for now. Do not shoot the messenger :)

Argh, I wanted to say "please do a full kdesrc-build of all modules" before approving, and forgot to do that.

where "full" would also need to mean "clean fresh build dirs" :) so that the codegenerator is triggered to be run, as the current cmake code seems to not inject a build dep between generator instance and generated files, so a new version does not trigger regeneration. Something that could also get a fix perhaps, IIRC other tools have such a dep.

We can revert, and I’ll fix the full build. Doing that now.

We can revert, and I’ll fix the full build. Doing that now.

I Just reverted this and I'm working on a full build of kde using kdesrc-build --refresh-build, I'll reopen this patch when *all* projects build sucessfully, with a unittest for each one that broke.

tcanabrava reopened this revision.Jan 17 2020, 10:06 AM

Reopening for review :)

This revision is now accepted and ready to land.Jan 17 2020, 10:06 AM
tcanabrava requested review of this revision.Jan 17 2020, 10:06 AM

where "full" would also need to mean "clean fresh build dirs" :) so that the codegenerator is triggered to be run, as the current cmake code seems to not inject a build dep between generator instance and generated files, so a new version does not trigger regeneration. Something that could also get a fix perhaps, IIRC other tools have such a dep.

D26723 now up to fix the automatic rebuild on new versions of kconfig_compiler.

I Just reverted this and I'm working on a full build of kde using kdesrc-build --refresh-build, I'll reopen this patch when *all* projects build sucessfully, with a unittest for each one that broke.

Thanks. Don't forget to change the title when done ;)

who knew? This actually was not a false positive: the kdevelop build failure was a bug in kdevelop.
I already opened a ticket: https://invent.kde.org/kde/kdevelop/merge_requests/90
but at the same time I added code to handle the case of broken / empty kconfigxt.

tcanabrava updated this revision to Diff 73846.Jan 18 2020, 9:14 PM
  • Revert "Revert "WIP: Refactor KConfigXT""
  • Add Reference files for Broken KDevelop Configuration
  • Fix generating of empty configuration files

Aparently the kmymoney issue was the same: empty kconfig file. I just successfully compiled kdevelop and kmymoney.
I'll let the computer to compile the whole kde applications from scratch tonigth to see if it will fail somewhere.

dfaure requested changes to this revision.Jan 19 2020, 9:09 AM

Please remove the "WIP" from the commit log (and from phabricator, which only gets updated if you do arc diff --verbatim)

This revision now requires changes to proceed.Jan 19 2020, 9:09 AM
tcanabrava retitled this revision from WIP: Refactor KConfigXT to Refactor KConfigXT .Jan 19 2020, 3:23 PM

@dfaure I tried to remove the WIP from the history but I'm worried that will force git push --force as I merged this with the wip before.
any hints?

dfaure accepted this revision.Jan 19 2020, 5:54 PM

Don't force push. It's not what I meant, and I don't think it's even permitted in kde repos.
The WIP commit was reverted anyway (so it was indeed work in progress, haha).
The comment from Christoph and myself was about your next commit :)

This revision is now accepted and ready to land.Jan 19 2020, 5:54 PM

I can confirm that force pushes are blocked within KDE Git repositories, although we will be opening up work/ branches at some point which will permit it (and be un-notified in terms of hook processing)

I checked https://cgit.kde.org/kconfig.git/tree/src/kconfig_compiler/kcfg.xsd and it says

<xsd:element name="group" maxOccurs="unbounded" >
                   <xsd:complexType>
                       <xsd:sequence>
                           <xsd:element name="entry" maxOccurs="unbounded">

According to google default of minOccurs is 1 so empty files are indeed not allowed

I checked https://cgit.kde.org/kconfig.git/tree/src/kconfig_compiler/kcfg.xsd and it says

<xsd:element name="group" maxOccurs="unbounded" >
                   <xsd:complexType>
                       <xsd:sequence>
                           <xsd:element name="entry" maxOccurs="unbounded">

According to google default of minOccurs is 1 so empty files are indeed not allowed

rigth now only kdevelop (wtha patch open) and kmymoney are doing those (and in both cases they commented out / removed the entries a long time ago) - being 'better safe than sorry' I'll leave as is till kf6

I'm admittedly very late to the party, so mostly pointing out stylistic things which are left. Since we're touching many lines anyway it's a good point in time to get that improved since the original was very foreign in some way.

src/kconfig_compiler/KCFGXmlParser.cpp
446 ↗(On Diff #73846)

Space before & not after

src/kconfig_compiler/KCFGXmlParser.h
41 ↗(On Diff #73846)

Please put the opening curly brace on its own line.

As far as the class name goes I'd propose KCfgXmlParser or KConfigXmlParser to make sure we have a consistent capitalization.

43 ↗(On Diff #73846)

Please have the space before & and not after.

src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
47

Space before & not after

81

This is an odd indentation logic, is it to stay close to the original? (which I'd actually support, just trying to understand where that's coming from).

130

Space before & not after

src/kconfig_compiler/KConfigCodeGeneratorBase.h
48

Space before & not after (and following lines as well).

57

Ditto

66

This screams to be const

101

I'd tend to put those private and provide only protected const getters. Just to make sure we don't temper with them by mistake, those instances are in control of the base class after all.

Also I wonder if it wouldn't be the right opportunity to give those members a "m_" prefix? It's the more common style in KF I think.

src/kconfig_compiler/KConfigCommonStructs.h
127

Hell yes, that's will be a nice second step.

173

Space before * not after

src/kconfig_compiler/KConfigHeaderGenerator.cpp
40

Ditto

383

Ditto

447

Ditto

477

Ditto

src/kconfig_compiler/KConfigHeaderGenerator.h
46

Well, you know it. ;-)

73

Ditto

src/kconfig_compiler/KConfigSourceGenerator.cpp
32

Ditto

315

Ditto

344

Ditto

src/kconfig_compiler/KConfigSourceGenerator.h
46

Ditto

74

Ditto

src/kconfig_compiler/KConfigXTParameters.h
27 ↗(On Diff #73846)

Typo, missing N (in both lines)

37 ↗(On Diff #73846)

I'd go for "Xt"

src/kconfig_compiler/kconfig_compiler.cpp
653

Space before & not after

@ervin since they are just nitpicks I'll fix in code and land the patch.

Well a couple of them are a bit more invasive than just dealing with spaces but fair enough.

I wouldn't mind an answer to my question though just to feel less stupid: https://phabricator.kde.org/D26202#inline-151385

tcanabrava marked 26 inline comments as done.Wed, Jan 22, 10:53 AM
tcanabrava added inline comments.
src/kconfig_compiler/KConfigCodeGeneratorBase.cpp
81

it's to stay close to the original. originally all the indentation was done manually, but first 4 spaces and later 2 spaces. This code is bogus and I would like to get rid of it spporting only one indentation style. I'm adding a TODO: for that here.

tcanabrava updated this revision to Diff 74118.Wed, Jan 22, 2:11 PM
tcanabrava marked an inline comment as done.
tcanabrava retitled this revision from Refactor KConfigXT to Refactor KConfigXT.
  • Rebase
This revision was automatically updated to reflect the committed changes.