Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated April 4, 2015, 11:45 a.m.) Status -- This change has been marked as submitted. Review request for KDE Frameworks and Matthew Dawson. Changes --- Submitted with commit ee599bfa17f7e56b0db1a4afbbfe929ec90f2c9d by David Faure to branch master. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Feb. 21, 2015, 8:19 p.m., Matthew Dawson wrote: Everything looked good, but when I tried to rerun the tests to make sure everything is ok the kconfig_in_global_object test I'm getting an abort with the same error message. Backtrace: ``` #5 0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541 #6 0x778a9c76 in QLockFilePrivate::tryLock_sys (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140 #7 0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) at io/qlockfile.cpp:208 #8 0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634 #9 0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:104 #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:415 #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267 #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270 #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous namespace)::Q_QGS_globalTestObject::innerFunction()::holder, __in_chrg=optimized out) at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56 ``` Which seems to be second issue we found. I'm not sure how to solve this issue yet, but I'd be happy to just document the issue and strike this test. I only get this when the configuration file does not exist before the program executes, which may be why we missed it from the earlier patches. David Faure wrote: Indeed. Good catch. Fixed in Qt. https://codereview.qt-project.org/106953. David Faure wrote: Ah, and https://codereview.qt-project.org/106954 Matthew Dawson wrote: Ok, sounds good :). I think it would be good to fix as much of the issue this RR covers while we wait for that patch to ship in a released Qt version (since I assume its Qt 5.5 or 5.6 territory). So, I think the ksharedconfig_in_global_object test and and the changes in the src/ folder should be committed now, so that KSharedConfig creation is safe. After the Qt patch lands, this RR (or a new one) can be updated to test everything, given a recent enough copy of Qt (basically kconfig_in_global object). Thoughts? If you are happy with this plan, the first part has a ship it from me. OK, done, thanks for the input. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review76407 --- On April 4, 2015, 11:45 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated April 4, 2015, 11:45 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Feb. 21, 2015, 8:19 p.m., Matthew Dawson wrote: Everything looked good, but when I tried to rerun the tests to make sure everything is ok the kconfig_in_global_object test I'm getting an abort with the same error message. Backtrace: ``` #5 0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541 #6 0x778a9c76 in QLockFilePrivate::tryLock_sys (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140 #7 0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) at io/qlockfile.cpp:208 #8 0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634 #9 0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:104 #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:415 #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267 #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270 #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous namespace)::Q_QGS_globalTestObject::innerFunction()::holder, __in_chrg=optimized out) at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56 ``` Which seems to be second issue we found. I'm not sure how to solve this issue yet, but I'd be happy to just document the issue and strike this test. I only get this when the configuration file does not exist before the program executes, which may be why we missed it from the earlier patches. Indeed. Good catch. Fixed in Qt. https://codereview.qt-project.org/106953. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review76407 --- On Feb. 21, 2015, 7:58 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Feb. 21, 2015, 7:58 p.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Feb. 21, 2015, 8:19 p.m., Matthew Dawson wrote: Everything looked good, but when I tried to rerun the tests to make sure everything is ok the kconfig_in_global_object test I'm getting an abort with the same error message. Backtrace: ``` #5 0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541 #6 0x778a9c76 in QLockFilePrivate::tryLock_sys (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140 #7 0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) at io/qlockfile.cpp:208 #8 0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634 #9 0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:104 #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:415 #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267 #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270 #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous namespace)::Q_QGS_globalTestObject::innerFunction()::holder, __in_chrg=optimized out) at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56 ``` Which seems to be second issue we found. I'm not sure how to solve this issue yet, but I'd be happy to just document the issue and strike this test. I only get this when the configuration file does not exist before the program executes, which may be why we missed it from the earlier patches. David Faure wrote: Indeed. Good catch. Fixed in Qt. https://codereview.qt-project.org/106953. Ah, and https://codereview.qt-project.org/106954 - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review76407 --- On Feb. 21, 2015, 7:58 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Feb. 21, 2015, 7:58 p.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Feb. 21, 2015, 3:19 p.m., Matthew Dawson wrote: Everything looked good, but when I tried to rerun the tests to make sure everything is ok the kconfig_in_global_object test I'm getting an abort with the same error message. Backtrace: ``` #5 0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541 #6 0x778a9c76 in QLockFilePrivate::tryLock_sys (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140 #7 0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) at io/qlockfile.cpp:208 #8 0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634 #9 0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:104 #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:415 #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267 #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270 #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous namespace)::Q_QGS_globalTestObject::innerFunction()::holder, __in_chrg=optimized out) at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56 ``` Which seems to be second issue we found. I'm not sure how to solve this issue yet, but I'd be happy to just document the issue and strike this test. I only get this when the configuration file does not exist before the program executes, which may be why we missed it from the earlier patches. David Faure wrote: Indeed. Good catch. Fixed in Qt. https://codereview.qt-project.org/106953. David Faure wrote: Ah, and https://codereview.qt-project.org/106954 Ok, sounds good :). I think it would be good to fix as much of the issue this RR covers while we wait for that patch to ship in a released Qt version (since I assume its Qt 5.5 or 5.6 territory). So, I think the ksharedconfig_in_global_object test and and the changes in the src/ folder should be committed now, so that KSharedConfig creation is safe. After the Qt patch lands, this RR (or a new one) can be updated to test everything, given a recent enough copy of Qt (basically kconfig_in_global object). Thoughts? If you are happy with this plan, the first part has a ship it from me. - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review76407 --- On Feb. 21, 2015, 2:58 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Feb. 21, 2015, 2:58 p.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review76407 --- Everything looked good, but when I tried to rerun the tests to make sure everything is ok the kconfig_in_global_object test I'm getting an abort with the same error message. Backtrace: ``` #5 0x7791b630 in qAppName () at kernel/qcoreapplication.cpp:541 #6 0x778a9c76 in QLockFilePrivate::tryLock_sys (this=this@entry=0x608ec0) at io/qlockfile_unix.cpp:140 #7 0x7783c1ec in QLockFile::tryLock (this=optimized out, timeout=-1) at io/qlockfile.cpp:208 #8 0x77ba17db in KConfigIniBackend::lock (this=0x608fe0) at /home/matthew/kde/kconfig/src/core/kconfigini.cpp:634 #9 0x77b84ce0 in KConfigPrivate::lockLocal (this=0x608b40) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:104 #10 0x77b86aa9 in KConfig::sync (this=0x606ae0) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:415 #11 0x77b85c28 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:267 #12 0x77b85c82 in KConfig::~KConfig (this=0x606ae0, __in_chrg=optimized out) at /home/matthew/kde/kconfig/src/core/kconfig.cpp:270 #13 0x00401844 in Tester::~Tester (this=0x6030e8 (anonymous namespace)::Q_QGS_globalTestObject::innerFunction()::holder, __in_chrg=optimized out) at /home/matthew/kde/kconfig/autotests/kconfig_in_global_object.cpp:56 ``` Which seems to be second issue we found. I'm not sure how to solve this issue yet, but I'd be happy to just document the issue and strike this test. I only get this when the configuration file does not exist before the program executes, which may be why we missed it from the earlier patches. - Matthew Dawson On Feb. 21, 2015, 2:58 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Feb. 21, 2015, 2:58 p.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 27, 2015, 8:55 p.m., Matthew Dawson wrote:
Unforunately, this cause test system failures in the the
kconfigskeletontest test suite. I'm not sure why this should create issues
there.
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.
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?
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.
David Faure wrote:
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.
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.
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.
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.
I'm currently thinking Revision 2 of this patch was the best, it fixed
the regression compared to kdelibs4 without introducing new issues.
Matthew Dawson wrote:
I'm thinking using KConfig when a QCoreApplication is not present should
just not be supported. At the very least, the new issue in the save path
means you can't actually save anything when the global destructors are
running, which makes using KConfig then pointless. If we can get QLockFile
to work correctly in this case, we could revisit this.
For now, I think the best plan is going back to revision 2 (as you
suggested) + a comment on KConfig/KSharedConfig warning about using them
without a valid QCoreApplication is not supported. Does that sound good to
you?
using without a valid QCoreApplication is not supported .. funny because this
commit is especially about making it work, at least in some cases.
Using anything from Qt before creating a QCoreApplication is a bad idea anyway
(e.g. locale8Bit not set up) - but I'm happy to repeat that in the
K{Shared}Config documentation.
On the other hand, using KConfig after QCoreApplication is deleted (e.g. in a
global object dtor) does seem to work, says the unittest.
Only KSharedConfig is an issue, depending on the order of construction. So I
added a comment in the KSharedConfig class docu.
- David
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122232/#review74858
---
On Jan. 27, 2015, 8:10 a.m., David Faure wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122232/
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Feb. 21, 2015, 7:58 p.m.) Review request for KDE Frameworks and Matthew Dawson. Changes --- back to rev2 + api doc for KSharedConfig Repository: kconfig Description (updated) --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is now in kdelibs4 too (where it works) and reproduces Albert's KgDifficulty testcase. Diffs (updated) - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 1da816faea5548d2f529755d6538a142a2193720 src/core/ksharedconfig.h d0791e461804d421010bc5f82873371a4d3bc992 Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 27, 2015, 3:55 p.m., Matthew Dawson wrote: Unforunately, this cause test system failures in the the kconfigskeletontest test suite. I'm not sure why this should create issues there. 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. 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? 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. David Faure wrote: 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. 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. 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. 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. I'm currently thinking Revision 2 of this patch was the best, it fixed the regression compared to kdelibs4 without introducing new issues. I'm thinking using KConfig when a QCoreApplication is not present should just not be supported. At the very least, the new issue in the save path means you can't actually save anything when the global destructors are running, which makes using KConfig then pointless. If we can get QLockFile to work correctly in this case, we could revisit this. For now, I think the best plan is going back to revision 2 (as you suggested) + a comment on KConfig/KSharedConfig warning about using them without a valid QCoreApplication is not supported. Does that sound good to you? - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74858 --- On Jan. 27, 2015, 3:10 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 27, 2015, 3:10 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a src/core/ksharedconfig.cpp
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 27, 2015, 8:55 p.m., Matthew Dawson wrote: Unforunately, this cause test system failures in the the kconfigskeletontest test suite. I'm not sure why this should create issues there. 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. 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? 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. 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. 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. 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. 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. I'm currently thinking Revision 2 of this patch was the best, it fixed the regression compared to kdelibs4 without introducing new issues. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74858 --- On Jan. 27, 2015, 8:10 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 27, 2015, 8:10 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a src/core/ksharedconfig.cpp e059b87a1cc1df50693a668ef791e7a61050ef88 autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 27, 2015, 8:10 a.m.) Review request for KDE Frameworks and Matthew Dawson. Changes --- create singleton after qApp automatically Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs (updated) - autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a src/core/ksharedconfig.cpp e059b87a1cc1df50693a668ef791e7a61050ef88 autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 27, 2015, 6:31 a.m., Matthew Dawson wrote: Sorry, I just discovered an issue with this change as is. If the global object is created before KConfig's Global object, a seg fault develops. In this case, KConfig's global is created first avoiding the pain. However, commenting out the noted line causes a seg fault in the tests. I don't think we can assume people will know to create appropriate KConfig objects, so I'd like to avoid relying on this in the test to pass. I'm not sure how we can force the creation of the global ahead of time. My best thought would be some sort of global pointer to the QStringList, combined with some atomic pointer operations to create it. Thoughts? Otherwise, everything looked go to me. David Faure wrote: The problem is not the order of construction in itself (construction is on demand), but the order of destruction (which is reverse to the order of construction, at least on linux). I know that it crashes without this line, but it does that in kdelibs4 too: Fatal Error: Accessed global static 'KGlobalPrivate *globalData()' after destruction. Defined at kde/kdelibs/kdecore/kernel/kglobal.cpp:128 Therefore I don't consider this a real problem. This crash or assert comes from relying on the order of destruction, which one is not supposed to do. In practice most apps will use KConfig soon after creating the app anyway, for their mainwindow, which is why we didn't have a big problem with this. If we really want to fix this, however, Q_CONSTRUCTOR_FUNCTION(someFunc) (in ksharedconfig.cpp) comes to mind. That would allow to automate exactly what the line in the test does, calling openConfig right after the qapp is created. I'm just not sure if this is a good idea ;) Hmm, I'm slowly warming up to the idea, seeing that it doesn't actually create a file if the app doesn't end up using KConfig at all (but just links to the lib indirectly). Let me try it out. I mean Q_COREAPP_STARTUP_FUNCTION. Q_CONSTRUCTOR_FUNCTION is too early, no qApp yet. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74802 --- On Jan. 26, 2015, 8:11 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 26, 2015, 8:11 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 27, 2015, 6:31 a.m., Matthew Dawson wrote: Sorry, I just discovered an issue with this change as is. If the global object is created before KConfig's Global object, a seg fault develops. In this case, KConfig's global is created first avoiding the pain. However, commenting out the noted line causes a seg fault in the tests. I don't think we can assume people will know to create appropriate KConfig objects, so I'd like to avoid relying on this in the test to pass. I'm not sure how we can force the creation of the global ahead of time. My best thought would be some sort of global pointer to the QStringList, combined with some atomic pointer operations to create it. Thoughts? Otherwise, everything looked go to me. The problem is not the order of construction in itself (construction is on demand), but the order of destruction (which is reverse to the order of construction, at least on linux). I know that it crashes without this line, but it does that in kdelibs4 too: Fatal Error: Accessed global static 'KGlobalPrivate *globalData()' after destruction. Defined at kde/kdelibs/kdecore/kernel/kglobal.cpp:128 Therefore I don't consider this a real problem. This crash or assert comes from relying on the order of destruction, which one is not supposed to do. In practice most apps will use KConfig soon after creating the app anyway, for their mainwindow, which is why we didn't have a big problem with this. If we really want to fix this, however, Q_CONSTRUCTOR_FUNCTION(someFunc) (in ksharedconfig.cpp) comes to mind. That would allow to automate exactly what the line in the test does, calling openConfig right after the qapp is created. I'm just not sure if this is a good idea ;) Hmm, I'm slowly warming up to the idea, seeing that it doesn't actually create a file if the app doesn't end up using KConfig at all (but just links to the lib indirectly). Let me try it out. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74802 --- On Jan. 26, 2015, 8:11 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 26, 2015, 8:11 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74858 --- Unforunately, this cause test system failures in the the kconfigskeletontest test suite. I'm not sure why this should create issues there. 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. 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? 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. - Matthew Dawson On Jan. 27, 2015, 3:10 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 27, 2015, 3:10 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a src/core/ksharedconfig.cpp e059b87a1cc1df50693a668ef791e7a61050ef88 autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74802 --- Sorry, I just discovered an issue with this change as is. If the global object is created before KConfig's Global object, a seg fault develops. In this case, KConfig's global is created first avoiding the pain. However, commenting out the noted line causes a seg fault in the tests. I don't think we can assume people will know to create appropriate KConfig objects, so I'd like to avoid relying on this in the test to pass. I'm not sure how we can force the creation of the global ahead of time. My best thought would be some sort of global pointer to the QStringList, combined with some atomic pointer operations to create it. Thoughts? Otherwise, everything looked go to me. autotests/ksharedconfig_in_global_object.cpp https://git.reviewboard.kde.org/r/122232/#comment51847 Line to comment out. For the final version this should work without this line. - Matthew Dawson On Jan. 26, 2015, 3:11 a.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 26, 2015, 3:11 a.m.) Review request for KDE Frameworks and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote: src/core/kconfig.cpp, line 542 https://git.reviewboard.kde.org/r/122232/diff/1/?file=35#file35line542 Reading the Qt documentation, I think its possible that appArgs[0] isn't the application name on Windows, and thus appArgs may stay empty. Maybe add another conditional checking its empty in this if statement, and if it still is add some random value? David Faure wrote: It's possible that appArgs[0] is not the app name, but I'm pretty sure the QStringList we get from QCoreApplication is never empty. Matthew Dawson wrote: I don't have a Windows machine to test this theory on. Could you forward this commit to the KDE Windows developers, so they can double check and fix as necessary? Since this solves the bug on *nix platforms, and should mostly solve it on Windows, I don't think blocking this version over this issue is worthwhile then. I read the qcoreapplication.cpp code, and it ensures that the number of arguments is respected, it's just that it grabs them in unicode instead of 8-bit. So arguments() is never empty except when the warning no QCoreApplication yet is printed, which this commit fixes. (and the empty list doesn't lead to e.g. a crash, just to querying again next time, so that's fine) Calling initConfig() before the QApp ctor still leads to that warning, though, obviously. But I think that's fine, one should create a QApp before anything else (and avoid global static constructors running before main, anyway). - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74671 --- On Jan. 23, 2015, 10:39 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 23, 2015, 10:39 p.m.) Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 26, 2015, 8:11 a.m.) Review request for KDE Frameworks and Matthew Dawson. Changes --- made the requested changes Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs (updated) - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote: autotests/kconfig_in_global_object.cpp, line 56 https://git.reviewboard.kde.org/r/122232/diff/1/?file=33#file33line56 This doesn't actually trigger anything when run without the fix below. Since a name is provided in initConfig, it never tries to fetch the global name and thus doesn't test the issue. I think these two files should be the exact same, except for testing KSharedConfig vs KConfig. Yes this unittest is not directly related to the change. It's just that I found it in kdelibs4support and brought it over. Any test is good to have, right? On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote: autotests/ksharedconfig_in_global_object.cpp, line 52 https://git.reviewboard.kde.org/r/122232/diff/1/?file=34#file34line52 Just before this, could you stick a: qputenv(QT_FATAL_WARNINGS, 1); That way if a warning is printed the test will fail. Otherwise this change might be removed with tests still passing. Yep, I completely agree. I actually thought of that some time after submitting the patch, and forgot to make the change. Done now, thanks :) On Jan. 24, 2015, 5:12 p.m., Matthew Dawson wrote: src/core/kconfig.cpp, line 542 https://git.reviewboard.kde.org/r/122232/diff/1/?file=35#file35line542 Reading the Qt documentation, I think its possible that appArgs[0] isn't the application name on Windows, and thus appArgs may stay empty. Maybe add another conditional checking its empty in this if statement, and if it still is add some random value? It's possible that appArgs[0] is not the app name, but I'm pretty sure the QStringList we get from QCoreApplication is never empty. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74671 --- On Jan. 23, 2015, 10:39 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 23, 2015, 10:39 p.m.) Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
On Jan. 24, 2015, 12:12 p.m., Matthew Dawson wrote: autotests/kconfig_in_global_object.cpp, line 56 https://git.reviewboard.kde.org/r/122232/diff/1/?file=33#file33line56 This doesn't actually trigger anything when run without the fix below. Since a name is provided in initConfig, it never tries to fetch the global name and thus doesn't test the issue. I think these two files should be the exact same, except for testing KSharedConfig vs KConfig. David Faure wrote: Yes this unittest is not directly related to the change. It's just that I found it in kdelibs4support and brought it over. Any test is good to have, right? I'd prefer to have a specific case for this, as it seems the test using ksharedconfig does the same thing anyways. The only difference is that the configuration is created before QCoreApplication quits. Since it's probably a good idea to ensure that works before worring about the argument copying, lets keep this test. Just two things: I don't think line 45 is necessary, since t is in fact used on line 49. Also, please add the environment variable, similar to below, to verify it doesn't complain. On Jan. 24, 2015, 12:12 p.m., Matthew Dawson wrote: src/core/kconfig.cpp, line 542 https://git.reviewboard.kde.org/r/122232/diff/1/?file=35#file35line542 Reading the Qt documentation, I think its possible that appArgs[0] isn't the application name on Windows, and thus appArgs may stay empty. Maybe add another conditional checking its empty in this if statement, and if it still is add some random value? David Faure wrote: It's possible that appArgs[0] is not the app name, but I'm pretty sure the QStringList we get from QCoreApplication is never empty. I don't have a Windows machine to test this theory on. Could you forward this commit to the KDE Windows developers, so they can double check and fix as necessary? Since this solves the bug on *nix platforms, and should mostly solve it on Windows, I don't think blocking this version over this issue is worthwhile then. - Matthew --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74671 --- On Jan. 23, 2015, 5:39 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 23, 2015, 5:39 p.m.) Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74671 --- The overall idea is sound. Just a few issues: autotests/kconfig_in_global_object.cpp https://git.reviewboard.kde.org/r/122232/#comment51750 This doesn't actually trigger anything when run without the fix below. Since a name is provided in initConfig, it never tries to fetch the global name and thus doesn't test the issue. I think these two files should be the exact same, except for testing KSharedConfig vs KConfig. autotests/ksharedconfig_in_global_object.cpp https://git.reviewboard.kde.org/r/122232/#comment51751 Just before this, could you stick a: qputenv(QT_FATAL_WARNINGS, 1); That way if a warning is printed the test will fail. Otherwise this change might be removed with tests still passing. src/core/kconfig.cpp https://git.reviewboard.kde.org/r/122232/#comment51752 Reading the Qt documentation, I think its possible that appArgs[0] isn't the application name on Windows, and thus appArgs may stay empty. Maybe add another conditional checking its empty in this if statement, and if it still is add some random value? - Matthew Dawson On Jan. 23, 2015, 5:39 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated Jan. 23, 2015, 5:39 p.m.) Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
Re: Review Request 122232: KConfig: fix using KSharedConfig in global object destructor.
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/#review74666 --- Makes sense to me, need someone else to approve though, not a kconfig expert myself - Albert Astals Cid On gen. 23, 2015, 10:39 p.m., David Faure wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/122232/ --- (Updated gen. 23, 2015, 10:39 p.m.) Review request for KDE Frameworks, Albert Astals Cid and Matthew Dawson. Repository: kconfig Description --- kconfig_in_global_object.cpp comes from kdelibs4support (after porting to Q_GLOBAL_STATIC) ksharedconfig_in_global_object.cpp is new (but works with kdelibs4) and reproduces Albert's KgDifficulty testcase. Diffs - autotests/CMakeLists.txt b91f754b705fc87bb8b729bea72fbb5f7d427ace autotests/kconfig_in_global_object.cpp PRE-CREATION autotests/ksharedconfig_in_global_object.cpp PRE-CREATION src/core/kconfig.cpp 782e9714521234a3e3d8f3a788967e5c9a40f38a Diff: https://git.reviewboard.kde.org/r/122232/diff/ Testing --- Both tests pass and the QCoreApplication::arguments warning (because called after qApp is destroyed) is gone. Thanks, David Faure ___ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
