[cmake-developers] Targets for FindGTK2.cmake

[Stephen Kelly]

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.