(0035145)
Clinton Stimpson (developer)
2014-02-18 23:12

Thanks for the patch.
Can you modify it to keep the code for the static library case?

(0035146)
Orion Poplawski (reporter)
2014-02-18 23:43

I'll try - what's the proper test for dynamic/static - ENABLE_SHARED?

(0035147)
Stephen Kelly (developer)
2014-02-19 03:49

This is surprising. For example, you need to use QtCore if you use QtGui, but your patch removes that dependency. Does it exist somewhere else?

(0035149)
Clinton Stimpson (developer)
2014-02-19 09:19

Stephen, thanks for jumping in. The UseQt4.cmake file doesn't add include directories for the dependencies, but it does add the dependencies to the link. So, if using QtGui required the include directories for QtCore, the dependency would exist the user's CMakeLists.txt file.

Do you think it makes sense to add the link dependency without adding the include directories for shared Qt libraries?

Orion, FindQt4.cmake sets a QT_IS_STATIC variable for the static Qt case.

(0035150)
Stephen Kelly (developer)
2014-02-19 09:32

Clinton, I think I need to understand the code before I understand your question. The foreach loop at the bottom of UseQt4.cmake seems to add includes and defines for dependencies. How do I map that to what you wrote?

> So, if using QtGui required the include directories for QtCore, the
> dependency would exist the user's CMakeLists.txt file.

How would that exist in the users CMakeLists.txt file? Please post a snippet.

(0035151)
Clinton Stimpson (developer)
2014-02-19 11:16

The for loop adds include directories only if QT_USE_${module} is set.
It adds the link library if QT_USE_${module} is set or if it is a dependency for another module with that variable set.

For example:
cmake_minimum_required(VERSION 2.8)
find_package(Qt4 REQUIRED QtGui)
include(${QT_USE_FILE})
add_executable(testqt main.cpp)
target_link_libraries(testqt ${QT_LIBRARIES})

That will end up with an include directory with QtGui, but not QtCore, but will link with both QtGui and QtCore.

If this is used instead
find_package(Qt4 REQUIRED QtGui QtCore)
then the include directory for QtCore is added.

(0035152)
Stephen Kelly (developer)
2014-02-19 11:50

That looks like a bug to my eyes. However, the better bug fix is to use the imported targets anyway.

I have no further comment :).

(0035154)
Orion Poplawski (reporter)
2014-02-19 15:29

How about this version - it's a lot simpler :).

--- cmake-2.8.12.2/Modules/UseQt4.cmake.qtdeps 2014-01-16 10:15:08.000000000 -0700
+++ cmake-2.8.12.2/Modules/UseQt4.cmake 2014-02-19 13:27:21.780219091 -0700
@@ -99,7 +99,9 @@ foreach(module QT3SUPPORT QTOPENGL QTASS
           include_directories(SYSTEM ${QT_${module}_INCLUDE_DIR})
         endif(QT_INCLUDE_DIRS_NO_SYSTEM)
       endif()
- set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIBRARY})
+ if(QT_USE_${module} OR QT_IS_STATIC)
+ set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIBRARY})
+ endif()
       set(QT_LIBRARIES_PLUGINS ${QT_LIBRARIES_PLUGINS} ${QT_${module}_PLUGINS})
       if(QT_IS_STATIC)
         set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIB_DEPENDENCIES})

(0035155)
Clinton Stimpson (developer)
2014-02-19 15:48

Thanks, but that doesn't look quite correct.
I think we can just move the part that does set(*_DEPENDS 1) to inside the QT_IS_STATIC case. Does this work for you?

@@ -102,10 +102,10 @@ foreach(module QT3SUPPORT QTOPENGL QTASSISTANT QTDESIGNER QTMOTIF QTNSPLUGIN
       set(QT_LIBRARIES_PLUGINS ${QT_LIBRARIES_PLUGINS} ${QT_${module}_PLUGINS})
       if(QT_IS_STATIC)
         set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIB_DEPENDENCIES})
+ foreach(depend_module ${QT_${module}_MODULE_DEPENDS})
+ set(QT_USE_${depend_module}_DEPENDS 1)
+ endforeach()
       endif()
- foreach(depend_module ${QT_${module}_MODULE_DEPENDS})
- set(QT_USE_${depend_module}_DEPENDS 1)
- endforeach()
     else ()
       message("Qt ${module} library not found.")
     endif ()

(0035156)
Orion Poplawski (reporter)
2014-02-19 16:00

Well, I think Stephan was correct in that we always want to add in any needed includes and defines. And I'm really just interested in preventing over linkage. So my version (in the note, not attachment - that was a mistaken reposting of the original) only links if we specifically ask for the module or we are building statically.

(0035210)
Clinton Stimpson (developer)
2014-02-26 13:05

Fixed in b94ecab.

(0036918)
Robert Maynard (manager)
2014-10-06 10:32

Closing resolved issues that have not been updated in more than 4 months.

