Daniele E. Domenichelli wrote:
> 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)
Ok, probably no need for the additional STREQUAL check then.
>> * 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
Ok, thanks for the explanation. I'm sure that's fine then.
> 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.
Are there also GTK_${_var}_LIBRARIES variables?
Conventionally, the *LIBRARIES variables are for user-use and the *LIBRARY
variables are not...
> I think it can be useful during a transition from variables to targets...
>
> Does it sound wrong?
It's probably ok.
>
>> * 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...
Yes, I didn't remove it from there yet. I do intend to though probably after
CMake 2.8.12.
> 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
Something missing here, but if backporting is the motivation, I can see the
reasoning. No need to remove the
IMPORTED_LINK_INTERFACE_LIBRARIES_${_config} if you don't want to.
>
>> * 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,
Indeed. I looked again at git diff stage/master...stage/FindGTK2-targets and
didn't find that chunk. Odd. I thought that's what I did before too...
> I'm not
> sure if it is possible to squash commits/rebase topics published and
> merged to next.
It's possible, but a bit tricky. If you rebase, the result of the rebasing
needs to be the exact same as what is already in next. When fixing up a
branch, that means making fixup commits, then pushing them, then squashing
the fixes with a rebase, then force pushing. You can test the merge to
stage/next locally too.
>
>
>> * 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.
*shrug*. I've seen the same argument presented before, but I don't agree
with it. As you wish.
> Does this add the glib target twice?
I'm not sure. Even if it does, that shouldn't be harmful.
>
>
>> * 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.
It would be interesting to get more information on this.
> 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...
Ok.
>
> 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)
>
Perhaps. It also might make sense to try to get fontconfig upstream to add
the cmake config files. fontconfig is not a public dependency though, right?
Anyway, I don't think your work should be blocked on that. If freetype
really is a link dependency, I think it would be acceptable to just add
${FREETYPE_LIBRARIES} to the INTERFACE_LINK_LIBRARIES.
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
