Review Request 115684: Generate local forwarding headers under a local subdir, to fix clash on Mac OS X.

[Alex Merry]

> On Feb. 14, 2014, 3:26 p.m., Alex Merry wrote:
> > OK, I'll be honest, something about this whole module interface rubs me up the wrong way.  There's either too much or not enough magic: code that calls ecm_generate_headers needs to know things about the implementation and use them in things like target_include_directories.  And this change only adds to that.  I'm regretting not paying more attention when this first went in.
> > 
> > Personally, my preference would be to add another argument to complement REQUIRED_HEADERS (like GENERATED_HEADERS, or perhaps an unnamed initial argument) that provides a list of the generated files (with paths!) that can be passed to install.  Then the mixed directory doesn't matter (and, in fact, matches how the installation would work).  This would (IMO) reduce, rather than increase, the knowledge needed by calling code.  It would make it work much more like the familiar macros that add things to FOO_sources variables (especially if the unnamed initial argument is used).
> > 
> > In this scenario, I would also omit MODULE_NAME, and just dump the headers in the build dir unless OUTPUT_DIR is set.
> > 
> > I would like usage to look like
> > ecm_generate_headers(
> >     KArchive_FORWARDING_HEADERS
> >     HEADERS
> >         KArchive
> >         KArchiveEntry
> >         # etc
> >     REQUIRED_HEADERS KArchive_HEADERS
> > )
> > target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KArchive_BINARY_DIR}>") # optional
> > install(FILES ${KArchive_FORWARDING_HEADERS} ${KArchive_HEADERS}
> >         DESTINATION ${INCLUDE_INSTALL_DIR}/KArchive
> >         COMPONENT Devel)
> > 
> > 
> > ecm_generate_headers(
> >     KParts_FORWARDING_HEADERS
> >     HEADERS
> >         Part
> >         PartBase
> >         # etc
> >     PREFIX KParts # files go in OUTPUT_DIR/KParts and OUTPUT_DIR/kparts
> >     REQUIRED_HEADERS KArchive_HEADERS
> > )
> > target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KParts_BINARY_DIR}>")
> > install(FILES ${KParts_FORWARDING_HEADERS}
> >         DESTINATION ${INCLUDE_INSTALL_DIR}/KParts
> >         COMPONENT Devel)
> > install(FILES ${KParts_HEADERS}
> >         DESTINATION ${INCLUDE_INSTALL_DIR}/kparts
> >         COMPONENT Devel)
> > 
> > 
> > And with output dir:
> > ecm_generate_headers(
> >     KParts_FORWARDING_HEADERS
> >     HEADERS
> >         Part
> >         PartBase
> >         # etc
> >     OUTPUT_DIR "${KParts_BINARY_DIR}/kparts_headers"
> >     PREFIX KParts # files go in OUTPUT_DIR/KParts and OUTPUT_DIR/kparts
> >     REQUIRED_HEADERS KArchive_HEADERS
> > )
> > target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KParts_BINARY_DIR}/kparts_headers>")
> > install(FILES ${KParts_FORWARDING_HEADERS}
> >         DESTINATION ${INCLUDE_INSTALL_DIR}/KParts
> >         COMPONENT Devel)
> > install(FILES ${KParts_HEADERS}
> >         DESTINATION ${INCLUDE_INSTALL_DIR}/kparts
> >         COMPONENT Devel)

Two of those target_include_directories are wrong, of course, and should be
target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>")

Also, "HEADERS" should probably be "HEADER_NAMES", for clarity.


- Alex


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


On Feb. 11, 2014, 10:15 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115684/
> -----------------------------------------------------------
> 
> (Updated Feb. 11, 2014, 10:15 p.m.)
> 
> 
> Review request for Build System, Extra Cmake Modules, KDE Frameworks, and Harald Fernengel.
> 
> 
> Repository: extra-cmake-modules
> 
> 
> Description
> -------
> 
> Generate local forwarding headers under a local subdir, to fix clash on Mac OS X.
> 
> This is intended to replace RR 115541.
> 
> With case-insensitive filesystems, creating KParts and kparts subdirs
> in the same parent was obviously a bad idea, especially since we then
> make a copy of "KParts" and don't expect the contents of "kparts" to tag along.
> Solved by making that KParts (installed) and local/kparts (not installed).
> 
> Downside: the modules that use this PREFIX feature need a change like this:
> -target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KParts_BINARY_DIR}>")
> +target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KParts_BINARY_DIR};${CMAKE_CURRENT_BINARY_DIR}/local>")
> 
> Easily scripted though:
> perl -pi -e 's/>/\;\${CMAKE_CURRENT_BINARY_DIR}\/local>/ if (/target_include_directories/ && /PUBLIC/)' `grep -rwl PREFIX .`
> 
> 
> Diffs
> -----
> 
>   modules/ECMGenerateHeaders.cmake e98a22e91151d23d7c798ff22a33097ec2a59d10 
> 
> Diff: https://git.reviewboard.kde.org/r/115684/diff/
> 
> 
> Testing
> -------
> 
> Applied it, ran the perl script, and a full build-from-scratch worked.
> Not tested on a Mac, though :)
> 
> 
> Thanks,
> 
> David Faure
> 
>

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