Merge lp:~mterry/unity-scopes-api/snap-root into lp:unity-scopes-api

[Michael Terry]

Handle running inside a snap by paying attention to the $SNAP prefix.

Handle running inside a snap by paying attention to the $SNAP prefix.

[lp:~mterry/unity-scopes-api/snap-root]

373. By Michael Terry on 2016-10-06

Add $SNAP to Art and Icon keys too, if needed

[Marcus Tomlinson (marcustomlinson)]

Thanks Michael! Michi and I do acknowledged this MP, but we'd like to investigate the possibility of a cleaner approach whereby we reduce some of the redundancy (especially with getenv()) that's introduced here.

This is not a stab at your work by any means ;) Thanks for your help with this!

[Michi Henning (michihenning)]

Could you re-target this MR at the devel branch instead of trunk please? That allows us to use our normal staging process.

Is $SNAP always going to have a trailing slash? If not, constructs such as

$SNAP@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry

and

snap_root + base_dflt_file

won't create the path name correctly. Wouldn't it be safer to append a slash regardless? If we end up with two adjacent slashes, they won't do any harm.

This does not work:

const string snap_root = getenv("SNAP");

If SNAP isn't set, getenv() returns a nullptr, which causes the string constructor to throw. That causes a bunch of tests to fail.

The repeated calls to getenv("SNAP") are a bit of a pain (and dangerous because of the needed check for a nullptr return). It would be better to do this is ConfigBase and and provide a protected accessor for the value. That puts the code to sanitize the value in a single place.

review: Needs Fixing

[Michi Henning (michihenning)]

Here is a diff against this branch that makes some changes. Could you review please?

Thanks,

Michi.

[Michael Terry (mterry)]

> Is $SNAP always going to have a trailing slash? If not, constructs such as
> [snip[
> won't create the path name correctly. Wouldn't it be safer to append a slash
> regardless? If we end up with two adjacent slashes, they won't do any harm.

@CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash, right? So that's fine. And base_dflt_file always starts with a slash. So I think we're fine there.

> If SNAP isn't set, getenv() returns a nullptr, which causes the string
> constructor to throw. That causes a bunch of tests to fail.

Ick, right you are. I wrote so many of these in a row, I got confused between QString and std::string.

> It would be better to do this is ConfigBase and and provide a protected
> accessor for the value. That puts the code to sanitize the value in a single
> place.

OK.

> Here is a diff against this branch that makes some changes. Could you review please?

I would go make your suggested changes, but it sounds like you were trying to upload a diff to make them for me? I don't see one. I don't think LP MPs can have attachments.

[Michi Henning (michihenning)]

So sorry, got distracted by my wife with an imap issue in the middle of things.

It's a it of a let-down that I can't add an attachment to comments here.

Here's a link to the diff:

https://www.dropbox.com/s/qbbs3yeo6f5ovom/snap_root.diff?dl=0

[Michi Henning (michihenning)]

> @CMAKE_INSTALL_PREFIX@ is guaranteed to either be empty or start with a slash,
> right? So that's fine. And base_dflt_file always starts with a slash. So I
> think we're fine there.

Not necessarily. I can set it to anything when I run the configure step.

I think having the slash there is safer, and it won't do any harm if there is an extra one.

[lp:~mterry/unity-scopes-api/snap-root]

374. By Michael Terry on 2016-10-16

Apply Michi's patch to fix review nits, thanks

[Michael Terry (mterry)]

Thanks! I've applied your patch.

[Michi Henning (michihenning)]

Still need to re-target this at the devel branch, otherwise Jenkins won't test it. Could you do that please?

[lp:~mterry/unity-scopes-api/snap-root]

375. By Michael Terry on 2016-10-17

Merge devel

Unmerged revisions

=== modified file 'data/scope-registry.conf.in'
--- data/scope-registry.conf.in	2014-05-07 11:45:50 +0000
+++ data/scope-registry.conf.in	2016-10-17 08:49:38 +0000
@@ -7,4 +7,4 @@
 respawn
 respawn limit 10 60
 
-exec @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry
+exec $SNAP/@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/scoperegistry

=== modified file 'data/smart-scopes-proxy.conf.in'
--- data/smart-scopes-proxy.conf.in	2014-05-07 11:45:50 +0000
+++ data/smart-scopes-proxy.conf.in	2016-10-17 08:49:38 +0000
@@ -9,4 +9,4 @@
 
 expect stop
 
-exec @CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/smartscopesproxy upstart
+exec $SNAP/@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_LIBDIR@/@UNITY_SCOPES_LIB@/smartscopesproxy upstart

=== modified file 'doc/tutorial.dox'
--- doc/tutorial.dox	2016-06-15 09:16:47 +0000
+++ doc/tutorial.dox	2016-10-17 08:49:38 +0000
@@ -1560,7 +1560,8 @@
 
 The `IdleTimeout` key controls how long a scope can remain idle before it is told to stop by the registry (or
 killed if it does not stop within 4 seconds). The default idle timeout is 40 seconds, meaning that a
-scope will be told to stop if no query was sent to it for that amount of time.
+scope will be told to stop if no query was sent to it for that amount of time. Only values in the range 1 to
+300 seconds are accepted.
 
 `ResultTtl` determines how long results should be cached by the UI before they are considered "stale"
 and should be refreshed. `None` indicates that results remain valid indefinitely; `Small` indicates

=== modified file 'include/unity/scopes/internal/ConfigBase.h'
--- include/unity/scopes/internal/ConfigBase.h	2014-11-03 05:31:30 +0000
+++ include/unity/scopes/internal/ConfigBase.h	2016-10-17 08:49:38 +0000
@@ -59,6 +59,7 @@
     virtual std::string get_middleware(std::string const& group, std::string const& key) const;
 
 protected:
+    std::string snap_root() const;
     void throw_ex(::std::string const& reason) const;
     bool path_exists(::std::string const& path) const;
 
@@ -70,6 +71,7 @@
 private:
     unity::util::IniParser::SPtr parser_;
     std::string configfile_;
+    std::string snap_root_;
 };
 
 } // namespace internal

