Mir

Merge lp:~afrantzis/mir/better-client-api-override into lp:mir

better-client-api-override
Merge into development-branch

Proposed by Alexandros Frantzis on 2014-08-04

Status:	Merged
Approved by:	Alexandros Frantzis on 2014-08-11
Approved revision:	no longer in the source branch.
Merged at revision:	1834
Proposed branch:	lp:~afrantzis/mir/better-client-api-override
Merge into:	lp:mir
Diff against target:	447 lines (+180/-163) 6 files modified include/test/mir_test_framework/using_stub_client_platform.h (+11/-3) src/client/api_impl.h (+0/-27) src/client/api_impl_types.h (+0/-32) src/client/mir_connection_api.cpp (+73/-66) src/client/mir_connection_api.h (+59/-0) tests/mir_test_framework/using_stub_client_platform.cpp (+37/-35)
To merge this branch:	bzr merge lp:~afrantzis/mir/better-client-api-override
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-08-11
Chris Halse Rogers			Approve on 2014-08-07
Kevin DuBois (community)			Approve on 2014-08-07
Alan Griffiths		2014-08-04	Approve on 2014-08-06
Review via email: mp+229479@code.launchpad.net

Commit message

client: Rework mechanism to override Mir client functions

Description of the change

client: Rework mechanism to override Mir client functions

In addition to being more versatile and powerful in general, the proposed approach is needed so that we can fix the tests for [1] (which is currently WIP) in a more sane way. I have provided override points only for connect, release (which existed before) and a new one (the ConnectionConfiguration to use in the default connect implementation). We can add new override points as needed.

IMPORTANT UPDATE: it turns out that because of recent changes in how we build unit and integration tests, client global code is executed twice: once from the client code that was compiled in mir_unit_tests/mir_integration_tests and once because we indirectly load libmirclient.so. This means that constructors
and destructors of global objects are run twice, causing issues. I have worked around this issue in r1814, but this problem may come back to bite us at later time, so it's worth some further consideration (not as part of this MP please, unless the proposed workaround is not acceptable).

[1] https://code.launchpad.net/~afrantzis/mir/client-lifecycle-terminate-properly/+merge/229234

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-04:

FAILED: Continuous integration, rev:1813
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/2303/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1184
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1190
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1172/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-ci/825/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-ci/826/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/107/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2303/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/2315/rebuild

review: Approve (continuous-integration)

Revision history for this message

Alan Griffiths (alan-griffiths) wrote on 2014-08-06:

I've no objection to this step (the direction is sane), but I've a feeling that this and the [Default]ConnectionConfiguration stuff all belongs in one customization point.

review: Approve

Revision history for this message

Kevin DuBois (kdub) wrote on 2014-08-07:

hah, I was just staring at this today when writing an integration test, and thought that this should be done. looks good to me.

review: Approve

Revision history for this message

Chris Halse Rogers (raof) wrote on 2014-08-07:

Looks sensible to me.

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-08:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-08-08:

Oh, no...

Setting up libmirplatform1:armhf (0.6.0bzr1815pkg0utopic172+autopilot0) ...
dpkg: dependency problems prevent configuration of libmirplatform-dev:armhf:
libmirplatform-dev:armhf depends on libboost-program-options-dev; however:
Package libboost-program-options-dev is not installed.

dpkg: error processing package libmirplatform-dev:armhf (--install):
dependency problems - leaving unconfigured
dpkg: libmirplatform:armhf: dependency problems, but configuring anyway as you requested:
libmirplatform-dev:armhf (0.6.0bzr1815pkg0utopic172+autopilot0) breaks libmirplatform (<= 0.5.1+14.10.20140728-0ubuntu1) and is unpacked but not configured.
Version of libmirplatform:armhf to be configured is 0.5.1+14.10.20140728-0ubuntu1.

