-----------------------------------------------------------
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
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