=== modified file 'src/scopes/internal/ConfigBase.cpp'
--- src/scopes/internal/ConfigBase.cpp	2014-08-28 00:20:56 +0000
+++ src/scopes/internal/ConfigBase.cpp	2016-10-17 08:49:38 +0000
@@ -43,10 +43,22 @@
 // If configfile is the empty string, we create a default instance that returns "Zmq" for the middleware
 // and throws for the other methods.
 
-ConfigBase::ConfigBase(string const& configfile, string const& dflt_file) :
+ConfigBase::ConfigBase(string const& configfile, string const& base_dflt_file) :
     parser_(nullptr),
     configfile_(configfile)
 {
+    char const* sroot = getenv("SNAP");
+    if (sroot)
+    {
+        snap_root_ = sroot;
+        if (!snap_root_.empty() && snap_root_[snap_root_.size() - 1] != '/')
+        {
+            snap_root_ += '/';
+        }
+    }
+
+    const string dflt_file = base_dflt_file.empty() ? "" : snap_root_ + base_dflt_file;
+
     if (!configfile.empty())
     {
         boost::filesystem::path path(configfile);
@@ -157,6 +169,11 @@
     return val;
 }
 
+string ConfigBase::snap_root() const
+{
+    return snap_root_;
+}
+
 void ConfigBase::throw_ex(string const& reason) const
 {
     string s = "\"" + configfile_ + "\": " + reason;

=== modified file 'src/scopes/internal/RegistryConfig.cpp'
--- src/scopes/internal/RegistryConfig.cpp	2016-08-31 17:54:04 +0000
+++ src/scopes/internal/RegistryConfig.cpp	2016-10-17 08:49:38 +0000
@@ -53,7 +53,7 @@
     identity_ = identity;
     mw_kind_ = get_middleware(registry_config_group, mw_kind_key);
     mw_configfile_ = get_optional_string(registry_config_group, mw_kind_ + configfile_key);
-    scope_installdir_ = get_optional_string(registry_config_group, scope_installdir_key, DFLT_SCOPE_INSTALL_DIR);
+    scope_installdir_ = get_optional_string(registry_config_group, scope_installdir_key, snap_root() + DFLT_SCOPE_INSTALL_DIR);
     oem_installdir_ = get_optional_string(registry_config_group, oem_installdir_key, DFLT_OEM_INSTALL_DIR);
     click_installdir_ = get_optional_string(registry_config_group, click_installdir_key);
     if (click_installdir_.empty())
@@ -65,7 +65,7 @@
         }
         click_installdir_ = string(home) + "/.local/share/unity-scopes/";
     }
