Merge lp:~mir-team/mir/remove-global-constructor-socket into lp:mir

[Brandon Schaefer]

Remove the global default_server_socket and replace it with a function. It causes issues when you dlopen/dlclose on mir common during tests. Issues invalid reading of the strings.

Remove the global default_server_socket and replace it with a function. It causes issues when you dlopen/dlclose on mir common during tests. Issues invalid reading of the strings.

Example issue:
https://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1206/console

[lp:~mir-team/mir/remove-global-constructor-socket]

2979. By Brandon Schaefer on 2015-09-30

* Remove static str, as if we ever need to change socket during a test this would fail

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2978
http://jenkins.qa.ubuntu.com/job/mir-ci/5076/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4197
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3104
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4139
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/97/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1227/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/97/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4140
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4140/artifact/work/output/*zip*/output.zip
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6816/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23806
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/98/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2979
http://jenkins.qa.ubuntu.com/job/mir-ci/5078/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4200
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3107
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4142/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/100/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1229/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/100/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4143/console
    None: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/101/console

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

review: Needs Fixing (continuous-integration)

[lp:~mir-team/mir/remove-global-constructor-socket]

2980. By Brandon Schaefer on 2015-09-30

* Move back to the orig symbol spot

2981. By Brandon Schaefer on 2015-09-30

* Undo that change

[Andreas Pokorny (andreas-pokorny)]

That change still looks reasonable, and the ci problem unrelated.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2981
http://jenkins.qa.ubuntu.com/job/mir-ci/5082/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4204
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3111
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4146/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/104/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1233/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/104/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4147
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4147/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6823/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23817
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/105/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2981
http://jenkins.qa.ubuntu.com/job/mir-ci/5088/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/4211
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/3118
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/4153/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-wily-touch/111/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1242
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/1242/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-wily-i386-ci/111/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4154
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/4154/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6830/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23834
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-wily-armhf/112/console

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I'm not sure this is well motivated.

The test case that triggers the issue this addresses is one that can hardly be described as a "unit test". The only reason I can see for adding it to that binary is the use of umockdev (and that usage for *unit* tests is already controversial).

The unit tests already statically link a *lot* of the contents of libmircommon and this leads to ODR violations.

[Alan Griffiths (alan-griffiths)]

- mir::default_server_socket;

This is an ABI break. We should bump the soname, consolidate the stanzas, etc.

(But see above comment as I'm not sure this "solution" is enabling something we want to do.)

review: Needs Fixing

[Brandon Schaefer (brandontschaefer)]

Sounds good, ill be moving the test (that needed this) over to acceptable tests once umockdev tests have landed there.

We should just be aware (should i make a comment/bug somewhere?) that if a server is ran after the test_module_deleter this will cause invalid reads on global strings.

Unmerged revisions

2981. By Brandon Schaefer on 2015-09-30

* Undo that change

2980. By Brandon Schaefer on 2015-09-30

* Move back to the orig symbol spot

2979. By Brandon Schaefer on 2015-09-30

* Remove static str, as if we ever need to change socket during a test this would fail

2978. By Brandon Schaefer on 2015-09-30

* Dont use global constructors, causes issues when you dlopen/dlclose on the mircommon during tests

=== modified file 'src/client/mir_connection_api.cpp'
--- src/client/mir_connection_api.cpp	2015-07-25 08:10:48 +0000
+++ src/client/mir_connection_api.cpp	2015-09-30 21:06:13 +0000
@@ -64,7 +64,7 @@
                 if (socket_env)
                     sock = socket_env;
                 else
-                    sock = mir::default_server_socket;
+                    sock = mir::default_server_socket();
             }
 
             auto const conf = configuration(sock);

=== modified file 'src/common/env/default_configuration.cpp'
--- src/common/env/default_configuration.cpp	2013-12-12 07:51:12 +0000
+++ src/common/env/default_configuration.cpp	2015-09-30 21:06:13 +0000
@@ -21,9 +21,7 @@
 #include <cstdlib>
 #include <sstream>
 
-namespace
-{
-const char* init()
+std::string mir::default_server_socket()
 {
     std::ostringstream formatter;
 
@@ -32,10 +30,5 @@
 
     formatter << dir << "/mir_socket";
 
-    static auto result = formatter.str();
-    return result.c_str();
-}
-}
-
-const char *const mir::default_server_socket = init();
-
+    return formatter.str();
+}

=== modified file 'src/common/symbols.map'
--- src/common/symbols.map	2015-09-17 06:06:28 +0000
+++ src/common/symbols.map	2015-09-30 21:06:13 +0000
@@ -105,7 +105,6 @@
     
     # These symbols are supposed to be "private" (they're under src/include)
     # but they are used by libmirplatform, libmirclient or libmirserver
-    mir::default_server_socket;
     mir::libraries_for_path*;
     mir::logging::input_timestamp*;
     mir::logging::log*;
@@ -199,6 +198,10 @@
       mir::dispatch::ReadableFd::dispatch*;
       mir::dispatch::ReadableFd::relevant_events*;
       mir::logger::Logger::log*;
+
+      # These symbols are supposed to be "private" (they're under src/include)
+      # but they are used by libmirplatform, libmirclient or libmirserver
+      mir::default_server_socket*;
     };
   local: *;
 } MIR_COMMON_5;

=== modified file 'src/include/common/mir/default_configuration.h'
--- src/include/common/mir/default_configuration.h	2015-02-22 07:46:25 +0000
+++ src/include/common/mir/default_configuration.h	2015-09-30 21:06:13 +0000
@@ -21,9 +21,11 @@
 #ifndef MIR_DEFAULT_CONFIGURATION_
 #define MIR_DEFAULT_CONFIGURATION_
 
+#include <string>
+
 namespace mir
 {
-extern const char *const default_server_socket;
+extern std::string default_server_socket();
 }
 
 #endif

=== modified file 'src/platform/options/default_configuration.cpp'
--- src/platform/options/default_configuration.cpp	2015-09-24 15:42:21 +0000
+++ src/platform/options/default_configuration.cpp	2015-09-30 21:06:13 +0000
@@ -139,7 +139,7 @@
     add_options()
         (host_socket_opt, po::value<std::string>(),
             "Host socket filename")
-        (server_socket_opt, po::value<std::string>()->default_value(::mir::default_server_socket),
+        (server_socket_opt, po::value<std::string>()->default_value(::mir::default_server_socket()),
             "Socket filename [string:default=$XDG_RUNTIME_DIR/mir_socket or /tmp/mir_socket]")
         (no_server_socket_opt, "Do not provide a socket filename for client connections")
         (arw_server_socket_opt, "Make socket filename globally rw (equivalent to chmod a=rw)")