Enable Qt framework in CI #140

konserw · 2017-03-30T10:03:21Z

Main purpose of this PR is to setup Qt framework in CI on all 3 platforms:

Linux via apt
OSX via brew
Windows using preinstalled Qt in appveyor (only mingwx64 is missing on the image)
Rework of Qt-related code in CMake files was needed to prepare for QtTest driver and ensure that CalcQt can be build under all Qt versions.
I've also done some refactoring in appveyor configuration:
removed not used variables
moved build steps to batch file
ruby version is now hard coded instead of variable
I've also changed how travis matrix is described in config but still build targets are the same. I hope now config is more clear.

Please leave a comment how you like those changes.

muggenhor · 2017-03-31T11:17:32Z

CMakeLists.txt

@@ -13,7 +13,9 @@ set(CUKE_ENABLE_EXAMPLES OFF CACHE BOOL "Enable the examples")
 set(GMOCK_SRC_DIR "" CACHE STRING "Google Mock framework sources path (otherwise downloaded)")
 set(GMOCK_VER "1.7.0" CACHE STRING "Google Mock framework version to be used")
 option(VALGRIND_TESTS "Run tests within Valgrind" OFF)
+set(CUKE_DISABLE_QT OFF CACHE BOOL "Disable using Qt framework")


For boolean parameters I recommend using option instead (it's intended to give user options and only supports booleans).

I'll change it

In fact I've changed all BOOLs to options - how do you feel about that?

Sounds good 👍

muggenhor · 2017-03-31T11:17:55Z

CMakeLists.txt


+set(ignoreMe "${QT_QMAKE_EXECUTABLE}") #supress warning


What warning are you suppressing?

Something like Not used variable passed to CMake, and this variable is needed for appveyor CI

I don't think it's a good idea to silence that warning. Because when it gives that warning the parameter you passed on the command line actually isn't used. If that's okay under some circumstances then I would just allow the warning to be displayed.

See my comment about adding %QT_DIR% to CMAKE_PREFIX_PATH in appveyor.bat: I don't think you need to set this variable at all. So please remove this line.

muggenhor · 2017-03-31T11:20:08Z

CMakeLists.txt

+
+    if(${Qt5Core_FOUND} AND ${Qt5Widgets_FOUND} AND ${Qt5Test_FOUND})
+        message(STATUS "Found Qt version: ${Qt5Core_VERSION_STRING}")
+        include_directories(${Qt5Core_INCLUDE_DIRS} ${Qt5Widgets_INCLUDE_DIRS} ${Qt5Test_INCLUDE_DIRS})


Please don't add these to the global search path. Instead use target_include_directories(${target} <PRIVATE|PUBLIC> ${dirs}) for the targets that need it.

Or better yet, only use target_link_libraries(${target} <PRIVATE|PUBLIC> Qt5::${required_component}) because that already sets up the include path correctly.

done :) Didn't know that link_libraries also setup includes

It propagates all properties that have INTERFACE in the name (and when the linked library is a STATIC library also LINK_LIBRARIES). This is nice because it allows you to specify all usage requirements of a target and have CMake do all the work of ensuring those are met in other targets that use that one.

muggenhor · 2017-03-31T11:20:39Z

.travis.yml


 before_install:
-  - if [[ "${TRAVIS_OS_NAME}" = "osx" ]]; then brew update && brew install ninja; fi
+  - if [[ "${TRAVIS_OS_NAME}" = "osx" ]]; then brew update && brew install ninja && brew install qt5; fi


Question: doesn't brew support installing multiple packages at once? E.g. brew install ninja qt5 ?

muggenhor · 2017-03-31T11:21:48Z

CMakeLists.txt

+
+if(NOT CUKE_DISABLE_QT)
+    set(CMAKE_PREFIX_PATH $ENV{QTDIR})
+    find_package(Qt5Core  QUIET)


Why do you search QUIET for these packages? I think it's rather helpful to have the output of these searches shown on the console.

I wasn't sure how to do it, becouse if finding qt5 failed i'm searching for qt4 which is sufficient for the examples. REQUIRED would fail the configuration. Mabey I'll go without quiet

You don't have to specify either one of those. I.e. just this should work:

find_package(Qt5Core)

muggenhor · 2017-03-31T11:23:02Z

CMakeLists.txt

