Merge lp://qastaging/~thomas-voss/unity-mir/switch-to-cmake-take-2 into lp://qastaging/unity-mir

PASSED: Continuous integration, rev:163
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/204/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/67
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/68
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/68/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/67

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/204/rebuild

Here's some I came up with.

First of all there's this:

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror -Wall -pedantic -Wextra -fPIC -pthread")

First of all there's the fPIC, which should not be necessary. Tvoss said this was a typo so it should just be removed.

Second of all, the code specifies -std=c++11 for C++ but not any specific version for C. Maybe it should be -std=c99 because the definitions (stdint etc) go hand in hand.

Third, specifying -pthread manually works, but is not the recommended way to do it. Instead you should do this:

find_package(Threads REQUIRED)
target_link_libraries(threaded-target ${CMAKE_THREAD_LIBS_INIT})

This piece is a bit ugly:

IF(CMAKE_BUILD_TYPE MATCHES [cC][oO][vV][eE][rR][aA][gG][eE])

In other projects (libcolumbus, scopes, etc) we have standardised on doing this:

string(TOLOWER "${CMAKE_BUILD_TYPE}" cmake_build_type_lower)

and then using the lowercased version everywhere. You might consider doing the same, it's a lot nicer.

A few files do not have a newline at the end of file.

Instead of ${GLIB_LIBRARIES} you should use ${GLIB_LDFLAGS}. That brings in the -L flags as well so packages installed to non-standard locations will work.

Pkg-config checks for Qt5 and some others are duplicated. These should be done only once at some top level file.

Also, having -Werror on always is really unpleasant when you are developing. Especially when debugging you need to do unclean things every now and then. Werror interferes with that quite badly.

For unity-scopes-api we enable -Werror only on release and package builds. You might consider the same. We use a CMake option for this, so something along the lines of:

option(Werror "Treat warnings as errors" ON)
if (Werror)
    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror")
    set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
endif()

Thanks for the review and the suggestions.

> Here's some I came up with.
>
> First of all there's this:
>
> set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror -Wall -pedantic -Wextra -fPIC
> -pthread")
>
> First of all there's the fPIC, which should not be necessary. Tvoss said this
> was a typo so it should just be removed.
>

Fixed.

> Second of all, the code specifies -std=c++11 for C++ but not any specific
> version for C. Maybe it should be -std=c99 because the definitions (stdint
> etc) go hand in hand.
>
> Third, specifying -pthread manually works, but is not the recommended way to
> do it. Instead you should do this:
>

Fixed.

> find_package(Threads REQUIRED)
> target_link_libraries(threaded-target ${CMAKE_THREAD_LIBS_INIT})
>

Fixed.

> This piece is a bit ugly:
>
> IF(CMAKE_BUILD_TYPE MATCHES [cC][oO][vV][eE][rR][aA][gG][eE])
>
> In other projects (libcolumbus, scopes, etc) we have standardised on doing
> this:
>
> string(TOLOWER "${CMAKE_BUILD_TYPE}" cmake_build_type_lower)
>
> and then using the lowercased version everywhere. You might consider doing the
> same, it's a lot nicer.
>

Fixed.

> A few files do not have a newline at the end of file.
>
> Instead of ${GLIB_LIBRARIES} you should use ${GLIB_LDFLAGS}. That brings in
> the -L flags as well so packages installed to non-standard locations will
> work.
>

Fixed.

> Pkg-config checks for Qt5 and some others are duplicated. These should be done
> only once at some top level file.

Fixed.

> Also, having -Werror on always is really unpleasant when you are developing.
> Especially when debugging you need to do unclean things every now and then.
> Werror interferes with that quite badly.
>
> For unity-scopes-api we enable -Werror only on release and package builds. You
> might consider the same. We use a CMake option for this, so something along
> the lines of:
>
> option(Werror "Treat warnings as errors" ON)
> if (Werror)
> set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -Werror")
> set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
> endif()

Fixed.

Would you consider adding this line to src/unity-mir/logging.h

#pragma GCC diagnostic ignored "-Wformat"

to silence all the warnings about the DLOG() macro. I plan to replace those macros with qCDebug when Qt5.2 lands.

-Wall seems to have dropped away from this:

set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99 -pedantic -Wextra")

Other than that, looks good to me.

> Would you consider adding this line to src/unity-mir/logging.h
>
> #pragma GCC diagnostic ignored "-Wformat"
>
> to silence all the warnings about the DLOG() macro. I plan to replace those
> macros with qCDebug when Qt5.2 lands.

Fixed. But I would rather not wait for logging macros and instead switch to a reporting approach that would allow us to export to lttng, too. We can take that discussion to a follow-up MP, though.

PASSED: Continuous integration, rev:166
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/207/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/70
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/71
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/71/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/70

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/207/rebuild

Looks good to me, would prefer someone with more unity-mir knowledge to approve though :-)

PASSED: Continuous integration, rev:167
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/213/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/76
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/77
        deb: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/77/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/76

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/213/rebuild

Built, tested and works. Thanking you!