Merge lp:unity-scopes-api/staging into lp:unity-scopes-api

[Paweł Stołowski]

Merged devel.

Merged devel.

[Marcus Tomlinson (marcustomlinson)]

ACK

review: Approve

=== modified file 'include/unity/scopes/internal/Utils.h'
--- include/unity/scopes/internal/Utils.h	2015-04-10 03:55:25 +0000
+++ include/unity/scopes/internal/Utils.h	2015-09-21 09:13:55 +0000
@@ -22,6 +22,8 @@
 #include <string>
 #include <sstream>
 
+#include <boost/filesystem.hpp>
+
 namespace unity
 {
 
@@ -57,6 +59,9 @@
 
 void make_directories(std::string const& path_name, mode_t mode);
 
+std::vector<std::string> split_exec_args(std::string const& id, std::string const& custom_exec);
+std::string convert_exec_rel_to_abs(std::string const& id, boost::filesystem::path const& scope_dir, std::string const& custom_exec);
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'scoperegistry/scoperegistry.cpp'
--- scoperegistry/scoperegistry.cpp	2015-07-01 11:02:46 +0000
+++ scoperegistry/scoperegistry.cpp	2015-09-21 09:13:55 +0000
@@ -259,30 +259,6 @@
     }
 }
 
-string get_first_component(string const& id, string const& custom_exec)
-{
-    if (custom_exec.empty())
-    {
-        throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
-    }
-
-    wordexp_t exp;
-    string result;
-
-    // Split command into program and args
-    if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
-    {
-        util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
-        result = exp.we_wordv[0];
-    }
-    else
-    {
-        throw unity::InvalidArgumentException("Invalid scope runner executable for scope: " + id);
-    }
-
-    return result;
-}
-
 // For each scope, open the config file for the scope, create the metadata info from the config,
 // and add an entry to the RegistryObject.
 // If the scope uses settings, also parse the settings file and add the settings to the metadata.