+    if(${Qt5Core_FOUND})
+        find_program(QMAKE_EXECUTABLE NAMES qmake HINTS ${QTDIR} ENV QTDIR PATH_SUFFIXES bin)
+        execute_process(COMMAND ${QMAKE_EXECUTABLE} -query QT_VERSION OUTPUT_VARIABLE QT_VERSION)
+        if(QT_VERSION GREATER 5.6.99)


Please use this instead:

if(NOT QT_VERSION VERSION_LESS 5.7)

That does a proper lexicographic comparison of the version number.

Done - one more thing I've learned today about cmake.

muggenhor · 2017-03-31T11:23:24Z

CMakeLists.txt

@@ -222,4 +249,16 @@ endif()

 if(CUKE_ENABLE_EXAMPLES)
    add_subdirectory(examples)
+
+    #check if c++11 is nedded


Typo: "needed"

muggenhor · 2017-03-31T11:24:16Z

CMakeLists.txt

+    #check if c++11 is nedded
+    if(${Qt5Core_FOUND})
+        find_program(QMAKE_EXECUTABLE NAMES qmake HINTS ${QTDIR} ENV QTDIR PATH_SUFFIXES bin)
+        execute_process(COMMAND ${QMAKE_EXECUTABLE} -query QT_VERSION OUTPUT_VARIABLE QT_VERSION)


Can't you just use Qt5Core_VERSION_STRING instead?

konserw · 2017-03-31T15:02:53Z

I've also updated CMake minimium version as @muggenhor in #142 seems to agree that it is needed

muggenhor · 2017-03-31T15:39:27Z

@konserw:

I've also updated CMake minimium version as @muggenhor in #142 seems to agree that it is needed

Ack.

3.0+ is needed to have find_dependency(), that is used by FindGMock.cmake. 3.1+ is needed to have the Threads package create the Threads::Threads IMPORT target.

konserw · 2017-06-03T12:50:58Z

Should I change anything else?

muggenhor

I just have a few more questions and found two definitive problems (overriding CMAKE_PREFIX_PATH and missing CXX_STANDARD_REQUIRED).

muggenhor · 2017-06-03T13:35:21Z

.travis.yml

-      compiler: gcc #does anyone on osx use it?
+    - os: linux
+      compiler: gcc
+      env: GMOCK_VER=1.8.0


Why are you excluding this configuration?

I thought it wasn't necessary as gmock 1.8 is used for the valgrind build. Reverted it now.

muggenhor · 2017-06-03T13:38:21Z

CMakeLists.txt

+#
+
+if(NOT CUKE_DISABLE_QT)
+    set(CMAKE_PREFIX_PATH $ENV{QTDIR})


I'm assuming you're doing this to be able to use the QTDIR environment variable you've set in .travis.yml for OSX? Could you instead alter travis.sh to pass this variable via the command-line to cmake?

Because right now you're unconditionally overruling this when a user has gone through the effort of setting it via the command line.

I'll try, but I'm not sure how it should be done - via -D switch?

Yes, the approach you've taken in travis.sh now is what I meant.

muggenhor · 2017-06-03T13:45:54Z

CMakeLists.txt

+        if(NOT ${Qt5Core_VERSION_STRING} VERSION_LESS 5.7)
+            message(STATUS "C++11 is needed from Qt version 5.7.0, building with c++11 enabled")
+            set_property(TARGET libcalcqt PROPERTY CXX_STANDARD 11)
+            set_property(TARGET calcqt PROPERTY CXX_STANDARD 11)


This will just ask CMake to use that standard if possible, but won't make it fail if it cannot. So please do this instead (set_target_properties allows setting multiple properties for multiple targets in one call):

set_target_properties(libcalcqt calcqt PROPERTIES CXX_STANDARD 11 CXX_STANDARD_REQUIRED ON )

See CXX_STANDARD_REQUIRED for more details.

Ok - didn't know that

No worries, I had to discover this the hard way myself. Unfortunately CMake's documentation is more of a reference manual than a usage manual.

muggenhor · 2017-06-03T13:47:38Z

CMakeLists.txt

+    if(${Qt5Core_FOUND} AND ${Qt5Widgets_FOUND} AND ${Qt5Test_FOUND})
+        message(STATUS "Found Qt version: ${Qt5Core_VERSION_STRING}")
+        if (Qt5_POSITION_INDEPENDENT_CODE)
+            SET(CMAKE_POSITION_INDEPENDENT_CODE ON)


Could you explain why this is necessary?

And if it is, please limit yourself to setting the POSITION_INDEPENDENT_CODE target property for only those targets that need it: your current approach will set it for everything, including executables, which I doubt need it.

