Hello Stephen,
Thanks for the review
On 09/08/13 10:50, Stephen Kelly wrote:
> * 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?
GTK2 has "config" files that usually are not in the same directory as
the other include files. Therefore there are 2 different variables, and
both needs to be included (for example gdk/gdk.h includes
<gdk/gdktypes.h> that includes <gdkconfig.h>)
I will add a check to ensure that the same path is not added twice (even
though I didn't see any system where they are the same)
> * 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.
Ok
> * 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 is something I took from FindQT4.cmake...
The GTK2_${_var}_LIBRARY are not cached, it is generated from
GTK2_${_var}_LIBRARY_DEBUG and GTK2_${_var}_LIBRARY_RELEASE (that are
cached). The user can override these 2 variables, and they will end in
the target and in the GTK2_${_var}_LIBRARY variable
The difference is that if GTK2_USE_IMPORTED_TARGETS the
GTK_${_var}_LIBRARY will link the target (and it's dependencies)
otherwise it will link only the library (without dependencies) using the
DEBUG or RELEASE version, depending on what was found.
I think it can be useful during a transition from variables to targets...
Does it sound wrong?
> * 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.
Does this mean that setting the INTERFACE_LINK_LIBRARIES is enough?
Again I took this from FindQT4...
To be honest want to be able to backport the module, even though not the
target part (INTERFACE_INCLUDE_DIRECTORIES won't work anyway afaik), so
I can remove the
> * The diff contains this:
> + #_GTK2_ADD_TARGET_DEPENDS(GOBJECT glib)
> + _GTK2_ADD_TARGET_DEPENDS(GOBJECT glib)
It should be already removed in one of the following commits, I'm not
sure if it is possible to squash commits/rebase topics published and
merged to next.
> * 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.
atk explicitly requires glib (glib.h is included in some headers)
therefore I thought it was better to make this explicit here as well.
Does this add the glib target twice?
> * 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)?
To be honest I'm not completely sure here... On windows (with the GtkMM
installer) I'm having some issues with freetype, when linking
${GTK2_LIBRARIES}, but it links when I use the libraries one by one.
On the other hand, "pkg-config --libs gtk+-2.0" on my system reports
that the freetype library is required, even though the headers does not
seem to ... (I'm still investigating this).
Also for fontconfig it looks like that it doesn't need to be linked,
even though pkg-config reports so...
By the way, fontconfig is a freedesktop package, it is not part of GTK,
does it sound reasonable to create a module for that (possibly with
imported targets)
Regards,
Daniele
--
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
