Fix messagelib core includes
AbandonedPublic

Authored by mlaurent on Mar 20 2018, 12:20 PM.

Details

Reviewers
joselema
Group Reviewers
KDE PIM
KDE PIM: KMail
Summary

Fix messagelist core includes.

There is a test in Kubuntu's packaging which builds the installed library headers alone, detecting problems such as having other headers #include'd which can't be found.

Right now, without this patch, the compilation of 'widgetbase.h' would fail because it won't be able to find '#include <core/enums.h>', so I replaced this with '#include <messagelist/enums.h>' and I also applied the same change to other headers which otherwise wouldn't be found.

It would be nice to get this problem sorted out in KDE's code, so this way we won't have to patch it in the packaging.

Test Plan

I have built the Kubuntu's messagelist package (17.12.3) with the patch and now the headers test pass. Also I made a complete rebuild of KDE Applications 17.12.3 from the packaging against this patched messagelib in order to make sure it doesn't break the compilation of other PIM components; everything seems to build fine.

Diff Detail

Repository
R94 PIM: Message Library
Lint
Lint Skipped
Unit
Unit Tests Skipped
joselema requested review of this revision.Mar 20 2018, 12:20 PM
joselema created this revision.
mlaurent requested changes to this revision.Mar 20 2018, 12:42 PM
mlaurent added a subscriber: mlaurent.

We don't have this problem in KDE CI...
I will not install not useful class as it's internal.

I don't know why you have this error but we need to find why but not install header which is not necessary.

Please paste the compile log error (not all just compile error failed)

This revision now requires changes to proceed.Mar 20 2018, 12:42 PM
joselema added a comment.EditedMar 20 2018, 5:30 PM

We don't have this problem in KDE CI...

... because the KDE CI is like Superman, with X-Ray vision. A god made of bits. And of course if a problem isn't catched by the KDE CI, that problem simply doesn't exist.

Now, seriously: we don't have this problem in the KDE CI because the KDE CI isn't checking what Kubuntu's packaging acc header test is checking. Which is, in case you are missing the point, building the installed headers alone. This way we can detect problems such as having installed headers relying on other headers which are not installed, or are not installed properly or just don't exist. That's not what the KDE CI does, right?

I will not install not useful class as it's internal.

I don't know why you have this error but we need to find why [...]

You need to find why, I already did and I already told you:
" the compilation of 'widgetbase.h' would fail because it won't be able to find '#include <core/enums.h>'"

Please paste the compile log error (not all just compile error failed)

As I already said above:

In file included from /usr/include/KF5/MessageList/WidgetBase:1:0,
                 from /tmp/ic7YlsVlR2/dump1.h:14:
/usr/include/KF5/messagelist/widgetbase.h:28:10: fatal error: core/enums.h: No such file or directory
 #include <core/enums.h>
          ^~~~~~~~~~~~~~
compilation terminated.

[...] but not install header which is not necessary.

Right now widgetbase.h is installed. That includes <core/enums.h>, that header is not installed in that path, therefore any external program including widgetbase.h won't be able to build. If that's the case, what's the point of installing a header which can't be used?

Maybe you shouldn't install widgetbase.h then?
If that's the case, the correct patch would be something like:

--- messagelib.orig/messagelist/src/CMakeLists.txt
+++ messagelib/messagelist/src/CMakeLists.txt
@@ -94,7 +94,6 @@ ecm_generate_headers(MessageList_Camelca
     View
     Enums
     StorageModelBase
-    WidgetBase
     REQUIRED_HEADERS MessageList_core_HEADERS
     RELATIVE core
     PREFIX MessageList

Thanks for the review.

"... because the KDE CI is like Superman, with X-Ray vision. A god made of bits. And of course if a problem isn't catched by the KDE CI, that problem simply doesn't exist."
We can try all wierd configuration for sure but with KDE CI we use standard configuration.

So after that ubuntu likes wierd configuration it's your choice :)

For me the second patch seems more logical.

Perhaps you can test it.
Indeed we don't need to install it.
so we don't need your first patch where you want to install not necessary headers.

I removed some bad include in 18.04 so perhaps it will help you in your wierd config

"... because the KDE CI is like Superman, with X-Ray vision. A god made of bits. And of course if a problem isn't catched by the KDE CI, that problem simply doesn't exist."
We can try all wierd configuration for sure but with KDE CI we use standard configuration.

So after that ubuntu likes wierd configuration it's your choice :)

Well, it's not a "weird configuration", it's just a headers check which reveals a problem with the KDE code which the KDE CI, right now, doesn't detect. Just for the record the headers check is meant to reveal problems in the packaging, however, as a side effect, sometimes it reveals problems in the KDE code like in this case.

To fix this problem for Kubuntu, we simply commented out temporarily the headers check. But please note that the universe is bigger than KDE and Ubuntu; this test is inherited from Debian, so consider the following hypothetical situation:

  1. The Debian developer packages a newer version of messagelib.
  2. The headers test fail.
  3. One possible solution is patching the code, if the Debian developer patches the code, the patch may be right or wrong (like the first version of the patch I proposed)
  4. Let's supose the patch is wrong, the Debian developer may or may not have time to send the patch here for review.
  5. I wouldn't be surprised if the wrong patch goes unnoticed to the Kubuntu and KDE Neon packaging via merges.

It's in our best interest to keep to a bare minimum the distribution patching, right? So what's the right solution here? Commenting out the headers check in Kubuntu like we did? Well, I think it's a lot better to be a step ahead and fix the problem in the KDE scope (like you just did this morning) rather than leaving loose ends which may result in an exccesive/wrong distribution patching.

For me the second patch seems more logical.

Perhaps you can test it.
Indeed we don't need to install it
so we don't need your first patch where you want to install not necessary headers.

That's great to know, I'm going to update the patch then.

I removed some bad include in 18.04 so perhaps it will help you in your wierd config

It's not a "weird config"...

Anyway, I have tested a modified 17.12.3 package with the following commits you did in master:
https://cgit.kde.org/messagelib.git/commit/?id=feda4e33e9e935384fc322878d8da58e1f08e562
https://cgit.kde.org/messagelib.git/commit/?id=28e8731a152b98f8f80c896c6b389748d5fdee43
https://cgit.kde.org/messagelib.git/commit/?id=e068f81e9a6176309301eed7765aa54b2ef120dc
https://cgit.kde.org/messagelib.git/commit/?id=1d9e28e3aa8bd1d7ad1b7b56720c436cd2dce8b9

That indeed fixes the problem, avoiding to install extra headers because you did something similar to the first patch, but removing <core/sortorder.h> from widgetbase.h which removes the need to install other extra headers in the pack, and thus, the main mistake from the first patch. Thank you for fixing it.

joselema updated this revision to Diff 30153.Mar 21 2018, 7:26 PM

Second version of the patch, just don't install widgetbase.h

mlaurent requested changes to this revision.Mar 22 2018, 7:50 AM

No as we use this include in MessageList/Widget

So we need to install it.

This revision now requires changes to proceed.Mar 22 2018, 7:50 AM

No as we use this include in MessageList/Widget

So we need to install it.

Well that seems to me exactly the opposite of what you said above; I sent the second version of the patch because you said above:

For me the second patch seems more logical.

Perhaps you can test it.
Indeed we don't need to install it.

So it's my understanding that you said that "we don't need to install widgetbase.h".

However, I tested the second patch against the stable branch, and I just realized it wouldn't build against master because of:
https://cgit.kde.org/messagelib.git/commit/?id=e068f81e9a6176309301eed7765aa54b2ef120dc

So using #include "core/widgetbase.h" instead of the camelcase <MessageList/Widget> would fix the compilation of the second patch against master.

So, I'm sending a third version of the patch in case your first quoted comment is right and the second one is wrong (i.e. the installation of widgetbase.h is not actually neccesary). If it's the opossite we can just close the review.

joselema updated this revision to Diff 30253.Mar 22 2018, 8:20 PM

Updating patch.

First it was an error to told you that we don't need to install WigetBase.
So we need to install it and we need to use camelcase headers.
So indeed I will close this one.

mlaurent commandeered this revision.Mar 23 2018, 5:42 AM
mlaurent abandoned this revision.
mlaurent edited reviewers, added: joselema; removed: mlaurent.