Re: [cmake-developers] Targets for FindGTK2.cmake
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
Re: [cmake-developers] Targets for FindGTK2.cmake
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
Re: [cmake-developers] Please review CXXFeatures.cmake
Rolf Eike Beer wrote: 5) This is not nice API: #if defined (CXXFEATURES_NULLPTR_FOUND) void *nix = nullptr; #else /* CXXFEATURES_NULLPTR_FOUND */ void *nix = 0; #endif /* CXXFEATURES_NULLPTR_FOUND */ Much better would be: void *nix = CXXFEATURES_NULLPTR; where -DCXXFEATURES_NULLPTR=0 or -DCXXFEATURES_NULLPTR=nullptr. See what Qt does for other similar API decisions on what should be defined to something (like nullptr, constexpr, final etc), and what should be a 'guard' define like above (eg lambdas, variadic templates etc). Note also that by defining the CXXFEATURES_FINAL to something, you get to use the 'sealed' extension, which does the same thing, and works with VC2005. See qcompilerdetection.h. The module returns just a list of CMake flags. How this is passed to the user (header, defines, whatever) is currently something the user must decide. I will not do anything fancy in the testcase for now. Imagine I wanted to set Grantlee_FINAL to empty or final based on whether c++11 was active or not. How would I do that? I might do this: # This seems like an API smell. With g++ I want to wrap the add_definitions # in a condition for enabling c++11 at all. Does this mean that c++11 # features are not made available when using MSVC? if(CXX11_COMPILER_FLAGS) # Can't use add_compile_options as CXX11_COMPILER_FLAGS is a string, not # a list. # This means that -std=c++11 is also passed when linking. set(CMAKE_CXX_FLAGS ${CMAKE_CXX_FLAGS} ${CXX11_COMPILER_FLAGS}) if (CXXFeatures_class_override_final_FOUND) add_definitions(-DGrantlee_FINAL=final) set(_final_defined 1) endif() endif() if (NOT _final_defined) add_definitions(-DGrantlee_FINAL=) endif() Do you have any more-real-world examples of what code using your module would look like? My c++ code would then look like: struct A Grantlee_FINAL { int data; }; However, now downstreams need to define Grantlee_FINAL to something in order to compile. We can help of course by putting Grantlee_FINAL in the INTERFACE_COMPILE_DEFINITIONS of Grantlee. However, what do we define it to? We need to define it based on the capabilities of the downstream. Ok, the way to do things like that in CMake is generator expressions. target_compile_definitions(Grantlee INTERFACE Grantlee_FINAL=$$TARGET_PROPERTY:CXX11:final ) So, if the consumer has the CXX11 property ON, then it will be defined to 'final'. However, the problem is that there is no standard CXX11 target property. At best, I'd choose a property name which is a de-facto standard. See point 3: http://thread.gmane.org/gmane.comp.programming.tools.cmake.devel/6726/focus=7671 To repeat my point 5, the target_compile_definitions code might look more like this: set(msvc_genex $STREQUAL:$COMPILER_ID,MSVC) set(msvc_sealed_min $NOT:$VERSION_LESS:$CXX_COMPILER_VERSION,1400) set(msvc_sealed_max $VERSION_LESS:$CXX_COMPILER_VERSION,1700) set(msvc_sealed $AND:${msvc_genex},${msvc_sealed_min},${msvc_sealed_max}) set(_use_sealed $AND:$TARGET_PROPERTY:CXX11,${msvc_sealed}) set(_use_final $AND:$TARGET_PROPERTY:CXX11,$NOT:${msvc_sealed}) set(cxx11_final $${_use_final}:final$${_use_sealed}:sealed) target_compile_definitions(Grantlee INTERFACE Grantlee_FINAL=${cxx11_final} ) See https://qt.gitorious.org/qt/qtbase/commit/37a660c594a I really think that's something that should be solved by your module. I still think you should revisit my review mail on point 2 too. 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