Issue History
Date Modified	Username	Field	Change
2014-02-11 10:53	Orion Poplawski	New Issue
2014-02-11 10:53	Orion Poplawski	File Added: cmake-qtdeps.patch
2014-02-11 11:01	Brad King	Assigned To	=> Clinton Stimpson
2014-02-11 11:01	Brad King	Status	new => assigned
2014-02-18 23:12	Clinton Stimpson	Note Added: 0035145
2014-02-18 23:43	Orion Poplawski	Note Added: 0035146
2014-02-19 03:49	Stephen Kelly	Note Added: 0035147
2014-02-19 09:19	Clinton Stimpson	Note Added: 0035149
2014-02-19 09:32	Stephen Kelly	Note Added: 0035150
2014-02-19 11:16	Clinton Stimpson	Note Added: 0035151
2014-02-19 11:50	Stephen Kelly	Note Added: 0035152
2014-02-19 15:29	Orion Poplawski	Note Added: 0035154
2014-02-19 15:30	Orion Poplawski	File Added: cmake-qtdeps-static.patch
2014-02-19 15:48	Clinton Stimpson	Note Added: 0035155
2014-02-19 16:00	Orion Poplawski	Note Added: 0035156
2014-02-26 13:05	Clinton Stimpson	Note Added: 0035210
2014-02-26 13:05	Clinton Stimpson	Status	assigned => resolved
2014-02-26 13:05	Clinton Stimpson	Resolution	open => fixed
2014-10-06 10:32	Robert Maynard	Note Added: 0036918
2014-10-06 10:32	Robert Maynard	Status	resolved => closed

Notes
(0035145) Clinton Stimpson (developer) 2014-02-18 23:12	Thanks for the patch. Can you modify it to keep the code for the static library case?

(0035146) Orion Poplawski (reporter) 2014-02-18 23:43	I'll try - what's the proper test for dynamic/static - ENABLE_SHARED?

(0035147) Stephen Kelly (developer) 2014-02-19 03:49	This is surprising. For example, you need to use QtCore if you use QtGui, but your patch removes that dependency. Does it exist somewhere else?

(0035149) Clinton Stimpson (developer) 2014-02-19 09:19	Stephen, thanks for jumping in. The UseQt4.cmake file doesn't add include directories for the dependencies, but it does add the dependencies to the link. So, if using QtGui required the include directories for QtCore, the dependency would exist the user's CMakeLists.txt file. Do you think it makes sense to add the link dependency without adding the include directories for shared Qt libraries? Orion, FindQt4.cmake sets a QT_IS_STATIC variable for the static Qt case.

(0035150) Stephen Kelly (developer) 2014-02-19 09:32	Clinton, I think I need to understand the code before I understand your question. The foreach loop at the bottom of UseQt4.cmake seems to add includes and defines for dependencies. How do I map that to what you wrote? > So, if using QtGui required the include directories for QtCore, the > dependency would exist the user's CMakeLists.txt file. How would that exist in the users CMakeLists.txt file? Please post a snippet.

(0035151) Clinton Stimpson (developer) 2014-02-19 11:16	The for loop adds include directories only if QT_USE_${module} is set. It adds the link library if QT_USE_${module} is set or if it is a dependency for another module with that variable set. For example: cmake_minimum_required(VERSION 2.8) find_package(Qt4 REQUIRED QtGui) include(${QT_USE_FILE}) add_executable(testqt main.cpp) target_link_libraries(testqt ${QT_LIBRARIES}) That will end up with an include directory with QtGui, but not QtCore, but will link with both QtGui and QtCore. If this is used instead find_package(Qt4 REQUIRED QtGui QtCore) then the include directory for QtCore is added.

(0035152) Stephen Kelly (developer) 2014-02-19 11:50	That looks like a bug to my eyes. However, the better bug fix is to use the imported targets anyway. I have no further comment :).

(0035154) Orion Poplawski (reporter) 2014-02-19 15:29	How about this version - it's a lot simpler :). --- cmake-2.8.12.2/Modules/UseQt4.cmake.qtdeps 2014-01-16 10:15:08.000000000 -0700 +++ cmake-2.8.12.2/Modules/UseQt4.cmake 2014-02-19 13:27:21.780219091 -0700 @@ -99,7 +99,9 @@ foreach(module QT3SUPPORT QTOPENGL QTASS include_directories(SYSTEM ${QT_${module}_INCLUDE_DIR}) endif(QT_INCLUDE_DIRS_NO_SYSTEM) endif() - set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIBRARY}) + if(QT_USE_${module} OR QT_IS_STATIC) + set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIBRARY}) + endif() set(QT_LIBRARIES_PLUGINS ${QT_LIBRARIES_PLUGINS} ${QT_${module}_PLUGINS}) if(QT_IS_STATIC) set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIB_DEPENDENCIES})

(0035155) Clinton Stimpson (developer) 2014-02-19 15:48	Thanks, but that doesn't look quite correct. I think we can just move the part that does set(*_DEPENDS 1) to inside the QT_IS_STATIC case. Does this work for you? @@ -102,10 +102,10 @@ foreach(module QT3SUPPORT QTOPENGL QTASSISTANT QTDESIGNER QTMOTIF QTNSPLUGIN set(QT_LIBRARIES_PLUGINS ${QT_LIBRARIES_PLUGINS} ${QT_${module}_PLUGINS}) if(QT_IS_STATIC) set(QT_LIBRARIES ${QT_LIBRARIES} ${QT_${module}_LIB_DEPENDENCIES}) + foreach(depend_module ${QT_${module}_MODULE_DEPENDS}) + set(QT_USE_${depend_module}_DEPENDS 1) + endforeach() endif() - foreach(depend_module ${QT_${module}_MODULE_DEPENDS}) - set(QT_USE_${depend_module}_DEPENDS 1) - endforeach() else () message("Qt ${module} library not found.") endif ()

(0035156) Orion Poplawski (reporter) 2014-02-19 16:00	Well, I think Stephan was correct in that we always want to add in any needed includes and defines. And I'm really just interested in preventing over linkage. So my version (in the note, not attachment - that was a mistaken reposting of the original) only links if we specifically ask for the module or we are building statically.

(0035210) Clinton Stimpson (developer) 2014-02-26 13:05	Fixed in b94ecab.

(0036918) Robert Maynard (manager) 2014-10-06 10:32	Closing resolved issues that have not been updated in more than 4 months.

Relationships