Setting up libmirplatform:armhf (0.5.1+14.10.20140728-0ubuntu1) ...
Setting up libmirplatformgraphics-android:armhf (0.6.0bzr1815pkg0utopic172+autopilot0) ...
Setting up libmirplatformgraphics-mesa:armhf (0.6.0bzr1815pkg0utopic172+autopilot0) ...
dpkg: dependency problems prevent configuration of libmirserver-dev:armhf:
libmirserver-dev:armhf depends on libmirplatform-dev (= 0.6.0bzr1815pkg0utopic172+autopilot0); however:
Package libmirplatform-dev:armhf is not configured yet.

dpkg: error processing package libmirserver-dev:armhf (--install):
dependency problems - leaving unconfigured

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-08:

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-autolanding/959/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/1257
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/1263
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1246/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-amd64-autolanding/343/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-utopic-armhf-autolanding/343/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/181/console

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-08:

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-08-11:

review: Needs Fixing (continuous-integration)

Revision history for this message

Alexandros Frantzis (afrantzis) wrote on 2014-08-11:

Started by upstream project "mir-mediumtests-utopic-touch" build number 1256
Building remotely on cyclops-node07 in workspace /home/ubuntu/jenkins/workspace/mir-mediumtests-builder-utopic-armhf
java.io.IOException: Failed to mkdirs: /home/ubuntu/jenkins/workspace/mir-mediumtests-builder-utopic-armhf
at hudson.FilePath.mkdirs(FilePath.java:911)
at hudson.model.AbstractProject.checkout(AbstractProject.java:1254)
at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:590)
at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:88)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:495)
at hudson.model.Run.execute(Run.java:1502)
at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:46)
at hudson.model.ResourceController.execute(ResourceController.java:88)
at hudson.model.Executor.run(Executor.java:237)
Archiving artifacts
Build was marked for publishing on https://jenkins.qa.ubuntu.com/
Finished: FAILURE

Revision history for this message

PS Jenkins bot (ps-jenkins) on 2014-08-11:

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Alexandros Frantzis

Emanuele Antonio Faraone

Gerry Boland

Mir development team

 === modified file 'include/test/mir_test_framework/using_stub_client_platform.h'
 --- include/test/mir_test_framework/using_stub_client_platform.h	2014-01-29 13:06:25 +0000
 +++ include/test/mir_test_framework/using_stub_client_platform.h	2014-08-05 10:18:32 +0000
@@ -19,7 +19,15 @@
  #ifndef MIR_TEST_FRAMEWORK_USING_STUB_CLIENT_PLATFORM_H_
  #define MIR_TEST_FRAMEWORK_USING_STUB_CLIENT_PLATFORM_H_
--#include "src/client/api_impl_types.h"
++#include <memory>
++
++namespace mir
++{
++namespace client
++{
++class MirConnectionAPI;
++}
++}
  namespace mir_test_framework
+ {
@@ -34,8 +42,8 @@
      UsingStubClientPlatform(UsingStubClientPlatform const&) = delete;
      UsingStubClientPlatform operator=(UsingStubClientPlatform const&) = delete;
--    mir_connect_impl_func const prev_mir_connect_impl;
--    mir_connection_release_impl_func const prev_mir_connection_release_impl;
++    mir::client::MirConnectionAPI* prev_api;
++    std::unique_ptr<mir::client::MirConnectionAPI> stub_api;
  };
+ }
 === removed file 'src/client/api_impl.h'
 --- src/client/api_impl.h	2014-01-28 18:21:14 +0000
 +++ src/client/api_impl.h	1970-01-01 00:00:00 +0000
@@ -1,27 +0,0 @@
--/*
-- * Copyright © 2014 Canonical Ltd.
-- *
-- * This program is free software: you can redistribute it and/or modify it
-- * under the terms of the GNU Lesser General Public License version 3,
-- * as published by the Free Software Foundation.
-- *
-- * This program is distributed in the hope that it will be useful,
-- * but WITHOUT ANY WARRANTY; without even the implied warranty of
-- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-- * GNU Lesser General Public License for more details.
-- *
-- * You should have received a copy of the GNU Lesser General Public License
-- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
-- *
-- * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
-- */
--
--#ifndef MIR_CLIENT_API_IMPL_H_
--#define MIR_CLIENT_API_IMPL_H_
--
--#include "api_impl_types.h"
--
--extern mir_connect_impl_func mir_connect_impl;
--extern mir_connection_release_impl_func mir_connection_release_impl;
--
--#endif /* MIR_CLIENT_API_IMPL_H_ */
 === removed file 'src/client/api_impl_types.h'
 --- src/client/api_impl_types.h	2014-01-28 18:21:14 +0000
 +++ src/client/api_impl_types.h	1970-01-01 00:00:00 +0000
