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

[Alex Merry]

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


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)

- Alex Merry


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-buildsystem/attachments/20140214/4cd21483/attachment.html>