Review Request 120959: [KPimUtils] Don't install headers into $include_dir/KPIMUtils/KPIMUtils

Martin Klapetek martin.klapetek at gmail.com
Mon Nov 3 23:13:09 UTC 2014 


> On Nov. 3, 2014, 8:59 p.m., Christophe Giboudeaux wrote:
> > KPIMUtils/KPIMUtils/SomeHeader is correct. This patch isn't (and the facebook resource builds fine without this patch).
> > 
> > KF5PimUtilsTargets.cmake sets this:
> > set_target_properties(KF5::PimUtils PROPERTIES
> >   INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/KF5/KPIMUtils;${_IMPORT_PREFIX}/include/KF5"
> > )
> > 
> > so, example:
> > #include <KPIMUtils/Email>  (ie (install prefix)/include/KF5/KPIMUtils/KPIMUtils/Email) won't have troubles finding kpimutils/email.h (ie (install prefix)/include/KF5/KPIMUtils/kpimutils/email.h)
> 
> Martin Klapetek wrote:
>     Well nothing in current Frameworks5 collection installs headers like this, so I'm not sure why kdepimlibs is/should be different. I'll add the frameworks group to weigh in.
>     
>     > and the facebook resource builds fine without this patch
>     
>     Fwiw, the facebook resource is disabled in master and it in no way builds fine, there are many other build errors, this is just one of them :)
> 
> Christophe Giboudeaux wrote:
>     ls /kde/inst/5/attica/include/KF5/Attica 
>     attica  Attica
>     
>     ls /kde/inst/5/baloo/include/KF5/Baloo 
>     baloo  Baloo
>     
>     ls /kde/inst/5/kdeclarative/include/KF5/KDeclarative 
>     kdeclarative  KDeclarative  quickaddons  QuickAddons
>     
>     ls /kde/inst/5/kdesu/include/KF5/KDESu 
>     kdesu  KDESu
>     
>     and so on...
>     
>     The first KPIMUtils is the module name, that's consistent with all the current frameworks
>     The second one is there to give a hint that the include should be KPIMUtils/PrettyHeader and not just PrettyHeader.
>     
>     That's why you can't see this for all frameworks.
> 
> Martin Klapetek wrote:
>     But then there is
>     
>     $ ls /opt/kde5/include/KF5/KArchive/
>     K7Zip    KAr       KArchiveDirectory    KArchiveEntry    karchive_export.h  karchivefile.h  kar.h               kcompressiondevice.h  kfilterbase.h  kfilterdev.h  ktar.h  KZipFileEntry    kzip.h
>     k7zip.h  KArchive  karchivedirectory.h  karchiveentry.h  KArchiveFile       karchive.h      KCompressionDevice  KFilterBase           KFilterDev     KTar          KZip    kzipfileentry.h
>     
>     $ ls /opt/kde5/include/KF5/KNotifications/
>     KNotification  knotification.h  KNotificationPlugin  knotificationplugin.h  KNotificationRestrictions  knotificationrestrictions.h  knotifications_export.h  KNotifyPlugin  knotifyplugin.h  KPassivePopup  kpassivepopup.h  KStatusNotifierItem  kstatusnotifieritem.h
>     
>     $ ls /opt/kde5/include/KF5/KCoreAddons/
>     (no subdirs and loots of files)
>     
>     and so on...
>     
>     Also, there is this: http://mail.kde.org/pipermail/kde-frameworks-devel/2013-December/009001.html (see Kevin's reply at the bottom).
>     
>     In fact, after looking a bit more into this, about half of frameworks installs headers into same directory while the rest uses subdirs. According to the above archive, it should use the same directory however.
> 
> Christophe Giboudeaux wrote:
>     probably fixed with http://commits.kde.org/kdepim-runtime/c43c404e915084b07582177928643c3f16ffc7cf
> 
> Martin Klapetek wrote:
>     Well, no. The issue as described above still stands, the consensus was different than the current way and I'd like us to get things less random.
> 
> Christophe Giboudeaux wrote:
>     There's nothing random. Let's take kcoreaddons and attica as examples:
>     
>     KCoreAddons/KAboutData contains #include "kaboutdata.h"
>     Attica/Attica/AccountBalance contains #include "attica/accountbalance.h"
>     
>     The namespaced camelcase headers just follow what is done for the files they include.
>     and if you need another reason:
>     
>         # ls **/Plugin
>         kabc/include/KF5/KABC/KABC/Plugin                              kontactinterface/include/KF5/KontactInterface/KontactInterface/Plugin  ktexteditor/include/KF5/KTextEditor/KTextEditor/Plugin kdelibs4support/include/KF5/KDELibs4Support/KDE/KParts/Plugin  kparts/include/KF5/KParts/KParts/Plugin
>     
>     If you use these build dependencies, what should happen if you just #include <Plugin> ?
> 
> Martin Klapetek wrote:
>     Please refer to http://mail.kde.org/pipermail/kde-frameworks-devel/2013-December/009001.html
>     
>     Quoting:
>     
>     The camel cased includes and the .h ones were planned to live in the same folder.
>     
>     Ending quote.
>     
>     Now, as you posted above:
>     
>     set_target_properties(KF5::PimUtils PROPERTIES
>       INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include/KF5/KPIMUtils;${_IMPORT_PREFIX}/include/KF5"
>     )
>     
>     ...will clearly make #include <KPIMUtils/Whatever> still work, without the need of another KPIMUtils subdirectory. Which is pretty much my whole point.
> 
> Christophe Giboudeaux wrote:
>     and would be inconsistent with the other frameworks expecting users to include <SomeModule/SomeHeader>
>     
>     Anyway, end of discussion, the include now works fine.

Can you please please read the whole discussion I linked to twice?

Nevertheless, I would still like to hear the opinion from Frameworks maintainer; and it's just rude if you close my review request without the maintainers authority when it has concerns based on valid arguments. Please don't do that.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120959/#review69749
-----------------------------------------------------------


On Nov. 4, 2014, 12:06 a.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120959/
> -----------------------------------------------------------
> 
> (Updated Nov. 4, 2014, 12:06 a.m.)
> 
> 
> Review request for KDE Frameworks, KDEPIM-Libraries and Laurent Montel.
> 
> 
> Repository: kdepimlibs
> 
> 
> Description
> -------
> 
> The generated headers have "#include "kpimutils/linklocator.h" but since they are being installed in KPIMUtils/KPIMUtils/, the inclusion fails because those real headers are installed in KPIMUtils/kpimutils (rather than KPIMUtils/KPIMUtils/kpimutils). I also don't see a reason why to install into KPIMUtils/KPIMUtils/...?
> 
> 
> Diffs
> -----
> 
>   kpimutils/src/CMakeLists.txt 1acc88e 
> 
> Diff: https://git.reviewboard.kde.org/r/120959/diff/
> 
> 
> Testing
> -------
> 
> Akonadi-facebook fails to build with "kpimutils/linklocator.h - No such file or directory", with this it no longer gives the error.
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20141103/fec0e4fd/attachment-0001.html>

More information about the Kde-frameworks-devel mailing list