Code review comment for lp:~saviq/dee-qt/rename-libs

Revision history for this message
Albert Astals Cid (aacid) wrote :

You still have OUR_QT_TEST_LIB in the root CMakeLists.txt and seems we don't need it there since the uses where moved to tests/CMakeLists.txt, same for the use of the test includes inside OUR_QT_INCLUDES

You don't need the enable_testing in tests/CMakeLists.txt, the one in the root CMakeLists.txt is enough

Also I think that you don't need to concatenate OUR_QT_INCLUDES like you do in tests/CMakeLists.txt, you can just use a new variable and use it in include_directories, as far as i read the include_directories entry in the cmake man page the include_directories commands work like a stack so if the "old" OUR_QT_INCLUDES were already included in the root you'll already have them, just need to include the new stuff

review: Needs Fixing

« Back to merge proposal