@@ -1,32 +0,0 @@
--/*
-- * Copyright © 2014 Canonical Ltd.
-- *
-- * This program is free software: you can redistribute it and/or modify it
-- * under the terms of the GNU Lesser General Public License version 3,
-- * as published by the Free Software Foundation.
-- *
-- * This program is distributed in the hope that it will be useful,
-- * but WITHOUT ANY WARRANTY; without even the implied warranty of
-- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-- * GNU Lesser General Public License for more details.
-- *
-- * You should have received a copy of the GNU Lesser General Public License
-- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
-- *
-- * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
-- */
--
--#ifndef MIR_CLIENT_API_IMPL_TYPES_H_
--#define MIR_CLIENT_API_IMPL_TYPES_H_
--
--#include "mir_toolkit/client_types.h"
--
--typedef MirWaitHandle* (*mir_connect_impl_func)(
--     char const *server,
--     char const *app_name,
--     mir_connected_callback callback,
--     void *context);
--
--typedef void (*mir_connection_release_impl_func)(MirConnection *connection);
--
--#endif /* MIR_CLIENT_API_IMPL_TYPES_H_ */
 === modified file 'src/client/mir_connection_api.cpp'
 --- src/client/mir_connection_api.cpp	2014-06-09 13:48:18 +0000
 +++ src/client/mir_connection_api.cpp	2014-08-05 10:18:32 +0000
@@ -16,6 +16,7 @@
   * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
   */
++#include "mir_connection_api.h"
  #include "mir_toolkit/mir_connection.h"
  #include "mir_toolkit/mir_client_library_drm.h"
  #include "mir/default_configuration.h"
@@ -25,7 +26,6 @@
  //#include "egl_native_display_container.h"
  #include "default_connection_configuration.h"
  #include "display_configuration.h"
--#include "api_impl_types.h"
  #include "error_connections.h"
@@ -49,78 +49,85 @@
      return ((a - 1) / b) + 1;
