KDE Frameworks 5.29.0

Sat Dec 10 18:49:07 UTC 2016

On Saturday, December 10, 2016 3:47:56 PM CET David Faure wrote:
> On samedi 10 décembre 2016 09:52:06 CET Martin Graesslin wrote:
> > On Thursday, December 8, 2016 9:32:13 AM CET David Faure wrote:
> > > On mercredi 7 décembre 2016 21:06:11 CET Kevin Funk wrote:
> > > > On Wednesday, 7 December 2016 20:10:40 CET Albert Astals Cid wrote:
> > > > > El dimecres, 7 de desembre de 2016, a les 10:08:18 CET, David Faure
> > > > > va
> > > > > 
> > > > > escriure:
> > > > > > On lundi 5 décembre 2016 18:40:46 CET Martin Gräßlin wrote:
> > > > > > > Am 2016-12-05 09:20, schrieb David Faure:
> > > > > > > > On dimanche 4 décembre 2016 23:42:44 CET šumski wrote:
> > > > > > > >> On nedjelja, 4. prosinca 2016. 00:37:52 CET David Faure 
wrote:
> > > > > > > >> > Dear packagers,
> > > > > > > >> > 
> > > > > > > >> > KDE Frameworks 5.29.0 has been uploaded to the usual place.
> > > > > > > >> > 
> > > > > > > >> > New framework: prison
> > > > > > > >> > 
> > > > > > > >> > Public release next Saturday.
> > > > > > > >> > 
> > > > > > > >> > Thanks for the packaging work!
> > > > > > > >> 
> > > > > > > >> kconfig (r129382) breaks compilation of kdevplatform:
> > > > > > > >> http://paste.opensuse.org/82016854
> > > > > > > > 
> > > > > > > > Indeed (but it's not the change from RR 129382, it's commit
> > > > > > > > cd4e650
> > > > > > > > from
> > > > > > > > https://phabricator.kde.org/D3386
> > > > > > > > 
> > > > > > > > Seems to come from Inherits=BaseClass while BaseClass doesn't
> > > > > > > > use
> > > > > > > > arg="true".
> > > > > > > > 
> > > > > > > > Here's a testcase for the kconfig unittests. Martin, can you
> > > > > > > > take
> > > > > > > > a
> > > > > > > > look?
> > > > > > > 
> > > > > > > The earliest I can have a look is probably on Friday, I'm sorry.
> > > > > > > 
> > > > > > > My suggestion is to revert my two commits and I'll redo for next
> > > > > > > frameworks.
> > > > > > 
> > > > > > OK, done. New git tag and tarball:
> > > > > > 
> > > > > > kconfig v5.29.0-rc2
> > > > > > 47f7e954a58ba5538d055e2f75e483cade48ee8a
> > > > > > d6c12e0908de1b91529de15e75a52c9974685c91b423d5b5abeb06f261d0fa47
> > > > > > sources/kconfig-5.29.0.tar.xz
> > > > > 
> > > > > Acoording to kfunk the thing that broke kdevplatform wasn't really
> > > > > kconfigs
> > > > > fault but a side effect of kdevplatform code not being very good.
> > > > 
> > > > Heya,
> > > > 
> > > > the patch restoring the kdevplatform build with KF5 5.29:
> > > >   https://cgit.kde.org/kdevplatform.git/commit/?
> > > > 
> > > > id=e84645d1694bdad7f179cd41babce723fe07aa63
> > > > 
> > > > The code in kdevplatform is a bit special, it's probably the only
> > > > place
> > > > in
> > > > whole KDE which broke due to the recent changes in kconfig. I don't
> > > > see
> > > > an
> > > > easy migration path, even if you introduce said change in a later
> > > > kconfig
> > > > release.
> > > > 
> > > > I don't mind if you leave kconfig as-is. But that's probably something
> > > > for
> > > > dfaure to decide.
> > > 
> > > Well, the change to kdevplatform isn't released yet, so kconfig 5.29-rc1
> > > would break compilation of the current kdevplatform releases.
> > > 
> > > Also, the fact that I'm able to write a kconfig unittest that doesn't
> > > compile tells me that something isn't right with these kconfig changes
> > > ---
> > > unless it can be proven that what I'm doing in that new test is not
> > > meaningful and is (now) forbidden, in which case it should at least be
> > > documented. This is certainly worth another month of careful thinking
> > > rather than rushing this into 5.29 now that it proved to be not 100%
> > > perfect.
> > 
> > I investigated and can prove now that the test is not meaningful: it
> > doesn't compile on master either. See https://paste.kde.org/po6oahg5p
> 
> Well, that only means my testcase is not good (sorry about that).
> KDevelop *did* compile, so clearly my testcase failed to emulate exactly
> what happened in kdevelop.
> 
> > The problem is the "Inherits" - it doesn't really specify the conditions.
> > All we have in the documentation is "Class the generated class inherits
> > from. This class must inherit  KConfigSkeleton."
> 
> I see.
> 
> > But inheriting from KConfigSkeleton is not enough as the test case and the
> > kdevelop example shows. It must have the same ctors as KConfigSkeleton
> > available for the inheriting class.
> 
> This makes sense. But adding a new constructor to KConfigSkeleton cannot
> mean that suddenly the requirement on base classes used by Inherits changes
> to include a new constructor - that would make it a source incompatible
> change.
> 
> So the requirement has to be more precise than "do whatever KConfigSkeleton
> does", which is not a fixed requirement (it can change over time).
> 
> Instead the requirement has to be "you need a constructor that takes a
> QString" or whatever the requirement actually is.

I just created a phab request to update the documentation about that: https://
phabricator.kde.org/D3636

> 
> > That's the problem with the autotest and
> > the problem with kdevelop's case. There the ctor existed, but was private
> > instead of public.
> 
> Yes, because it was not needed before. That's what I mean by this change
> being source incompatible.
> 
> > Given that I think my change can go in, but we also should specify more
> > clearly the Inherits requirements.
> 
> It's still a SIC. Which is only acceptable if what KDevelop was doing made
> no sense at all (e.g. broken behaviour at runtime). But from your
> description, it sounds to me like it did make sense, it's just that the
> change suddenly raises the requirements for base classes...

The code generated didn't match the description of what it would generate.

To quote: "The \<kcfgfile\> tag may contain either the "name" attribute, which 
should be the name of the configuration file described, or the "arg" attribute, 
which, if set to "true", will allow you to pass the KSharedConfig::Ptr object 
to use."

We have here the condition of arg="true" and the generated code did not follow 
that. If Singleton=true was set in kcfgc an incorrect ctor taking a QString as 
argument is generated. That is what my change addresses.

So yes, anybody who used that was depending on a bug of kconfig compiler.

One can argue that my change breaks SIC and I won't deny it. But the usage of 
the behavior was wrong.

I can do a special casing for the (incorrect) kdevelop case to provide SIC, 
but it will mean that we get another incorrect code compilation. The special 
casing would have to be:
* arg=true
* Singleton=true
* Inherits=true
-> preserve old behavior. I doubt that we win anything by that except of 
making the code more complex and the behavior seemingly more random.

So from my point of view breaking the incorrect behavior could be acceptable 
here.

For reference I attached the relevant kcfg, kcfgc and generated files.

I leave the decision to you. I don't mind doing the special casing, so if told 
I'll add it.

Cheers
Martin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: projectfiltersettings.kcfg
Type: application/xml
Size: 386 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/release-team/attachments/20161210/9d1b4f91/attachment.wsdl>
-------------- next part --------------
File=projectfiltersettings.kcfg
ClassName=ProjectFilterSettings
Singleton=true
UseEnumTypes=true
SetUserTexts=true
ItemAccessors=true
Inherits=KDevelop::ProjectConfigSkeleton
IncludeFiles=project/projectconfigskeleton.h
-------------- next part --------------
A non-text attachment was scrubbed...
Name: projectfiltersettings.cpp
Type: text/x-c++src
Size: 1327 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/release-team/attachments/20161210/9d1b4f91/attachment.cpp>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: projectfiltersettings.h
Type: text/x-chdr
Size: 657 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/release-team/attachments/20161210/9d1b4f91/attachment.h>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/release-team/attachments/20161210/9d1b4f91/attachment.sig>