<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122232/">https://git.reviewboard.kde.org/r/122232/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 27th, 2015, 8:55 p.m. UTC, <b>Matthew Dawson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Unforunately, this cause test system failures in the the kconfigskeletontest test suite. I'm not sure why this should create issues there.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, I have a partial solution that avoids creating a full KSharedConfig. Instead, only globalData needs to be called in KConfig to ensure the object is created as soon as possible (without needing a QCoreApplication). This should avoid having any other globals created before.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, this causes a crash later. The root of the issue is in the KConfigPrivate constructor, line 98. Creating the QLocale after QCoreApplication is gone is apparently broken as well. I'm not sure how to solve that, maybe cache the value we need from QLocale, similar to caching the arguments?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also, I think the copied over KConfig test case really needs to match the KSharedConfig test, as using KSharedConfig can mask the problem since KSharedConfig's pointer is cached, avoiding the KConfig constructor.</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Oh. Indeed. I can explain the test failure: that test was removing the config file and then creating a KConfigSkeleton around it (which was supposed to be the instanciation of the KSharedConfig, starting from "no file"). With my change, the KSharedConfig already exists before that, we delete the file underneath, but it keeps existing, so the KConfigSkeleton also uses the data from that now-deleted file. It's more or less like refcounting - we kept an old object alive by creating it upfront.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You're right, we could just store the args instead, for a more minimal change.
But I tried it and I hit the same QLocale issue as you did. Argh.
We're really doing too much in that global object dtor. Wrapping up is ok, creating new stuff is not.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I tried working around the issue by keeping a KSharedConfig::Ptr data member in the Tester class - which sounds like an extremely good idea for apps to do, in order to reuse it rather than recreate and reparse at shutdown time. This takes care of that issue, but another one then shows up:
~Tester -> ~KSharedConfig -> KConfig::sync -> QLockFile::lock -> "QApplication::qAppName: Please instantiate the QApplication object first".
We didn't hit that in kdelibs4 either because I wrote QLockFile for Qt5, and KLockFile didn't call qAppName, it used a refcounting KComponentData.
I guess I could add an if (qApp) in QLockFile, but we're really going into fragile territories. We're definitely not just fixing a regression anymore since kdelibs4 asserted with this testcase.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The KSharedConfig testcase doesn't avoid the KConfig constructor, the way it's written. The caching is refcounting, it only actually caches if there's something storing a Ptr somewhere (hence my suggestion to actually do that, because apps should really do that, especially since they don't have KComponentData doing that for them anymore). I'm fine with any change you want to see in the KConfig testcase though, but let's sort out the main testcase first.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm currently thinking Revision 2 of this patch was the best, it fixed the regression compared to kdelibs4 without introducing new issues.</p></pre>
<br />
<p>- David</p>
<br />
<p>On January 27th, 2015, 8:10 a.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and Matthew Dawson.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Jan. 27, 2015, 8:10 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">kconfig_in_global_object.cpp comes from kdelibs4support
(after porting to Q_GLOBAL_STATIC)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">ksharedconfig_in_global_object.cpp is new (but works with kdelibs4)
and reproduces Albert's KgDifficulty testcase.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>autotests/kconfig_in_global_object.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/ksharedconfig_in_global_object.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/core/kconfig.cpp <span style="color: grey">(782e9714521234a3e3d8f3a788967e5c9a40f38a)</span></li>
<li>src/core/ksharedconfig.cpp <span style="color: grey">(e059b87a1cc1df50693a668ef791e7a61050ef88)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(b91f754b705fc87bb8b729bea72fbb5f7d427ace)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122232/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>