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

Sat Feb 15 17:43:36 UTC 2014


> 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)
> 
> Alex Merry wrote:
>     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.
> 
> David Faure wrote:
>     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!).
> 
> Alex Merry wrote:
>     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.

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


- 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-buildsystem/attachments/20140215/edeff841/attachment.html>