<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115684/">https://git.reviewboard.kde.org/r/115684/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 14th, 2014, 3:26 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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)</pre>
</blockquote>
<p>On February 14th, 2014, 4:11 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On February 15th, 2014, 12:17 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for looking into this.
I very much like the idea of getting a list of camelcase headers to install rather than using install(DIRECTORIES) which just picks up what was left over by a previous build in one's builddir.
The thing about OUTPUT_DIR is that it's not used anywhere, so I would be in favour of just removing it, for simplicity.
About target_include_directories: what we want is
target_include_directories(KF5Parts PUBLIC "$<BUILD_INTERFACE:${KParts_BINARY_DIR};${CMAKE_CURRENT_BINARY_DIR}>")
so that the version header is found (it's at the toplevel). Easy to script with my perl line (just remove /local).
To make the porting easier, maybe we should write this in a macro with a slightly different name, port incrementally, and then remove ecm_generate_headers? Otherwise it's harder to work together on this. My suggestion was: if you write the new macro and test it on two frameworks (kcoreaddons and kparts, to have prefixed and non-prefixed), I can write scripts to port all the modules to that without manual labour.
Alternatively, I can do it all next friday (vacation!).</pre>
</blockquote>
<p>On February 15th, 2014, 12:24 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I would argue that the purpose of OUTPUT_DIR is to deal with repos where the usual behaviour could cause a file conflict in the build directory. We don't expect most projects to use it, but I think it's still worth having to deal with that.
target_include_directories: yes, I was ignoring the version headers in my example and just concentrating on the code directly related to this function.
Even better, if we use the HEADER_NAMES keyword, we can select between implementations based on the presence of that. Then we can remove the old implementation once everything is ported.
I should be able to do this today.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">https://git.reviewboard.kde.org/r/115765/ for the new ECMGenerateHeaders
https://git.reviewboard.kde.org/r/115766/ for KCoreAddons
https://git.reviewboard.kde.org/r/115767/ for KParts</pre>
<br />
<p>- Alex</p>
<br />
<p>On February 11th, 2014, 10:15 p.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Build System, Extra Cmake Modules, KDE Frameworks, and Harald Fernengel.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Feb. 11, 2014, 10:15 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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 .`</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Applied it, ran the perl script, and a full build-from-scratch worked.
Not tested on a Mac, though :)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>modules/ECMGenerateHeaders.cmake <span style="color: grey">(e98a22e91151d23d7c798ff22a33097ec2a59d10)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115684/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>