Merge lp:~bregma/mir/build-on-saucy into lp:~mir-team/mir/trunk

[Stephen M. Webb]

Tweaks to get the code base building on Ubuntu 13.10 "Saucy Salamander".

Fixes to get mir building on Saucy Salamander. Mostly due to GCC 4.8 and CMake 2.8.11.

(1) include the required headers where previously they were indirectly included
(2) disables some new GCC 4.8 warnings in the presence of -Werror
(3) avoid depending on the side effects of code executed within an assert() statement, since CMake now sets NDEBUG

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:731
http://jenkins.qa.ubuntu.com/job/mir-ci/697/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/861
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/743
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/182
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/182/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/389

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/697/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

6 +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused")

Many (all?) of the assert() invocations in the code are pre/postcondition checks that are cheap and useful enough to have enabled all the time. I think the proper way to handle this would be to use some other (custom) function/macro that doesn't take NDEBUG into account.

30 +#ifndef NDEBUG

Since NDEBUG is set by CMake now, this test now never runs by default. See point above.

48 + if (pid <= 0)
49 + abort();

Mir uses 4 space indentation.

If it's urgent to get GCC 4.8 support then I am OK with this approach for now, but we should handle this issue in better way in the future.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> 6 +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-unused")
>
> Many (all?) of the assert() invocations in the code are pre/postcondition
> checks that are cheap and useful enough to have enabled all the time. I think
> the proper way to handle this would be to use some other (custom)
> function/macro that doesn't take NDEBUG into account.

All of the asserts (including the one changed) are precondition checks to provide a better diagnostic. I don't believe that we rely on side effects.

If NDEBUG is being set, then we shouldn't be using assert() for this.

> 30 +#ifndef NDEBUG
>
> Since NDEBUG is set by CMake now, this test now never runs by default. See
> point above.

If the test passing depends upon NDEBUG then at least one of the test and the behavior under test is wrong.

> If it's urgent to get GCC 4.8 support then I am OK with this approach for now,
> but we should handle this issue in better way in the future.

+1

review: Needs Fixing

[lp:~bregma/mir/build-on-saucy]

732. By Stephen M. Webb on 2013-06-11

adjusted coding style to match the oral folklore of the project

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:732
http://jenkins.qa.ubuntu.com/job/mir-ci/707/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/873
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/755
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/192
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/192/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/397

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/707/rebuild

review: Approve (continuous-integration)

[lp:~bregma/mir/build-on-saucy]

733. By Stephen M. Webb on 2013-06-11

made removed warning more specific

[Stephen M. Webb (bregma)]

> I don't believe that we rely on side effects.

You may keep your faith. I will choose to rely on reading the code and logs from the ongoing unaddressed build failures in the staging PPA.

I modified the code to conform to the tab style. I also fixed the failing swapper factory test by moving the executed code outside of the assert() macro instead of commenting out the test. A more invasive change to provide a project-specific replacement for the standard assert() macro is beyond the scope of this change, which is targeted at getting mir to build on one its two target platforms (the current Ubuntu desktop development series).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:733
http://jenkins.qa.ubuntu.com/job/mir-ci/709/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/875
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/757/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/194
        deb: http://jenkins.qa.ubuntu.com/job/mir-raring-amd64-ci/194/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/399

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/709/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

Clang says no, because:
[ 26%] Building CXX object examples/CMakeFiles/mirdraw.dir/graphics_utils.cpp.o
clang: error: unknown warning option '-Werror=unused-but-set-variable'; did you mean '-Werror=unused-variable'?

[Daniel van Vugt (vanvugt)]

To fix the clang failure reported by Jenkins, you'll need to change:

6 +set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=unused-but-set-variable")

to something like:

if (NOT "${CMAKE_CXX_COMPILER}" MATCHES "clang")
  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=unused-but-set-variable")
endif()

[Alan Griffiths (alan-griffiths)]

Rather than tactically hiding the issues (e.g. disabling warnings) as proposed here I'd rather see them addressed strategically (fixing the code).

Is there any urgency to get mir building on saucy? Can we take the time to "do it right"?

I confess to a current lack of interest in saucy: I tried loading saucy on my netbook last week, but found that compiz/unity crashed whenever I started some applications.

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> Rather than tactically hiding the issues (e.g. disabling warnings) as proposed
> here I'd rather see them addressed strategically (fixing the code).
>
> Is there any urgency to get mir building on saucy? Can we take the time to "do
> it right"?
>
> I confess to a current lack of interest in saucy: I tried loading saucy on my
> netbook last week, but found that compiz/unity crashed whenever I started some
> applications.

Correct tag "Needs Fixing"

review: Needs Information

[Robert Ancell (robert-ancell)]

> I confess to a current lack of interest in saucy: I tried loading saucy on my
> netbook last week, but found that compiz/unity crashed whenever I started some
> applications.