@@ -435,33 +411,7 @@
 
     try
     {
-        auto custom_exec = sc.scope_runner();
-        filesystem::path program(get_first_component( scope.first, custom_exec));
-        if (program.is_relative())
-        {
-            // First look inside the arch-specific directory
-            if (filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / program))
-            {
-                // Join the full command, not just the program path
-                exec_data.custom_exec = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
-            }
-            // Next try in the non arch-aware directory
-            else if (filesystem::exists(scope_dir / program))
-            {
-                // Join the full command, not just the program path
-                exec_data.custom_exec = (scope_dir / custom_exec).native();
-            }
-            else
-            {
-                throw unity::InvalidArgumentException(
-                        "Nonexistent scope runner '" + custom_exec
-                                + "' executable for scope: " + scope.first);
-            }
-        }
-        else
-        {
-            exec_data.custom_exec = custom_exec;
-        }
+        exec_data.custom_exec = convert_exec_rel_to_abs(scope.first, scope_dir, sc.scope_runner());
     }
     catch (NotFoundException const&)
     {

=== modified file 'src/scopes/internal/JsonCppNode.cpp'
--- src/scopes/internal/JsonCppNode.cpp	2015-07-13 08:44:43 +0000
+++ src/scopes/internal/JsonCppNode.cpp	2015-09-21 09:13:55 +0000
@@ -279,6 +279,11 @@
 
 JsonNodeInterface::SPtr JsonCppNode::get_node(std::string const& node_name) const
 {
+    if (!root_)
+    {
+        throw unity::LogicException("Current node is empty");
+    }
+
     if (!root_.isMember(node_name))
     {
         throw unity::LogicException("Node " + node_name + " does not exist");
@@ -290,6 +295,11 @@
 
 JsonNodeInterface::SPtr JsonCppNode::get_node(unsigned int node_index) const
 {
+    if (!root_)
+    {
+        throw unity::LogicException("Current node is empty");
+    }
+
     if (root_.type() != Json::arrayValue)
     {
         throw unity::LogicException("Root node is not an array");

=== modified file 'src/scopes/internal/Utils.cpp'
--- src/scopes/internal/Utils.cpp	2015-04-10 03:55:25 +0000
+++ src/scopes/internal/Utils.cpp	2015-09-21 09:13:55 +0000
@@ -19,12 +19,14 @@
 #include <unity/scopes/internal/Utils.h>
 #include <unity/scopes/ScopeExceptions.h>
 #include <unity/UnityExceptions.h>
+#include <unity/util/ResourcePtr.h>
 
 #include <boost/filesystem.hpp>
 
 #include <iomanip>
 #include <locale>
 #include <mutex>
+#include <wordexp.h>
 
 #include <sys/stat.h>
 
@@ -190,6 +192,87 @@
     }
 }
 
+vector<string> split_exec_args(string const& id, string const& custom_exec)
+{
+    if (custom_exec.empty())
+    {
+        throw unity::InvalidArgumentException("Invalid empty executable for scope: '" + id + "'");
+    }
+
+    wordexp_t exp;
+    vector<string> result;
+
+    // Split command into program and args
+    if (wordexp(custom_exec.c_str(), &exp, WRDE_NOCMD) == 0)
+    {
+        util::ResourcePtr<wordexp_t*, decltype(&wordfree)> free_guard(&exp, wordfree);
+        for (size_t i = 0; i < exp.we_wordc; ++i )
+        {
+            auto argument = string(exp.we_wordv[i]);
+            if(argument.find_first_of(' ') != std::string::npos)
+            {
+                // This argument contains spaces, enclose it in quotation marks
+                result.emplace_back("\"" + argument + "\"");
+            }
+            else
+            {
+                result.emplace_back(argument);
+            }
+        }
+    }
+    else
+    {
+        throw unity::InvalidArgumentException("Invalid executable for scope: '" + id + "'");
+    }
+
+    return result;
+}
+
+string convert_exec_rel_to_abs(string const& id, boost::filesystem::path const& scope_dir, string const& custom_exec)
+{
+    string result;
+
+    // Loop through each argument of the scope runner command and ensure all path args are absolute
+    auto custom_exec_args = split_exec_args(id, custom_exec);
+    for (auto custom_exec_arg : custom_exec_args)
+    {
+        boost::filesystem::path argument(custom_exec_arg);
+        if (argument.is_relative())
+        {
+            // First look inside the arch-specific directory
+            if (boost::filesystem::exists(scope_dir / DEB_HOST_MULTIARCH / argument))
+            {
+                // Append the argument as a relative path
+                result += (scope_dir / DEB_HOST_MULTIARCH / argument).native() + " ";
+            }
+            // Next try in the non arch-aware directory
+            else if (boost::filesystem::exists(scope_dir / argument))
+            {
+                // Append the argument as a relative path
+                result += (scope_dir / argument).native() + " ";
+            }
+            // If this is the first argument (program name) it must exist, throw here
+            else if (result.empty())
+            {
+                throw unity::InvalidArgumentException(
+                        "Nonexistent scope runner executable: '" + custom_exec_arg
+                                + "' for scope: '" + id + "'");
+            }
+            // Otherwise just append the argument as is
+            else
+            {
+                result += custom_exec_arg + " ";
+            }
+        }
+        else
+        {
+            result += custom_exec_arg + " ";
+        }
+    }
+    result.resize(result.size() - 1);
+    return result;
+}
+
 } // namespace internal
 
 } // namespace scopes

=== modified file 'src/scopes/internal/zmq_middleware/ZmqObject.cpp'
--- src/scopes/internal/zmq_middleware/ZmqObject.cpp	2015-08-04 04:56:01 +0000
+++ src/scopes/internal/zmq_middleware/ZmqObject.cpp	2015-09-21 09:13:55 +0000
@@ -250,16 +250,13 @@
 
 ZmqObjectProxy::TwowayOutParams ZmqObjectProxy::invoke_twoway__(capnp::MessageBuilder& request, int64_t timeout)
 {
-    RequestMode mode;
     std::string endpoint;
     {
         lock_guard<mutex> lock(shared_mutex);
-        mode = mode_;
         endpoint = endpoint_;
+        assert(mode_ == RequestMode::Twoway);
     }
 
-    assert(mode == RequestMode::Twoway);
-
     zmqpp::socket s(*mw_base()->context(), zmqpp::socket_type::request);
     // Allow some linger time so we don't hang indefinitely if the other end disappears.
     s.set(zmqpp::socket_option::linger, 100);

=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp'
--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp	2014-12-05 03:05:37 +0000
+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.cpp	2015-09-21 09:13:55 +0000
@@ -25,6 +25,9 @@
 #include <unity/scopes/Runtime.h>
 #include <unity/scopes/PreviewReply.h>
 
+#include <boost/filesystem.hpp>
+
+using namespace boost;
 using namespace std;
 using namespace unity::scopes;
 
@@ -102,10 +105,11 @@
     std::string scope_config = argc > 2 ? argv[2] : "FAIL";
     std::string rt_config = argc > 3 ? argv[3] : "FAIL";
     std::string unused_arg_2 = argc > 4 ? argv[4] : "FAIL";
+    filesystem::path abs_path_arg(argc > 5 ? argv[5] : "FAIL");
 
     // In order to test that custom exec splitting works, we check here that our arbitrary arguments
     // have been delivered to us as expected.
-    if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2")
+    if (unused_arg_1 == "unused arg 1" && unused_arg_2 == "unused arg 2" && filesystem::exists(abs_path_arg))
     {
         MyScope scope;
         auto runtime = Runtime::create_scope_runtime("testscopeB", rt_config);

=== modified file 'test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in'
--- test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in	2014-12-08 15:17:04 +0000
+++ test/gtest/scopes/Registry/scopes/testscopeB/testscopeB.ini.in	2015-09-21 09:13:55 +0000
@@ -6,5 +6,5 @@
 Icon = data/scope-B.Icon
 SearchHint = scope-B.SearchHint
 HotKey = scope-B.HotKey
-ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2
+ScopeRunner = testscopeB "unused arg 1" %S %R unused' 'arg' '2 ./testscopeB.ini
 IdleTimeout = 2

=== modified file 'test/gtest/scopes/internal/SettingsDB/SettingsDB_test.cpp'
--- test/gtest/scopes/internal/SettingsDB/SettingsDB_test.cpp	2015-07-13 14:43:24 +0000
+++ test/gtest/scopes/internal/SettingsDB/SettingsDB_test.cpp	2015-09-21 09:13:55 +0000
@@ -44,7 +44,7 @@
     int fd = ::open(db_name.c_str(), O_WRONLY|O_CREAT, S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP);
     if (fd == -1)
     {
-        FAIL() << "Couldn't open file " << db_name << " " << errno;
+        FAIL() << "Couldn't open file " << db_name << ", errno: " << errno;
     }
 
     struct flock fl;
@@ -56,7 +56,7 @@
 
     if (::fcntl(fd, F_SETLKW, &fl) != 0)
     {
-        FAIL() << "Couldn't get write lock for " << db_name << " " << errno;
+        FAIL() << "Couldn't get write lock for " << db_name << ", errno: " << errno;
     }
 
     string pth = string(TEST_SRC_DIR) + "/" + src;
@@ -71,7 +71,11 @@
     char buf[1024];
     while ((n = ::read(fd2, buf, 1024)) > 0)
     {
-        ::write(fd, buf, n);
+        int rc = ::write(fd, buf, n);
+        if (rc != n)
+        {
+            FAIL() << "Write returned " << rc << ", expected " << n << " (" << db_name << ", errno: " << errno << ")";
+        }
     }
     ::close(fd2);
     ::close(fd);

=== modified file 'test/gtest/scopes/internal/Utils/Utils_test.cpp'
--- test/gtest/scopes/internal/Utils/Utils_test.cpp	2015-04-10 03:55:25 +0000
+++ test/gtest/scopes/internal/Utils/Utils_test.cpp	2015-09-21 09:13:55 +0000
@@ -132,3 +132,89 @@
 
     chmod(child.c_str(), 0777);  // Don't leave the dir behind without permissions.
 }
+
+TEST(Utils, split_exec_args)
+{
+    // Test empty executable
+    try
+    {
+        split_exec_args("test", "");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
+    }
+
+    // Test invalid executable
+    try
+    {
+        split_exec_args("test", "\"");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
+    }
+
+    // Test argument splitting
+    auto exec_args = split_exec_args("test", "/path\\ to/exec' 'file arg \"arg 2\" arg' '3 arg\\ 4");
+    ASSERT_EQ(5, exec_args.size());
+    EXPECT_STREQ("\"/path to/exec file\"", exec_args[0].c_str());
+    EXPECT_STREQ("arg", exec_args[1].c_str());
+    EXPECT_STREQ("\"arg 2\"", exec_args[2].c_str());
+    EXPECT_STREQ("\"arg 3\"", exec_args[3].c_str());
+    EXPECT_STREQ("\"arg 4\"", exec_args[4].c_str());
+}
+
+TEST(Utils, convert_exec_rel_to_abs)
+{
+    // Test empty executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid empty executable for scope: 'test'", e.what());
+    }
+
+    // Test invalid executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "\"");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Invalid executable for scope: 'test'", e.what());
+    }
+
+    // Test nonexistent executable
+    try
+    {
+        convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "noexec");
+        FAIL();
+    }
+    catch (std::exception const& e)
+    {
+        EXPECT_STREQ("unity::InvalidArgumentException: Nonexistent scope runner executable: 'noexec' for scope: 'test'", e.what());
+    }
+
+    // Test absolute executable
+    auto abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash");
+    EXPECT_STREQ("/bin/bash", abs_exec.c_str());
+
+    // Test absolute executable w/ args
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "/bin/bash arg \"arg 2\" arg' '3 arg\\ 4");
+    EXPECT_STREQ("/bin/bash arg \"arg 2\" \"arg 3\" \"arg 4\"", abs_exec.c_str());
+
+    // Test relative executable (w/ ./)
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "./Utils_test");
+    EXPECT_TRUE(boost::filesystem::exists(abs_exec));
+
+    // Test relative executable (w/o ./)
+    abs_exec = convert_exec_rel_to_abs("test", boost::filesystem::path(TEST_DIR), "Utils_test");
+    EXPECT_TRUE(boost::filesystem::exists(abs_exec));
+}