+ }
--
--MirWaitHandle* mir_default_connect(
--    char const* socket_file,
--    char const* name,
--    mir_connected_callback callback,
--    void* context)
++class DefaultMirConnectionAPI : public mcl::MirConnectionAPI
+ {
--
--    try
--    {
--        std::string sock;
--        if (socket_file)
--            sock = socket_file;
++public:
++    MirWaitHandle* connect(
++        char const* socket_file,
++        char const* name,
++        mir_connected_callback callback,
++        void* context) override
++    {
++        try
++        {
++            std::string sock;
++            if (socket_file)
++                sock = socket_file;
++            else
++            {
++                auto socket_env = getenv("MIR_SOCKET");
++                if (socket_env)
++                    sock = socket_env;
++                else
++                    sock = mir::default_server_socket;
++            }
++
++            auto const conf = configuration(sock);
++
++            std::unique_ptr<MirConnection> connection{new MirConnection(*conf)};
++            auto const result = connection->connect(name, callback, context);
++            connection.release();
++            return result;
++        }
++        catch (std::exception const& x)
++        {
++            MirConnection* error_connection = new MirConnection(x.what());
++            mcl::ErrorConnections::instance().insert(error_connection);
++            callback(error_connection, context);
++            return nullptr;
++        }
++    }
++
++    void release(MirConnection* connection) override
++    {
++        if (!mcl::ErrorConnections::instance().contains(connection))
++        {
++            try
++            {
++                auto wait_handle = connection->disconnect();
++                wait_handle->wait_for_all();
++            }
++            catch (std::exception const&)
++            {
++                // We're implementing a C API so no exceptions are to be
++                // propagated. And that's OK because if disconnect() fails,
++                // we don't care why. We're finished with the connection anyway.
++            }
++        }
          else
+         {
--            auto socket_env = getenv("MIR_SOCKET");
--            if (socket_env)
--                sock = socket_env;
--            else
--                sock = mir::default_server_socket;
++            mcl::ErrorConnections::instance().remove(connection);
+         }
--        mcl::DefaultConnectionConfiguration conf{sock};
--
--        std::unique_ptr<MirConnection> connection{new MirConnection(conf)};
--        auto const result = connection->connect(name, callback, context);
--        connection.release();
--        return result;
++        delete connection;
+     }
--    catch (std::exception const& x)
++
++    std::unique_ptr<mcl::ConnectionConfiguration> configuration(std::string const& socket) override
+     {
--        MirConnection* error_connection = new MirConnection(x.what());
--        mcl::ErrorConnections::instance().insert(error_connection);
--        callback(error_connection, context);
--        return nullptr;
++        return std::unique_ptr<mcl::ConnectionConfiguration>{
++            new mcl::DefaultConnectionConfiguration{socket}};
+     }
--}
--
--
--void mir_default_connection_release(MirConnection* connection)
++};
++
++mcl::MirConnectionAPI* mir_connection_api_impl_default()
+ {
--    if (!mcl::ErrorConnections::instance().contains(connection))
--    {
--        try
--        {
--            auto wait_handle = connection->disconnect();
--            wait_handle->wait_for_all();
--        }
--        catch (std::exception const&)
--        {
--            // We're implementing a C API so no exceptions are to be
--            // propagated. And that's OK because if disconnect() fails,
--            // we don't care why. We're finished with the connection anyway.
--        }
--    }
--    else
--    {
--        mcl::ErrorConnections::instance().remove(connection);
--    }
--
--    delete connection;
--}
--
--}
--
--//mir_connect and mir_connection_release can be overridden by test code that sets these function
--//pointers to do things like stub out the graphics drivers or change the connection configuration.
--
--//TODO: we could have a more comprehensive solution that allows us to substitute any of the functions
--//for test purposes, not just the connect functions
--mir_connect_impl_func mir_connect_impl = mir_default_connect;
--mir_connection_release_impl_func mir_connection_release_impl = mir_default_connection_release;
++    static DefaultMirConnectionAPI default_api;
++    return &default_api;
++}
++
++}
++
++mcl::MirConnectionAPI* mir_connection_api_impl{mir_connection_api_impl_default()};
  MirWaitHandle* mir_connect(
      char const* socket_file,
@@ -130,7 +137,7 @@
+ {
      try
+     {
--        return mir_connect_impl(socket_file, name, callback, context);
++        return mir_connection_api_impl->connect(socket_file, name, callback, context);
+     }
      catch (std::exception const&)
+     {
@@ -164,7 +171,7 @@
+ {
      try
+     {
--        return mir_connection_release_impl(connection);
++        return mir_connection_api_impl->release(connection);
+     }
      catch (std::exception const&)
+     {
 === added file 'src/client/mir_connection_api.h'
 --- src/client/mir_connection_api.h	1970-01-01 00:00:00 +0000
 +++ src/client/mir_connection_api.h	2014-08-05 10:18:32 +0000
@@ -0,0 +1,59 @@
++/*
++ * Copyright © 2014 Canonical Ltd.
++ *
++ * This program is free software: you can redistribute it and/or modify it
++ * under the terms of the GNU Lesser General Public License version 3,
++ * as published by the Free Software Foundation.
++ *
++ * This program is distributed in the hope that it will be useful,
++ * but WITHOUT ANY WARRANTY; without even the implied warranty of
++ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
++ * GNU Lesser General Public License for more details.
++ *
++ * You should have received a copy of the GNU Lesser General Public License
++ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
++ *
++ * Authored by: Alexandros Frantzis <alexandros.frantzis@canonical.com>
++ */
++
++#ifndef MIR_CLIENT_MIR_CONNECTION_API_H_
++#define MIR_CLIENT_MIR_CONNECTION_API_H_
++
++#include "mir_toolkit/mir_wait.h"
++
++#include <string>
++#include <memory>
++
++namespace mir
++{
++namespace client
++{
++class ConnectionConfiguration;
++
++class MirConnectionAPI
++{
++public:
++    virtual ~MirConnectionAPI() = default;
++
++    virtual MirWaitHandle* connect(
++        char const* socket_file,
++        char const* name,
++        mir_connected_callback callback,
++        void* context) = 0;
++
++    virtual void release(MirConnection* connection) = 0;
++
++    virtual std::unique_ptr<ConnectionConfiguration> configuration(std::string const& socket) = 0;
++
++protected:
++    MirConnectionAPI() = default;
++    MirConnectionAPI(MirConnectionAPI const&) = default;
++    MirConnectionAPI& operator=(MirConnectionAPI const&) = default;
++};
++
++}
++}
++
++extern "C" mir::client::MirConnectionAPI* mir_connection_api_impl;
++
++#endif
 === modified file 'tests/mir_test_framework/using_stub_client_platform.cpp'
 --- tests/mir_test_framework/using_stub_client_platform.cpp	2014-07-29 10:25:43 +0000
 +++ tests/mir_test_framework/using_stub_client_platform.cpp	2014-08-05 10:18:32 +0000
@@ -18,54 +18,56 @@
  #include "mir_test_framework/using_stub_client_platform.h"
  #include "mir_test_framework/stub_client_connection_configuration.h"
--#include "src/client/mir_wait_handle.h"
--#include "src/client/mir_connection.h"
--#include "src/client/api_impl.h"
++#include "src/client/mir_connection_api.h"
  namespace mtf = mir_test_framework;
++namespace mcl = mir::client;
  namespace
+ {
--MirWaitHandle* mir_connect_override(
--    char const *socket_file,
--    char const *app_name,
--    mir_connected_callback callback,
--    void *context)
--{
--    mtf::StubConnectionConfiguration conf(socket_file);
--    auto connection = new MirConnection(conf);
--    return connection->connect(app_name, callback, context);
--}
--
--void mir_connection_release_override(MirConnection *connection)
--{
--    try
--    {
--        auto wait_handle = connection->disconnect();
--        mir_wait_for(wait_handle);
--    }
--    catch (std::exception const&)
--    {
--        // Really, we want try/finally, but that's not C++11
--        delete connection;
--        throw;
--    }
--    delete connection;
--}
++class StubMirConnectionAPI : public mcl::MirConnectionAPI
++{
++public:
++    StubMirConnectionAPI(mcl::MirConnectionAPI* prev_api)
++        : prev_api{prev_api}
++    {
++    }
++
++    MirWaitHandle* connect(
++        char const* socket_file,
++        char const* name,
++        mir_connected_callback callback,
++        void* context) override
++    {
++        return prev_api->connect(socket_file, name, callback, context);
++    }
++
++    void release(MirConnection* connection) override
++    {
++        return prev_api->release(connection);
++    }
++
++    std::unique_ptr<mcl::ConnectionConfiguration> configuration(std::string const& socket) override
++    {
++        return std::unique_ptr<mcl::ConnectionConfiguration>{
++            new mtf::StubConnectionConfiguration{socket}};
++    }
++
++private:
++    mcl::MirConnectionAPI* const prev_api;
++};
+ }
  mtf::UsingStubClientPlatform::UsingStubClientPlatform()
--    : prev_mir_connect_impl{mir_connect_impl},
--      prev_mir_connection_release_impl{mir_connection_release_impl}
++    : prev_api{mir_connection_api_impl},
++      stub_api{new StubMirConnectionAPI{prev_api}}
+ {
--    mir_connect_impl = mir_connect_override;
--    mir_connection_release_impl = mir_connection_release_override;
++    mir_connection_api_impl = stub_api.get();
+ }
  mtf::UsingStubClientPlatform::~UsingStubClientPlatform()
+ {
--    mir_connect_impl = prev_mir_connect_impl;
--    mir_connection_release_impl = prev_mir_connection_release_impl;
++    mir_connection_api_impl = prev_api;
+ }