Sorry, it is indeed not needed for this PR. PIC will be required by Qt test driver, i.e. another PR. I'll revert it here.

muggenhor · 2017-06-03T13:56:31Z

CMakeLists.txt

+    #check if c++11 is needed
+    if(${Qt5Core_FOUND})
+#        find_program(QMAKE_EXECUTABLE NAMES qmake HINTS ${QTDIR} ENV QTDIR PATH_SUFFIXES bin)
+#        execute_process(COMMAND ${QMAKE_EXECUTABLE} -query QT_VERSION OUTPUT_VARIABLE QT_VERSION)


don't add commented out code

muggenhor · 2017-06-03T14:01:09Z

appveyor.bat

@@ -0,0 +1,20 @@
+set PATH=C:\Ruby200\bin;%BOOST_LIBRARYDIR%;%PATH%
+if defined MINGW_ROOT set PATH=%MINGW_ROOT%\bin;C:\msys64\usr\bin\;%PATH%
+if defined QT_DIR set PATH=%QT_DIR%\bin;%PATH%


I think it might make more sense to add %QT_DIR% to CMAKE_PREFIX_PATH. That should also remove the need for manually specifying QT_QMAKE_EXECUTABLE.

Ok, I'll check it

muggenhor · 2017-06-03T14:08:15Z

examples/CalcQt/CMakeLists.txt

+    else()
+        qt5_wrap_cpp(CALCQT_MOC ${CALCQT_HEADERS})
+    endif()
+    add_library(libcalcqt src/CalculatorWidget.cpp ${CALCQT_MOC})


I think you can just use AUTOMOC here and get rid of the qtX_wrap_cpp stuff altogether.

PS: this is a change that's fine not to do too as well if it's too much effort. It can be done separately at another time too.

I thought you would like such self-contined implementation better. Automoc is fine by me :)

muggenhor · 2017-08-12T17:28:08Z

@konserw have you looked at my questions?

konserw · 2017-08-13T20:08:09Z

@muggenhor - to be honest I just forgot about all that. Now I have just pushed changes to address outlined issues. I hope CI will be green 🤞

konserw · 2017-08-14T07:19:22Z

It was all green so I've rebased against master

muggenhor

Nice work, it looks really clean now. Just a few small things left.

muggenhor · 2017-08-15T07:59:17Z

appveyor.bat

@@ -0,0 +1,20 @@
+set PATH=C:\Ruby200\bin;%BOOST_LIBRARYDIR%;%PATH%
+if defined MINGW_ROOT set PATH=%MINGW_ROOT%\bin;C:\msys64\usr\bin\;%PATH%
+REM if defined QT_DIR set PATH=%QT_DIR%\bin;%PATH%


Please remove this commented out code

Missed that one :)

muggenhor · 2017-08-15T07:59:47Z

appveyor.bat

+if defined MINGW_ROOT set PATH=%MINGW_ROOT%\bin;C:\msys64\usr\bin\;%PATH%
+REM if defined QT_DIR set PATH=%QT_DIR%\bin;%PATH%
+if "%CMAKE_GENERATOR%"=="NMake Makefiles" call "%VS140COMNTOOLS%\..\..\VC\vcvarsall.bat" %PLATFORM%
+echo %PATH%


Looks like left-over printf-style debugging code, please remove it.

muggenhor · 2017-08-15T08:01:10Z

appveyor.yml

- set PATH=C:\Ruby%RUBY_VERSION%\bin;%BOOST_LIBRARYDIR%;%PATH%
- gem install bundle
- bundle install
- bundle env


I don't see bundle env reappearing in appveyor.bat. Why is that?

it is as much debugging by print as echo %path%. This is what it looks like on my machine:
$ bundle env

Environment

Bundler 1.15.3 Rubygems 2.6.11 Ruby 2.4.1p111 (2017-03-22 revision 58053) [x86_64-linux] Git 2.14.1 Platform x86_64-linux OpenSSL OpenSSL 1.1.0e 16 Feb 2017

Okay, removing it is fine then.

muggenhor · 2017-08-15T08:06:11Z

examples/CalcQt/CMakeLists.txt

-
-    add_library(libcalcqt src/CalculatorWidget.cpp ${CALCQT_HEADERS})
+if(QT_LIBRARIES)
+    add_library(libcalcqt src/CalculatorWidget.cpp)


I'm expecting src/CalculatorWidget.h to be present here as well. Does automoc work without specifying the header file to run Qt's moc on?

Also please remove the now unused CALCQT_HEADERS variable.

