Merge lp:~cemil-azizoglu/mir/MesaBufferAllocatorTest-failure-under-sbuild into lp:mir

[Cemil Azizoglu]

When XDG_RUNTIME_DIR is defined but pointing to a non-existing-directory, use "/tmp" (LP: #1304873).

mir_unit_tests.MesaBufferAllocatorTest.* test fails under sbuild chroot env. (LP: #1304873)

This is due to the fact that $XDG_RUNTIME_DIR exported in the sbuild chroot env corresponding to a non-existent directory. A better solution would be to use shm with a O_TMPFILE, but due to build machines using older kernels this isn't feasible at the moment.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1968
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~cemil-azizoglu/mir/MesaBufferAllocatorTest-failure-under-sbuild/+merge/237495/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/mir-ci/1754/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2050
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2057
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1987
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/92
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/92/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/924
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/924/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2995
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14302

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

review: Needs Fixing (continuous-integration)

[Alexandros Frantzis (afrantzis)]

We could avoid the try/catch clause by using the boost::system::error_code overload instead:

if (runtime_dir)
{
    boost::system::error_code ec;
    boost::filesystem::path p(runtime_dir);
    runtime_dir_valid = boost::filesystem::is_directory(p, ec);
}

or even just stat():

struct stat sb;
bool const runtime_dir_valid = runtime_dir && !stat(runtime_dir, sb) && S_ISDIR(sb.st_mode);

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1968
http://jenkins.qa.ubuntu.com/job/mir-ci/1758/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2054
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2061
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1991
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/96
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/96/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/928
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/928/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/2998
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14331

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

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1968
http://jenkins.qa.ubuntu.com/job/mir-ci/1760/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-utopic-i386-build/2056
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-utopic-amd64-build/2063
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-utopic-touch/1993
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/98
        deb: http://jenkins.qa.ubuntu.com/job/mir-utopic-amd64-ci/98/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/930
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-utopic-armhf/930/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3000
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/14343

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

@"A better solution would be to use shm with a O_TMPFILE which is what this proposal is doing." - if only that were true. ;)

I don't like fallbacks in production code to cover situations that should only occur in tests. They tend to obscure problems in live running. (We don't want to use /tmp in a real system.)

An alternative solution be to set $XDG_RUNTIME_DIR to "/tmp" in MesaBufferAllocatorTest?

However, we already use "/tmp" if $XDG_RUNTIME_DIR isn't set, so it isn't introducing any new issue.

review: Abstain

[Chris Halse Rogers (raof)]

Can we just go back to O_TMPFILE and not fail the test when O_TMPFILE returns EISDIR (indicating that the kernel doesn't support O_TMPFILE)?

[Cemil Azizoglu (cemil-azizoglu)]

> Can we just go back to O_TMPFILE and not fail the test when O_TMPFILE returns
> EISDIR (indicating that the kernel doesn't support O_TMPFILE)?

I socialized that idea but neither duflu, nor alan_g was thrilled.

[Daniel van Vugt (vanvugt)]

Actually I like the idea of O_TMPFILE so long as it's not the only option and we have a fallback for systems that lack O_TMPFILE support, like this approach.

Minor nit: You're testing the value of runtime_dir_valid on all code paths including that for which it's guaranteed to be false. So this:
  target_dir = (runtime_dir_valid ? runtime_dir : "/tmp");
could move inside the previous if-statement with some rearrangement.

review: Approve

[Daniel van Vugt (vanvugt)]

P.S. I did approve of the previous O_TMPFILE form, for the record. Only disapproved of unrelated build-dep changes being included (now fixed). Also, Cemil found there was a CI problem with it (like some CI machine didn't have a new enough kernel for O_TMPFILE?).

[Robert Carr (robertcarr)]

This should be able to land?

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Let's get this out. We can add the O_TMPFILE code on top of it.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'src/platform/graphics/mesa/anonymous_shm_file.cpp'
--- src/platform/graphics/mesa/anonymous_shm_file.cpp	2014-10-06 03:02:44 +0000
+++ src/platform/graphics/mesa/anonymous_shm_file.cpp	2014-10-08 15:34:57 +0000
@@ -20,6 +20,7 @@
 #include "anonymous_shm_file.h"
 
 #include <boost/throw_exception.hpp>
+#include <boost/filesystem.hpp>
 #include <stdexcept>
 
 #include <vector>
@@ -38,7 +39,16 @@
 {
     char const* const tmpl = "/mir-buffer-XXXXXX";
     char const* const runtime_dir = getenv("XDG_RUNTIME_DIR");
-    char const* const target_dir = runtime_dir ? runtime_dir : "/tmp";
+    bool runtime_dir_valid = false;
+
+    if (runtime_dir)
+    {
+        boost::system::error_code ec;
+        boost::filesystem::path p(runtime_dir);
+        runtime_dir_valid = boost::filesystem::is_directory(p, ec);
+    }
+
+    char const* const target_dir = (runtime_dir_valid ? runtime_dir : "/tmp");
 
     /* We need a mutable array for mkostemp */
     std::vector<char> path(target_dir, target_dir + strlen(target_dir));

=== modified file 'tests/unit-tests/graphics/mesa/test_anonymous_shm_file.cpp'
--- tests/unit-tests/graphics/mesa/test_anonymous_shm_file.cpp	2014-10-01 03:41:21 +0000
+++ tests/unit-tests/graphics/mesa/test_anonymous_shm_file.cpp	2014-10-08 15:34:57 +0000
@@ -183,7 +183,7 @@
 TEST(AnonymousShmFile, is_created_and_deleted_in_xdg_runtime_dir)
 {
     using namespace testing;
-    
+
     TemporaryDirectory const temp_dir;
     TemporaryEnvironmentValue const env{"XDG_RUNTIME_DIR", temp_dir.path()};
     PathWatcher const path_watcher{temp_dir.path()};
@@ -201,7 +201,7 @@
 TEST(AnonymousShmFile, is_created_and_deleted_in_tmp_dir)
 {
     using namespace testing;
-    
+
     TemporaryEnvironmentValue const env{"XDG_RUNTIME_DIR", nullptr};
     PathWatcher const path_watcher{"/tmp"};
     size_t const file_size{100};
@@ -215,10 +215,27 @@
     path_watcher.process_events();
 }
 
+TEST(AnonymousShmFile, is_created_and_deleted_in_tmp_dir_with_nonexistent_xdg_runtime_dir)
+{
+    using namespace testing;
+
+    TemporaryEnvironmentValue const env{"XDG_RUNTIME_DIR", "/non-existent-dir"};
+    PathWatcher const path_watcher{"/tmp"};
+    size_t const file_size{100};
+
+    InSequence s;
+    EXPECT_CALL(path_watcher, file_created(StartsWith("mir-buffer-")));
+    EXPECT_CALL(path_watcher, file_deleted(StartsWith("mir-buffer-")));
+
+    mgm::AnonymousShmFile shm_file{file_size};
+
+    path_watcher.process_events();
+}
+
 TEST(AnonymousShmFile, has_correct_size)
 {
     using namespace testing;
-    
+
     TemporaryDirectory const temp_dir;
     TemporaryEnvironmentValue const env{"XDG_RUNTIME_DIR", temp_dir.path()};
     size_t const file_size{100};
@@ -234,7 +251,7 @@
 TEST(AnonymousShmFile, writing_to_base_ptr_writes_to_file)
 {
     using namespace testing;
-    
+
     TemporaryDirectory const temp_dir;
     TemporaryEnvironmentValue const env{"XDG_RUNTIME_DIR", temp_dir.path()};
     size_t const file_size{100};