Bug 84263 - compile fails due to bad Qt include directives
Summary: compile fails due to bad Qt include directives
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
4.4.1.2 release
Hardware: Other Linux (All)
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:5.2.0
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-24 06:30 UTC by Thomas.Eschenbacher
Modified: 2016-09-12 08:18 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments
patch to fix Qt includes (6.85 KB, text/plain)
2014-09-24 06:30 UTC, Thomas.Eschenbacher
Details
fix bad Qt include directives (5.91 KB, patch)
2015-04-03 06:22 UTC, Thomas.Eschenbacher
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas.Eschenbacher 2014-09-24 06:30:23 UTC
Created attachment 106763 [details]
patch to fix Qt includes

I wonder how anyone could compile libreoffice with KDE4/Qt4 support on an up-to-date installation. I had several compile errors because some Qt header files (like QString) were not found, which was quite obvious as the proper include paths were not set during compilation and the directory prefixes were not used (example: use <QtCore/QString> instead of <QString>).

Additionally there was an include with wrong quotation (#include "QString" instead of #include <QString>) and also some old (deprecated) include file names.

Fixing was quite simple, all affected files are under libreoffice-4.3.1.2/shell/source/backends/kde4be/*, please find the patch in the attachment...

my system:

- Gentoo Linux
- dev-qt/qtcore-4.8.5-r2
- kde-base/kdelibs-4.14.1
- sys-devel/gcc-4.8.3
Comment 1 Julien Nabet 2014-10-21 21:51:03 UTC
Lubos: I thought you might be interested in this one. Moreover, reporter suggests a patch.
Comment 2 Luboš Luňák 2014-10-22 05:29:29 UTC
LO gets Qt include paths from Qt's pkgconfig files. If your build does not use e.g .the QtCore include path, your Qt is presumably not installed properly.
Comment 3 Thomas.Eschenbacher 2015-04-03 06:22:51 UTC
Created attachment 114571 [details]
fix bad Qt include directives
Comment 4 Thomas.Eschenbacher 2015-04-03 06:24:46 UTC
Sorry, but the problem described is still present in recent versions (at least in 4.4.1.2) and makes compilation fail. Maybe I did not explain clear enough...

The problem is not in the include paths that are passed as compiler options, these are fine. The point is that the code uses deprecated and sometimes also wrong include directives - the errors are in the source code, not in the build environment! 

Somtimes the quotation is wrong, sometimes the component is missing (which is recommended by Qt since some years), somethimes the old fashioned (and also deprecated) file names are used.

Here some examples:

old/wrong:              => should be:
#include "QString"      => #include <QtCore/QString>
#include <qclipboard.h> => #include <QtGui/QClipboard>
#include <QHash>        => #include <QtCore/QHash>

I attached a patch to fix these things...
Comment 5 Buovjaga 2015-10-09 17:54:31 UTC
Jan-Marek: can you review the patch?
Comment 6 Jan-Marek Glogowski 2015-10-12 07:16:50 UTC
1. Using "" instead of <> for includes

Well - actually this is implementation specific. For gcc see: https://gcc.gnu.org/onlinedocs/cpp/Include-Syntax.html
If you send a patch to Gerrit, I'll merge it. The C standard, section 6.10.2, paragraphs 2 to 4 states, that "" falls back to <>, if the search doesn't succeed.

2. Include Qt components in #include <>

I was told there is no deprecation in #qt on IRC. Both include styles are correct. You have an independent build problem.
Comment 7 Thomas.Eschenbacher 2015-10-13 09:21:10 UTC
to 1.
yes, the theory says that it does not matter, but my experience says that this is also depends on the environment/distribution you are using, and their compiler flags. It is common habit to use "" for project local includes and <> for system includes. This can be enforced by CFLAGS. At least one handle do that in one unique way within one project, and not a wild mixture. That's just a matter of programming style.

to 2.
maybe you asked the wrong person...? I am sure that the Qt-4 include paths heavily depend on the distribution and in my case (Gentoo Linux) there is only one common qt4 include directive, so that the directory prefix is needed. This was different in Qt-3 and it will change (again) in Qt-5, but for Qt-4 you only are on the safe side if you use it the way I patched it. AFAIR this was the *recommended* way when porting from Qt-3 to Qt-4. Using the old way "might" work, but it definitely did not here.

So as a conclusion: what kind of risk do you see in fixing this?

BTW: my pkgconf and Qt setup *is* correct, definitely! I have hundreds of Qt/KDE applications that compile fine.
Comment 8 Thomas.Eschenbacher 2015-10-24 04:33:01 UTC
just for your reference, please read here:

https://techbase.kde.org/Policies/Library_Code_Policy#Getting_.23includes_right
Comment 9 Buovjaga 2015-10-26 13:42:40 UTC
Thomas: have you confirmed with Qt devs that your build setup is *not* broken?
The risk with merging your patch is that builds will break for others.
Comment 10 Thomas.Eschenbacher 2015-10-28 06:00:38 UTC
As I said above: I am working with a Gentoo Linux machine here, so I have to compile all kinds of Qt and KDE applications on my own and that works fine (except for unpatched LibreOffice). I am also maintaining a KDE application for more than 15 years, so I know about this topic from porting from KDE-3 to KDE-4.

If you want some "external reference", please read here for the topic "library prefix":

http://wiki.qt.io/Coding_Conventions

---------- citation begin ----------
Including headers

    In public header files, always use this form to include Qt headers: #include <QtCore/qwhatever.h>. The library prefix is neccessary for Mac OS X frameworks and is very convenient for non-qmake projects. 
---------- citation end ----------

The lower case file names like "qwhatever.h" seem to be ok, but most people use the class name like "QWhatEver". You can go to gitweb.kde.org, choose a KDE4 based application and look how they handle this, so just as a reference, you may want to take a look here for an example:

https://quickgit.kde.org/?p=kdelibs.git&a=blob&f=kdecore%2Fkernel%2Fkglobal.cpp
Comment 11 Commit Notification 2016-01-02 03:22:23 UTC
Jan-Marek Glogowski committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=d9bceb34310f05acce886fe28e9e42433fd488c2

tdf#84263 KDE4: unify includes

It will be available in 5.2.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 12 Xisco Faulí 2016-09-11 21:18:34 UTC Comment hidden (obsolete)
Comment 13 Thomas.Eschenbacher 2016-09-12 08:16:10 UTC
As stated in comment #11 a patch for this has already been submitted,
and it looks good now.

IMHO you can mark this bug as RESOLVED.
Comment 14 Julien Nabet 2016-09-12 08:18:09 UTC
Let's put this one to FIXED then.