muggenhor · 2017-08-15T08:08:38Z

appveyor.yml

    MINGW_ARCH: x86_64
-    MSYSTEM: MINGW64
    MINGW_ROOT: C:\msys64\mingw64
    BOOST_ROOT: C:\msys64\mingw64
    BOOST_LIBRARYDIR: C:\msys64\mingw64\lib
    BOOST_INCLUDEDIR: C:\msys64\mingw64\include\boost


Shouldn't there be a QT_DIR variable here (for 64-bit MinGW)?

Appveyor has only 32-bit mingw-compiled qt libs preinstalled. https://www.appveyor.com/docs/build-environment/#qt
AFAIK it is not possible to link VC-compiled Qt with Mingw-compiled cucumber, or use 32-bit mingw-compiled lib.

Thanks for your explanation

muggenhor · 2017-08-15T17:11:19Z

examples/CalcQt/CMakeLists.txt

@@ -1,36 +1,14 @@
 project(CalcQt)

-set(CALCQT_HEADERS src/CalculatorWidget.h)
-set(CALCQT_SOURCES src/CalcQt.cpp src/CalculatorWidget.cpp)
-include_directories(${CUKE_INCLUDE_DIRS} src)


I like that you remove this global include_directories but you should probably combine it with target_include_directories(libcalcqt PUBLIC src) to fix the build errors you get.

thank you for pointing that - I haven't yet used target_include_directories

muggenhor · 2017-08-15T17:12:17Z

examples/CalcQt/CMakeLists.txt

    target_link_libraries(libcalcqt ${QT_LIBRARIES})

-    add_executable(calcqt ${CALCQT_SOURCES})
+    add_executable(calcqt src/CalcQt.cpp src/CalculatorWidget.h)


I don't think you should be adding the CalculatorWidget.h header file to this executable: it's logically part of the libcalcqt library, not this.

of course - it's my mistake when cut-pasting ;)

muggenhor

Looks good, with these last requested changes I'll merge it.

muggenhor · 2017-08-16T09:03:27Z

CMakeLists.txt

+        endif()
+    endif()
+    if(QT_LIBRARIES)
+        set(CMAKE_AUTOMOC ON)


Unfortunately this causes it to add a ton of extra files to compile (one for every target). Can you remove this and instead add this to examples/CalcQt/CMakeLists.txt?

set_target_properties(libcalcqt PROPERTIES AUTOMOC ON)

And this is why I avoided automoc at first - didn't know it can be done with target property.

muggenhor · 2017-08-16T09:25:52Z

CMakeLists.txt

