Review Request 113406: Add a macro to automatically generate forward headers

[Stephen Kelly]

> On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote:
> > modules/ECMGenerateHeaders.cmake, line 11
> > <http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line11>
> >
> >     I really think the answers to my questions here need to be found first:
> >     
> >      http://thread.gmane.org/gmane.comp.kde.cvs/1272841
> >     
> >     It should be more-simple than it currently is.
> 
> Aleix Pol Gonzalez wrote:
>     I think that this patch answers most of these questions, leaving up to discussion if we expect people to include "module/something.h" or just "something.h".
>     
>     I'd say that if Qt5 transition teaches something is that it's not very flexible to namespace the include files, but that's up to the developer anyway...

I'm not sure it does...

For example, grantlee installs headers like this:

 include/grantlee/engine.h
 include/grantlee_export.h

include/grantlee/engine.h has include "grantlee_export.h" and even if it is <grantlee_export.h>, the intent is that the user only needs -Iinclude/, and does not need -Iinclude/grantlee.

The same is not true of where KF5 installs headers and how they are included. There is scope for improvement there, and this patch does not affect that.


> On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote:
> > modules/ECMGenerateHeaders.cmake, line 29
> > <http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line29>
> >
> >     This variable shouldn't be needed at all.
> 
> Aleix Pol Gonzalez wrote:
>     Variable? Or argument? Why?

Argument, yes, sorry.

I think that because the source dir is already in the INTERFACE_INCLUDE_DIRECTORIES of the target, there is no need to have to specify it, and there should be no need to use ${EGH_HEADERS_DIR}/ in the generated include.

I guess you have some reason for doing that, but that just points to non-uniformity which should maybe be fixed, as I wrote in the KIO commit I linked to.


> On Oct. 24, 2013, 1:54 p.m., Stephen Kelly wrote:
> > modules/ECMGenerateHeaders.cmake, line 32
> > <http://git.reviewboard.kde.org/r/113406/diff/2/?file=205462#file205462line32>
> >
> >     I recommend not putting this in the API of the function, and instead users should use 
> >     
> >      install(DIRECTORY) to install the generated files.
> 
> Aleix Pol Gonzalez wrote:
>     Why?

I think it's better API. It's what all existing functions which generate headers do.


- Stephen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/113406/#review42280
-----------------------------------------------------------


On Oct. 24, 2013, 1:40 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113406/
> -----------------------------------------------------------
> 
> (Updated Oct. 24, 2013, 1:40 p.m.)
> 
> 
> Review request for Build System, KDE Frameworks and Stephen Kelly.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> Create a macro that will generate the forward headers like we used to, in cmake configure time.
> 
> Here's an example of how we'd port KParts to that macro: http://paste.opensuse.org/9880051
> After the change, these are the installed headers: http://paste.opensuse.org/90980400
> 
> 
> Diffs
> -----
> 
>   modules/ECMGenerateHeaders.cmake PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/113406/diff/
> 
> 
> Testing
> -------
> 
> Ported KParts and still everything works.
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-buildsystem/attachments/20131024/846fdb8f/attachment.html>