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