+    if(${Qt5Core_FOUND})
+        if(NOT ${Qt5Core_VERSION_STRING} VERSION_LESS 5.7)
+            message(STATUS "C++11 is needed from Qt version 5.7.0, building with c++11 enabled")
+            set_target_properties(calcqt libcalcqt PROPERTIES


Looking at the Qt5CoreConfigExtras.cmake file in Qt 5.7.1 (as present on Debian stretch) this shouldn't be necessary because of this line:

set_property(TARGET Qt5::Core PROPERTY INTERFACE_COMPILE_FEATURES cxx_decltype)

That makes it a usage requirement (INTERFACE_\w+ are properties describing usage requirements automatically applied by CMake) to always compile this target with a compiler supporting decltype, that means C++11 (or C++0x on some very old compilers, but if that's not enough then that entire compiler won't work anyway). So it should already automatically switch to using C++11 for every target that links to Qt5::Core (via target_link_libraries).

So please remove this entire block.

muggenhor · 2017-08-16T10:07:04Z

One problem, since you're changes to the AppVeyor config failures are no longer fatal (search for "undefined reference"): https://ci.appveyor.com/project/paoloambrosio/cucumber-cpp-qqrt7/build/master-ci-249/job/fxe93x2pmd8ewhlc

muggenhor · 2017-08-16T14:38:27Z

One problem, since you're changes to the AppVeyor config failures are no longer fatal (search for "undefined reference"): https://ci.appveyor.com/project/paoloambrosio/cucumber-cpp-qqrt7/build/master-ci-249/job/fxe93x2pmd8ewhlc

I think this is caused by your move of the script to appveyor.bat. Additionally that has the effect of no longer showing which command is being executed. Is there any particular reason why you've moved all this out of appveyor.yml and into the batch file?

konserw · 2017-08-16T19:57:35Z

@muggenhor - I thought it was good idea to separate configuration from build script like we have done with travis. It just turned out badly with cmd instead of bash - merged it again - I hope it'll help.

muggenhor

I'd appreciate it if you could logically group your changes, because right now you have cleanup, build system changes and CI config changes mixed together.

I have that, rebased to master, in this private branch muggenhor/pull-140-test. It's combined with the fixes suggested in this review. That has the advantage that everything except for the last commit in there could already be merged, because it's only the last commit (which only changes the CI config to actually use Qt) that breaks the build on AppVeyor. That last commit (muggenhor/cucumber-cpp@b2a0b0d78d4669e129f99179ab0efebea396980a) could even be split to have the Travis stuff first (which works) and the AppVeyor stuff last (which breaks only for MinGW + Qt). Feel free to use it, it's just your work separated into different commits.

muggenhor · 2017-08-17T11:22:07Z

CMakeLists.txt


+set(ignoreMe "${QT_QMAKE_EXECUTABLE}") #supress warning


Please remove this line now

muggenhor · 2017-08-17T11:24:40Z

appveyor.yml

- if "%build%"=="mingw" set PATH=%MINGW_ROOT%\bin;C:\msys64\usr\bin\;%PATH%
- if "%build%"=="mingw" bash -lc "pacman --needed --noconfirm -S mingw-w64-%MINGW_ARCH%-boost
+- call gem install bundle
+- call bundle install


Please don't shuffle the lines around (in comparison to master), that makes the diff unnecessarily large.

muggenhor · 2017-08-17T11:26:00Z

appveyor.yml

    MINGW_ROOT: C:\msys64\mingw64
    BOOST_ROOT: C:\msys64\mingw64
    BOOST_LIBRARYDIR: C:\msys64\mingw64\lib
    BOOST_INCLUDEDIR: C:\msys64\mingw64\include\boost
-    CMAKE_GENERATOR: 'MSYS Makefiles'
-  - build: msvc
+  - CMAKE_GENERATOR: 'NMake Makefiles'
    platform: x86


How about renaming this to MSVC_ARCH (similarly to MINGW_ARCH)? Then you can change the if %CMAKE_GENERATOR% == NMake check below to check for if defined MSVC_ARCH just like the MinGW stuff.

konserw · 2017-08-17T18:34:57Z

@muggenhor - very nice work! Of course I agree with your review. And I think I've found solution to that failing build - it was order in which libraries were added for linking. I'll create new pull request based off your branch and close this one.

This was referenced Mar 30, 2017

QtTest driver v2 #142

Closed

QtTest driver #110

Closed

muggenhor requested changes Mar 31, 2017

View reviewed changes

konserw mentioned this pull request Jun 3, 2017

Fix our CMake version dependency: 3.1 #152

Merged

muggenhor added the feature label Jun 3, 2017

muggenhor requested changes Jun 3, 2017

View reviewed changes

muggenhor added build and removed feature labels Jun 3, 2017

muggenhor mentioned this pull request Jun 3, 2017

CMake 2.8.12 isn't enough: 3.1 is needed #153

Closed

muggenhor added this to the v0.4.1 milestone Jun 3, 2017

Rework CMake logic around Qt and use Qt in CI

e548110

konserw force-pushed the cirework branch from a4c3e19 to e548110 Compare August 14, 2017 07:17

muggenhor requested changes Aug 15, 2017

View reviewed changes

review fixes

0f9c6ed

muggenhor requested changes Aug 15, 2017

View reviewed changes

fix

3d3177e

muggenhor requested changes Aug 16, 2017

View reviewed changes

merge appveyor bat into yml; other review fixes

ba7d83a

remove cmd

bc3da13

muggenhor requested changes Aug 17, 2017

View reviewed changes

konserw closed this Aug 17, 2017

konserw mentioned this pull request Aug 17, 2017

Enable Qt framework in CI and refactoring of CI configuration #160

Merged

konserw deleted the cirework branch April 2, 2018 10:06

Enable Qt framework in CI #140

Enable Qt framework in CI #140

Conversation

konserw commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konserw commented Mar 31, 2017

muggenhor commented Mar 31, 2017

konserw commented Jun 3, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor commented Aug 12, 2017

konserw commented Aug 13, 2017

konserw commented Aug 14, 2017

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Environment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muggenhor commented Aug 16, 2017

muggenhor commented Aug 16, 2017

konserw commented Aug 16, 2017

muggenhor left a comment

muggenhor Aug 15, 2017 •

edited

Loading

muggenhor Jun 3, 2017 •

edited

Loading