-    scoperunner_path_ = get_optional_string(registry_config_group, scoperunner_path_key, DFLT_SCOPERUNNER_PATH);
+    scoperunner_path_ = get_optional_string(registry_config_group, scoperunner_path_key, snap_root() + DFLT_SCOPERUNNER_PATH);
     if (scoperunner_path_[0] != '/')
     {
         throw ConfigException(configfile + ": " + scoperunner_path_key + " must be an absolute path");

=== modified file 'src/scopes/internal/RuntimeConfig.cpp'
--- src/scopes/internal/RuntimeConfig.cpp	2016-02-22 01:29:01 +0000
+++ src/scopes/internal/RuntimeConfig.cpp	2016-10-17 08:49:38 +0000
@@ -63,11 +63,11 @@
     if (configfile.empty())  // Default config
     {
         registry_identity_ = DFLT_REGISTRY_ID;
-        registry_configfile_ = DFLT_REGISTRY_INI;
+        registry_configfile_ = snap_root() + DFLT_REGISTRY_INI;
         ss_registry_identity_ = DFLT_SS_REGISTRY_ID;
-        ss_configfile_ = DFLT_SS_REGISTRY_INI;
+        ss_configfile_ = snap_root() + DFLT_SS_REGISTRY_INI;
         default_middleware_ = DFLT_MIDDLEWARE;
-        default_middleware_configfile_ = DFLT_ZMQ_MIDDLEWARE_INI;
+        default_middleware_configfile_ = snap_root() + DFLT_ZMQ_MIDDLEWARE_INI;
         reap_expiry_ = DFLT_REAP_EXPIRY;
         reap_interval_ = DFLT_REAP_INTERVAL;
         cache_directory_ = default_cache_directory();
@@ -83,7 +83,7 @@
         default_middleware_ = get_middleware(runtime_config_group, default_middleware_key);
         default_middleware_configfile_ = get_optional_string(runtime_config_group,
                                                              default_middleware_ + default_middleware_configfile_key,
-                                                             DFLT_MIDDLEWARE_INI);
+                                                             snap_root() + DFLT_MIDDLEWARE_INI);
         reap_expiry_ = get_optional_int(runtime_config_group, reap_expiry_key, DFLT_REAP_EXPIRY);
         if (reap_expiry_ < 1 && reap_expiry_ != -1)
         {

=== modified file 'src/scopes/internal/ScopeConfig.cpp'
--- src/scopes/internal/ScopeConfig.cpp	2016-03-31 09:27:33 +0000
+++ src/scopes/internal/ScopeConfig.cpp	2016-10-17 08:49:38 +0000
@@ -115,6 +115,10 @@
     try
     {
         string art = parser()->get_string(scope_config_group, art_key);
+        if (!art.empty() && art[0] == '/')
+        {
+            art = snap_root() + art;
+        }
         art_.reset(new string(art));
     }
     catch (LogicException const&)
@@ -123,6 +127,10 @@
     try
     {
         string icon = parser()->get_string(scope_config_group, icon_key);
+        if (!icon.empty() && icon[0] == '/')
+        {
+            icon = snap_root() + icon;
+        }
         icon_.reset(new string(icon));
     }
     catch (LogicException const&)
@@ -173,12 +181,11 @@
 
     idle_timeout_ = get_optional_int(scope_config_group, idle_timeout_key, DFLT_SCOPE_IDLE_TIMEOUT);
 
-    // Negative values and values greater than max int (once multiplied by 1000 (s to ms)) are illegal
-    const int max_idle_timeout = std::numeric_limits<int>::max() / 1000;
-    if ((idle_timeout_ < 0 || idle_timeout_ > max_idle_timeout) && idle_timeout_ != -1)
+    // Values less than 1s and greater than 300s are illegal
+    if (idle_timeout_ < 1 || idle_timeout_ > 300)
     {
         throw_ex("Illegal value (" + std::to_string(idle_timeout_) + ") for " + idle_timeout_key +
-                 ": value must be >= 0 and <= " + std::to_string(max_idle_timeout));
+                 ": value must be >= 1 and <= 300");
     }
 
     results_ttl_type_ = ScopeMetadata::ResultsTtlType::None;

=== modified file 'test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp'
--- test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp	2015-05-14 02:50:35 +0000
+++ test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp	2016-10-17 08:49:38 +0000
@@ -156,7 +156,7 @@
     catch(ConfigException const& e)
     {
         boost::regex r("unity::scopes::ConfigException: \".*\": Illegal value \\(-2\\) for IdleTimeout: "
-                       "value must be >= 0 and <= [0-9]+");
+                       "value must be >= 1 and <= [0-9]+");
         EXPECT_TRUE(boost::regex_match(e.what(), r));
     }
 }