Brad King wrote:
> On 08/06/2013 05:39 AM, Daniele E. Domenichelli wrote:
>> I added a commit introducing a couple of unit tests that should run only
>> if GTK and/or GTKMM are available on the system.
>
> Thanks. Steve, please take a quick look at this topic to
> review how the imported targets are constructed.
>
Here's my remarks:
* Using double-semicolons. Good.
* Release and debug libraries handled. Good.
* Release listed before Debug. Good. RelWithDebInfo etc should work.
* What is GTK2_GDKCONFIG_INCLUDE_DIR compared to GTK2_GDK_INCLUDE_DIR? Can
you conditionally append GTK2_GDKCONFIG_INCLUDE_DIR if it is not STREQUAL
GTK2_GDK_INCLUDE_DIR?
* Populating the INTERFACE_COMPILE_DEFINITIONS can be wrapped in
if(GTK2_DEFINITIONS) as it is only set when using msvc with two of the
targets. No need to set it to an empty string in all other cases.
* set(GTK2_${_var}_LIBRARY GTK2::${_basename} PARENT_SCOPE) seems to
override the user variable for the library and makes it impossible for the
user to set the library location by overriding the cache, right?
This line should not be needed anyway. I guess you want
if (GTK2_USE_IMPORTED_TARGETS)
list(APPEND GTK2_LIBRARIES GTK2::${basename})
else()
list(APPEND GTK2_LIBRARIES ${GTK2_${_var}_LIBRARY})
endif()
instead.
However, even then I don't recommend it. I'd say document that users should
use the imported target names directly if they want. Alex disagrees of
course, and would want you to populate a LIBRARY_TARGETS variable (I don't
see the point). I guess this change is not 2.8.12 material anyway though, so
maybe we'll finally sort all that out for in the next release anyway.
* CMake 2.8.12 will ignore IMPORTED_LINK_INTERFACE_LIBRARIES_${_config}, so
you don't need to set it. The only reason to want to set it is if you want
people to backport this updated module. I don't recommend setting it.
* The diff contains this:
+ #_GTK2_ADD_TARGET_DEPENDS(GOBJECT glib)
+ _GTK2_ADD_TARGET_DEPENDS(GOBJECT glib)
* If GObject depends on glib, then the line
_GTK2_ADD_TARGET_DEPENDS(ATK gobject glib) does not need to specify glib. I
would minimise those dependency listings.
* fontconfig seems to be only a compile dependency but not a link
dependency.
* freetype seems to be a link dependency (I assume, as it is added to
GTK2_LIBRARIES), but the library does not seem to be in the link interface
of the Cairo library. ${FREETYPE_LIBRARIES} can just be added to the
INTERFACE_LINK_LIBRARIES, but I think it might make sense to create an
imported target for it too anyway (in FindFreetype.cmake)?
* Thanks for the initiative. I have patches for zlib and png adding imported
targets, but I'm waiting for
http://public.kitware.com/Bug/view.php?id=14082. I'd like to add imported
targets to most Find modules shipped with cmake eventually.
Thanks,
Steve.
--
Powered by www.kitware.com
Visit other Kitware open-source projects at
http://www.kitware.com/opensource/opensource.html
Please keep messages on-topic and check the CMake FAQ at:
http://www.cmake.org/Wiki/CMake_FAQ
Follow this link to subscribe/unsubscribe:
http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