=== modified file 'test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp'
--- test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp	2015-07-22 03:54:35 +0000
+++ test/gtest/scopes/internal/zmq_middleware/RegistryI/RegistryI_test.cpp	2015-09-21 09:13:55 +0000
@@ -28,6 +28,7 @@
 #include <unity/scopes/internal/ScopeMetadataImpl.h>
 #include <unity/scopes/internal/ScopeImpl.h>
 #include <unity/scopes/internal/UniqueID.h>
+#include <unity/scopes/internal/Utils.h>
 #include <unity/scopes/internal/zmq_middleware/ZmqRegistry.h>
 #include <unity/scopes/SearchMetadata.h>
 #include <unity/scopes/ScopeExceptions.h>
@@ -465,42 +466,6 @@
         return count_child_procs() - start_process_count;
     }
 
-    string convert_custom_exec(fs::path const& scope_dir, string const& custom_exec)
-    {
-        string result;
-
-        vector<string> split;
-        boost::split(split, custom_exec, boost::is_space());
-        fs::path program(split.front());
-        if (program.is_relative())
-        {
-            // First look inside the arch-specific directory
-            if (fs::exists(scope_dir / DEB_HOST_MULTIARCH / program))
-            {
-                // Join the full command, not just the program path
-                result = (scope_dir / DEB_HOST_MULTIARCH / custom_exec).native();
-            }
-            // Next try in the non arch-aware directory
-            else if (fs::exists(scope_dir / program))
-            {
-                // Join the full command, not just the program path
-                result = (scope_dir / custom_exec).native();
-            }
-            else
-            {
-                throw unity::InvalidArgumentException(
-                        "Nonexistent scope runner '" + custom_exec
-                                + "' executable for scope");
-            }
-        }
-        else
-        {
-            result = custom_exec;
-        }
-
-        return result;
-    }
-
     ScopeProxy start_testscopeB()
     {
         std::string test_scope_id = "testscopeB";
@@ -519,7 +484,7 @@
 
         RegistryObject::ScopeExecData exec_data;
         exec_data.scope_id = test_scope_id;
-        exec_data.custom_exec = convert_custom_exec(REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
+        exec_data.custom_exec = convert_exec_rel_to_abs(test_scope_id, REGISTRY_TEST_DIR "/scopes/testscopeB", sc.scope_runner());
         exec_data.runtime_config = runtime_ini;
         exec_data.scope_config = test_scope_config;
         exec_data.timeout_ms = process_timeout;

=== modified file 'unity-scopes.map'
--- unity-scopes.map	2015-04-10 03:55:25 +0000
+++ unity-scopes.map	2015-09-21 09:13:55 +0000
@@ -13,6 +13,7 @@
         virtual?thunk?to?unity::scopes::[!iu]*;
         vtable?for?unity::scopes::utility::[!i]*;
         vtable?for?unity::scopes::[!iu]*;
+        unity::scopes::internal::convert_exec_rel_to_abs*;
         unity::scopes::internal::Executor::*;
         unity::scopes::internal::IniSettingsSchema::*;
         unity::scopes::internal::JsonSettingsSchema::*;