Ubuntu Touch is moving to Saucy = we're moving to saucy. A number of us have been running it for some time, so please update. There was a major Unity update recently which seems to have fixed some stability issues - please retry.

[Daniel van Vugt (vanvugt)]

That's odd. I just set up a fresh saucy machine and it builds lp:mir without any errors.

Can you please do an update and try again? I suspect this whole proposal is unnecessary.

There are warnings about things in gmock/gtest headers but we expect and ignore those because we'd rather not modify the 3rd_party stuff.

review: Needs Information

[Daniel van Vugt (vanvugt)]

Oh I see the problem now. It fails to build with debuild on saucy.

You can however do a regular cmake/make without any errors. And even cmake -Duse_debflags=ON still succeeds. There's something about debuild that makes it more sensitive though.

review: Abstain

[Daniel van Vugt (vanvugt)]

See here instead:
https://code.launchpad.net/~vanvugt/mir/saucy-build/+merge/169108

review: Disapprove

[Alan Griffiths (alan-griffiths)]

Rejected in favor of https://code.launchpad.net/~vanvugt/mir/saucy-build/+merge/169108

review: Disapprove

Unmerged revisions

733. By Stephen M. Webb on 2013-06-11

made removed warning more specific

732. By Stephen M. Webb on 2013-06-11

adjusted coding style to match the oral folklore of the project

731. By Stephen M. Webb on 2013-06-10

repaired the use of assert() on code with side effects

730. By Stephen M. Webb on 2013-06-10

removed unused-variable warning causing a FTBFS with GCC 4.8

729. By Stephen M. Webb on 2013-06-10

added required header to fix FTBFS

728. By Kevin DuBois on 2013-06-07

Add an algorithm for dynamically switching between two differrent BufferSwapper algorithms.

Approved by PS Jenkins bot, Alan Griffiths, Alexandros Frantzis, Robert Carr, kevin gunn.

727. By Alan Griffiths on 2013-06-07

client: close surface fds when finished with surface.

Approved by Alexandros Frantzis, PS Jenkins bot.

726. By Robert Carr on 2013-06-07

debian/: Add boost program options to dependencies for libmirserver-dev.

Approved by Alan Griffiths, Alexandros Frantzis, PS Jenkins bot.

725. By Daniel van Vugt on 2013-06-06

Make the default cursor more cursor-like.

Approved by Robert Ancell, Alan Griffiths, PS Jenkins bot.

724. By Robert Carr on 2013-06-06

Change client library display sizes to unsigned.

Approved by Robert Ancell, PS Jenkins bot, Alan Griffiths.

=== modified file 'examples/CMakeLists.txt'
--- examples/CMakeLists.txt	2013-05-22 14:19:25 +0000
+++ examples/CMakeLists.txt	2013-06-11 15:08:28 +0000
@@ -1,4 +1,5 @@
 set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -std=c99 -Wall -fno-strict-aliasing -Wextra")
+set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wno-error=unused-but-set-variable")
 
 add_library(eglapp STATIC
   eglapp.c

=== modified file 'src/server/compositor/swapper_factory.cpp'
--- src/server/compositor/swapper_factory.cpp	2013-05-29 23:18:31 +0000
+++ src/server/compositor/swapper_factory.cpp	2013-06-11 15:08:28 +0000
@@ -37,7 +37,8 @@
     gr_allocator(gr_alloc),
     number_of_buffers(2)
 {
-    assert(gr_alloc);
+    if (!gr_allocator)
+        abort();
 }
 
 mc::SwapperFactory::SwapperFactory(
@@ -45,7 +46,8 @@
     gr_allocator(gr_alloc),
     number_of_buffers(number_of_buffers)
 {
-    assert(gr_alloc);
+    if (!gr_allocator)
+        abort();
 }
 
 std::unique_ptr<mc::BufferSwapperMaster> mc::SwapperFactory::create_swapper_master(

=== modified file 'src/server/options/program_option.cpp'
--- src/server/options/program_option.cpp	2013-04-30 23:28:00 +0000
+++ src/server/options/program_option.cpp	2013-06-11 15:08:28 +0000
@@ -21,6 +21,7 @@
 #include <boost/program_options/parsers.hpp>
 
 #include <fstream>
+#include <iostream>
 
 namespace mo = mir::options;
 namespace po = boost::program_options;

=== modified file 'tests/mir_test_framework/process.cpp'
--- tests/mir_test_framework/process.cpp	2012-12-20 09:00:28 +0000
+++ tests/mir_test_framework/process.cpp	2013-06-11 15:08:28 +0000
@@ -66,7 +66,8 @@
     , terminated(false)
     , detached(false)
 {
-    assert(pid > 0);
+    if (pid <= 0)
+        abort();
 }
 
 mtf::Process::~Process()