Extensions: Improve logic to lookup script extensions

- Use identical logic for looking up <dependency>s and <command>s.
- Remove duplicate (but inconsistent and incomplete) logic from
  script.cpp that was used to search for the <command> again
- Remove <check> element from .inx format
  It seems unused (at the very least by core extensions) and
  redundant to <dependency> checking
- Deprecate the <command>-specific "reldir" attribute.
  Consistently use the functionally identical "location" attribute
  that was only used for <dependency>s before
- Introduce the new relative location value location="inx",
  which looks up <dependencies> and <command>s relative to the
  .inx file's location.

7 files changed, 148 insertions, 188 deletions

diff --git a/src/extension/dependency.cpp b/src/extension/dependency.cpp
index 0f45d02b5..1ea134c36 100644
--- a/src/extension/dependency.cpp
+++ b/src/extension/dependency.cpp
@@ -20,37 +20,37 @@
 namespace Inkscape {
 namespace Extension {
 
-// These strings are for XML attribute comparisons and should not be translated
-gchar const * Dependency::_type_str[] = {
+// These strings are for XML attribute comparisons and should not be translated;
+// make sure to keep in sync with enum defined in dependency.h
+gchar const * Dependency::_type_str[TYPE_CNT] = {
     "executable",
     "file",
     "extension",
 };
 
 // These strings are for XML attribute comparisons and should not be translated
-gchar const * Dependency::_location_str[] = {
+// make sure to keep in sync with enum defined in dependency.h
+gchar const * Dependency::_location_str[LOCATION_CNT] = {
     "path",
     "extensions",
+    "inx",
     "absolute",
 };
 
 /**
     \brief   Create a dependency using an XML definition
-    \param   in_repr   XML definition of the dependency
+    \param   in_repr     XML definition of the dependency
+    \param   extension   Reference to the extension requesting this dependency
 
     This function mostly looks for the 'location' and 'type' attributes
     and turns them into the enums of the same name.  This makes things
     a little bit easier to use later.  Also, a pointer to the core
     content is pulled out -- also to make things easier.
 */
-Dependency::Dependency (Inkscape::XML::Node * in_repr)
+Dependency::Dependency (Inkscape::XML::Node * in_repr, const Extension *extension)
+    : _repr(in_repr)
+    , _extension(extension)
 {
-    _type = TYPE_FILE;
-    _location = LOCATION_PATH;
-    _repr = in_repr;
-    _string = nullptr;
-    _description = nullptr;
-
     Inkscape::GC::anchor(_repr);
 
     if (const gchar * location = _repr->attribute("location")) {
@@ -60,7 +60,7 @@ Dependency::Dependency (Inkscape::XML::Node * in_repr)
                 break;
             }
         }
-    } else if (const gchar * location = _repr->attribute("reldir")) {
+    } else if (const gchar * location = _repr->attribute("reldir")) { // backwards-compatibility
         for (int i = 0; i < LOCATION_CNT && location != nullptr; i++) {
             if (!strcmp(location, _location_str[i])) {
                 _location = (location_t)i;
@@ -68,7 +68,7 @@ Dependency::Dependency (Inkscape::XML::Node * in_repr)
             }
         }
     }
-    
+
     const gchar * type = _repr->attribute("type");
     for (int i = 0; i < TYPE_CNT && type != nullptr; i++) {
         if (!strcmp(type, _type_str[i])) {
@@ -126,17 +126,19 @@ Dependency::~Dependency ()
     found then a TRUE is returned.  If we get all the way through the
     path then a FALSE is returned, the command could not be found.
 */
-bool Dependency::check () const
+bool Dependency::check ()
 {
-    // std::cout << "Checking: " << *this << std::endl;
+    if (_string == nullptr) {
+        return false;
+    }
 
-    if (_string == nullptr) return FALSE;
+    _absolute_location = "";
 
     switch (_type) {
         case TYPE_EXTENSION: {
             Extension * myext = db.get(_string);
-            if (myext == nullptr) return FALSE;
-            if (myext->deactivated()) return FALSE;
+            if (myext == nullptr) return false;
+            if (myext->deactivated()) return false;
             break;
         }
         case TYPE_EXECUTABLE:
@@ -146,21 +148,36 @@ bool Dependency::check () const
                 filetest |= Glib::FILE_TEST_IS_EXECUTABLE;
             }
 
-            Glib::ustring location(_string);
+            std::string location(_string);
             switch (_location) {
                 case LOCATION_EXTENSIONS: {
-                    using namespace Inkscape::IO::Resource;
-                    Glib::ustring temploc = get_filename(EXTENSIONS, location.c_str());
-                    if(Glib::file_test(temploc, filetest)) {
+                    std::string temploc =
+                        Inkscape::IO::Resource::get_filename(Inkscape::IO::Resource::EXTENSIONS, location.c_str());
+                    if (Glib::file_test(temploc, filetest)) {
                         location = temploc;
+                        _absolute_location = temploc;
                         break;
                     }
-                } /* PASS THROUGH!!! */
+                } /* PASS THROUGH!!! */ // TODO: the pass-through seems wrong - either it's relative or not.
                 case LOCATION_ABSOLUTE: {
+                    // TODO: should we check if the directory actually is absolute and/or sanitize the filename somehow?
                     if (!Glib::file_test(location, filetest)) {
-                        // std::cout << "Failing on location: " << location << std::endl;
-                        return FALSE;
+                        return false;
                     }
+                    _absolute_location = location;
+                    break;
+                }
+                case LOCATION_INX: {
+                    std::string base_directory = _extension->get_base_directory();
+                    if (base_directory.empty()) {
+                        g_warning("Dependency '%s' requests location relative to .inx file, "
+                                  "which is unknown for extension '%s'", _string, _extension->get_id());
+                    }
+                    std::string absolute_location = Glib::build_filename(base_directory, location);
+                    if (!Glib::file_test(absolute_location, filetest)) {
+                        return false;
+                    }
+                    _absolute_location = absolute_location;
                     break;
                 }
                 /* The default case is to look in the path */
@@ -178,7 +195,7 @@ bool Dependency::check () const
 
                     for (; path != nullptr;) {
                         gchar * local_path; // to have the path after detection of the separator
-                        Glib::ustring final_name;
+                        std::string final_name;
 
                         local_path = path;
                         path = g_utf8_strchr(path, -1, G_SEARCHPATH_SEPARATOR);
@@ -198,35 +215,38 @@ bool Dependency::check () const
 
                         if (Glib::file_test(final_name, filetest)) {
                             g_free(orig_path);
-                            return TRUE;
+                            _absolute_location = final_name;
+                            return true;
                         }
 
                         // give it a 2nd try with ".exe" added
                         Glib::ustring final_name_exe = final_name + ".exe";
                         if (Glib::file_test(final_name_exe, filetest)) {
                             g_free(orig_path);
-                            return TRUE;
+                            _absolute_location = final_name;
+                            return true;
                         }
 
                         // and a 3rd try with ".cmd" added (mainly for UniConvertor)
                         Glib::ustring final_name_cmd = final_name + ".cmd";
                         if (Glib::file_test(final_name_cmd, filetest)) {
                             g_free(orig_path);
-                            return TRUE;
+                            _absolute_location = final_name;
+                            return true;
                         }
                     }
 
                     g_free(orig_path);
-                    return FALSE; /* Reverse logic in this one */
+                    return false; /* Reverse logic in this one */
                 }
             } /* switch _location */
             break;
         } /* TYPE_FILE, TYPE_EXECUTABLE */
         default:
-            return FALSE;
+            return false;
     } /* switch _type */
 
-    return TRUE;
+    return true;
 }
 
 /**
@@ -234,7 +254,7 @@ bool Dependency::check () const
     \return  A string containing the name of the dependency.
 
     Returns the name of the dependency as found in the configuration file.
- 
+
 */
 const gchar* Dependency::get_name()
 {
@@ -242,6 +262,28 @@ const gchar* Dependency::get_name()
 }
 
 /**
+    \brief   Path of this dependency
+    \return  Absolute path to the dependency file
+             (or an empty string if dependency was not found or is of TYPE_EXTENSION)
+
+    Returns the verified absolute path of the dependency file.
+    This value is only available after checking the Dependency by calling Dependency::check().
+*/
+std::string Dependency::get_path()
+{
+    if (_type == TYPE_EXTENSION) {
+        g_warning("Requested absolute path of dependency '%s' which is of 'extension' type.", _string);
+        return "";
+    }
+    if (_absolute_location == UNCHECKED) {
+        g_warning("Requested absolute path of dependency '%s' which is unchecked.", _string);
+        return "";
+    }
+
+    return _absolute_location;
+}
+
+/**
     \brief   Print out a dependency to a string.
 */
 Glib::ustring Dependency::info_string()
diff --git a/src/extension/dependency.h b/src/extension/dependency.h
index 5ae685e07..1b4d5b819 100644
--- a/src/extension/dependency.h
+++ b/src/extension/dependency.h
@@ -17,16 +17,22 @@
 namespace Inkscape {
 namespace Extension {
 
+class Extension;
+
 /** \brief  A class to represent a dependency for an extension.  There
             are different things that can be done in a dependency, and
             this class takes care of all of them. */
 class Dependency {
+    static constexpr const char *UNCHECKED = "---unchecked---";
+    
     /** \brief  The XML representation of the dependency. */
     Inkscape::XML::Node * _repr;
     /** \brief  The string that is in the XML tags pulled out. */
-    const gchar * _string;
+    const gchar * _string = nullptr;
     /** \brief  The description of the dependency for the users. */
-    const gchar * _description;
+    const gchar * _description = nullptr;
+    /** \brief  The absolute path to the dependency file determined while checking this dependency. */
+    std::string  _absolute_location = UNCHECKED;
 
     /** \brief  All the possible types of dependencies. */
     enum type_t {
@@ -36,31 +42,37 @@ class Dependency {
         TYPE_CNT         /**< Number of types */
     };
     /** \brief  Storing the type of this particular dependency. */
-    type_t _type;
+    type_t _type = TYPE_FILE;
 
     /** \brief  All of the possible locations to look for the dependency. */
     enum location_t {
-        LOCATION_PATH,       /**< Look in the PATH for this dependency */
-        LOCATION_EXTENSIONS, /**< Look in the extensions directory */
+        LOCATION_PATH,       /**< Look in the PATH for this dependency  - historically this is the default
+                                  (it's a bit odd for interpreted script files but makes sense for other executables) */
+        LOCATION_EXTENSIONS, /**< Look in the extensions directory
+                                  (note: this can be in both, user and system locations!) */
+        LOCATION_INX,        /**< Look relative to the inx file's location */
         LOCATION_ABSOLUTE,   /**< This dependency is already defined in absolute terms */
         LOCATION_CNT         /**< Number of locations to look */
     };
     /** \brief  The location to look for this particular dependency. */
-    location_t _location;
+    location_t _location = LOCATION_PATH;
 
     /** \brief  Strings to represent the different enum values in
                 \c type_t in the XML */
-    static gchar const * _type_str[TYPE_CNT]; 
+    static gchar const * _type_str[TYPE_CNT];
     /** \brief  Strings to represent the different enum values in
                 \c location_t in the XML */
-    static gchar const * _location_str[LOCATION_CNT]; 
+    static gchar const * _location_str[LOCATION_CNT];
+
+    /** \brief  Reference to the extension requesting this dependency. */
+    const Extension *_extension;
 
 public:
-    Dependency  (Inkscape::XML::Node * in_repr);
+    Dependency (Inkscape::XML::Node *in_repr, const Extension *extension);
     virtual ~Dependency ();
-    bool check  () const;
+    bool check();
     const gchar* get_name();
-    Glib::ustring &get_link () const;
+    std::string get_path();
 
     Glib::ustring info_string();
 }; /* class Dependency */
diff --git a/src/extension/extension.cpp b/src/extension/extension.cpp
index 6ea9337cb..771a2105e 100644
--- a/src/extension/extension.cpp
+++ b/src/extension/extension.cpp
@@ -53,7 +53,7 @@ FILE *Extension::error_file = nullptr;
     \return  none
     \brief   Constructs an Extension from a Inkscape::XML::Node
     \param   in_repr        The repr that should be used to build it
-    \param   base_directory Base directory of extension that were loaded from a file (.inx file's location)
+    \param   base_directory Base directory of extensions that were loaded from a file (.inx file's location)
 
     This function is the basis of building an extension for Inkscape.  It
     currently extracts the fields from the Repr that are used in the
@@ -121,11 +121,11 @@ Extension::Extension(Inkscape::XML::Node *in_repr, Implementation::Implementatio
                 _widgets.push_back(widget);
             }
         } else if (!strcmp(chname, "dependency")) {
-            _deps.push_back(new Dependency(child_repr));
+            _deps.push_back(new Dependency(child_repr, this));
         } else if (!strcmp(chname, "script")) { // check command as a dependency (see LP #505920)
             for (Inkscape::XML::Node *child = child_repr->firstChild(); child != nullptr; child = child->next()) {
                 if (child->type() == Inkscape::XML::ELEMENT_NODE) { // skip non-element nodes (see LP #1372200)
-                    _deps.push_back(new Dependency(child));
+                    _deps.push_back(new Dependency(child, this));
                     break;
                 }
             }
@@ -309,7 +309,7 @@ Extension::check ()
     if (retval) {
         return imp->check(this);
     }
-    
+
     error_file_write("");
     return retval;
 }
@@ -342,7 +342,7 @@ Extension::get_repr ()
     \brief   Get the ID of this extension - not a copy don't delete!
 */
 gchar *
-Extension::get_id ()
+Extension::get_id () const
 {
     return _id;
 }
@@ -352,7 +352,7 @@ Extension::get_id ()
     \brief   Get the name of this extension - not a copy don't delete!
 */
 gchar *
-Extension::get_name ()
+Extension::get_name () const
 {
     return _name;
 }
@@ -395,6 +395,30 @@ Extension::deactivated ()
     return get_state() == STATE_DEACTIVATED;
 }
 
+/** Gets the location of the dependency file as an absolute path
+  *
+  * Iterates over all dependencies of this extension and finds the one with matching name,
+  * then returns the absolute path to this dependency file as determined previously.
+  *
+  * TODO: This function should not be necessary, but we parse script dependencies twice:
+  *       - Once here in the Extension::Extension() constructor
+  *       - A second time in Script::load() in "script.cpp" when determining the script location
+  *       Theoretically we could return the wrong path if an extension depends on two files with the same name
+  *       in different relative locations. In practice this risk should be close to zero, though.
+  *
+  * @return Absolute path of the dependency file
+  */
+std::string Extension::get_dependency_location(const char *name)
+{
+    for (auto dep : _deps) {
+        if (!strcmp(name, dep->get_name())) {
+            return dep->get_path();
+        }
+    }
+
+    return "";
+}
+
 /** Gets a translation within the context of the current extension
   *
   * Query gettext for the translated version of the input string,
@@ -690,7 +714,7 @@ void
 Extension::error_file_open ()
 {
     gchar *ext_error_file = Inkscape::IO::Resource::log_path(EXTENSION_ERROR_LOG_FILENAME);
-    error_file = Inkscape::IO::fopen_utf8name(ext_error_file, "w+");		
+    error_file = Inkscape::IO::fopen_utf8name(ext_error_file, "w+");
     if (!error_file) {
         g_warning(_("Could not create extension error log file '%s'"), ext_error_file);
     }
diff --git a/src/extension/extension.h b/src/extension/extension.h
index a2f6c19db..f986040b7 100644
--- a/src/extension/extension.h
+++ b/src/extension/extension.h
@@ -137,16 +137,17 @@ public:
     bool          loaded       ();
     virtual bool  check        ();
     Inkscape::XML::Node *      get_repr     ();
-    gchar *       get_id       ();
-    gchar *       get_name     ();
+    gchar *       get_id       () const;
+    gchar *       get_name     () const;
     void          deactivate   ();
     bool          deactivated  ();
     void          printFailure (Glib::ustring reason);
     Implementation::Implementation * get_imp () { return imp; };
     void          set_execution_env (ExecutionEnv * env) { execution_env = env; };
     ExecutionEnv *get_execution_env () { return execution_env; };
-    std::string   get_base_directory() { return _base_directory; };
+    std::string   get_base_directory() const { return _base_directory; };
     void          set_base_directory(std::string base_directory) { _base_directory = base_directory; };
+    std::string   get_dependency_location(const char *name);
     const char   *get_translation(const char* msgid, const char *msgctxt=nullptr);
 
 /* Parameter Stuff */
diff --git a/src/extension/implementation/script.cpp b/src/extension/implementation/script.cpp
index 1d90d1266..c38cc516f 100644
--- a/src/extension/implementation/script.cpp
+++ b/src/extension/implementation/script.cpp
@@ -155,104 +155,6 @@ Script::~Script()
 = default;
 
 
-
-/**
-    \return    A string with the complete string with the relative directory expanded
-    \brief     This function takes in a Repr that contains a reldir entry
-               and returns that data with the relative directory expanded.
-               Mostly it is here so that relative directories all get used
-               the same way.
-    \param     reprin   The Inkscape::XML::Node with the reldir in it.
-
-    Basically this function looks at an attribute of the Repr, and makes
-    a decision based on that.  Currently, it is only working with the
-    'extensions' relative directory, but there will be more of them.
-    One thing to notice is that this function always returns an allocated
-    string.  This means that the caller of this function can always
-    free what they are given (and should do it too!).
-*/
-std::string Script::solve_reldir(Inkscape::XML::Node *reprin) {
-
-    gchar const *s = reprin->attribute("reldir");
-
-    // right now the only recognized relative directory is "extensions"
-    if (!s || Glib::ustring(s) != "extensions") {
-        Glib::ustring str = reprin->firstChild()->content();
-        return str;
-    }
-    using namespace Inkscape::IO::Resource;
-    return get_filename(EXTENSIONS, reprin->firstChild()->content());
-}
-
-
-
-/**
-    \return   Whether the command given exists, including in the path
-    \brief    This function is used to find out if something exists for
-              the check command.  It can look in the path if required.
-    \param    command   The command or file that should be looked for
-
-    The first thing that this function does is check to see if the
-    incoming file name has a directory delimiter in it.  This would
-    mean that it wants to control the directories, and should be
-    used directly.
-
-    If not, the path is used.  Each entry in the path is stepped through,
-    attached to the string, and then tested.  If the file is found
-    then a TRUE is returned.  If we get all the way through the path
-    then a FALSE is returned, the command could not be found.
-*/
-bool Script::check_existence(const std::string &command)
-{
-    // Check the simple case first
-    if (command.empty()) {
-        return false;
-    }
-
-    //Don't search when it is an absolute path. */
-    if (Glib::path_is_absolute(command)) {
-        return Glib::file_test(command, Glib::FILE_TEST_EXISTS);
-    }
-
-    // First search in the current directory
-    std::string path = G_SEARCHPATH_SEPARATOR_S;
-    path.append(";");
-    // And then in the PATH environment variable.
-    path.append(Glib::getenv("PATH"));
-
-    std::string::size_type pos  = 0;
-    std::string::size_type pos2 = 0;
-    while ( pos < path.size() ) {
-
-        std::string localPath;
-
-        pos2 = path.find(G_SEARCHPATH_SEPARATOR, pos);
-        if (pos2 == path.npos) {
-            localPath = path.substr(pos);
-            pos = path.size();
-        } else {
-            localPath = path.substr(pos, pos2-pos);
-            pos = pos2+1;
-        }
-
-        //printf("### %s\n", localPath.c_str());
-        std::string candidatePath =
-                      Glib::build_filename(localPath, command);
-
-        if (Glib::file_test(candidatePath,
-                      Glib::FILE_TEST_EXISTS)) {
-            return true;
-        }
-
-    }
-
-    return false;
-}
-
-
-
-
-
 /**
     \return   none
     \brief    This function 'loads' an extension, basically it determines
@@ -290,13 +192,15 @@ bool Script::load(Inkscape::Extension::Extension *module)
                         std::string interpString = resolveInterpreterExecutable(interpretstr);
                         if (interpString.empty()) {
                             continue; // can't have a script extension with empty interpreter
-                        } else {
-                            command.push_back(interpString);
                         }
+                        command.push_back(interpString);
                     }
-                    command.push_back(solve_reldir(child_repr));
-                }
-                if (!strcmp(child_repr->name(), INKSCAPE_EXTENSION_NS "helper_extension")) {
+                    // TODO: we already parse commands as dependencies in extension.cpp
+                    //       can can we optimize this to be less indirect?
+                    const char *script_name = child_repr->firstChild()->content();
+                    std::string script_location = module->get_dependency_location(script_name);
+                    command.push_back(std::move(script_location));
+                } else if (!strcmp(child_repr->name(), INKSCAPE_EXTENSION_NS "helper_extension")) {
                     helper_extension = child_repr->firstChild()->content();
                 }
             }
@@ -343,21 +247,10 @@ bool Script::check(Inkscape::Extension::Extension *module)
     while (child_repr != nullptr) {
         if (!strcmp(child_repr->name(), INKSCAPE_EXTENSION_NS "script")) {
             script_count++;
+            
+            // check if all helper_extensions attached to this script were registered
             child_repr = child_repr->firstChild();
             while (child_repr != nullptr) {
-                if (!strcmp(child_repr->name(), INKSCAPE_EXTENSION_NS "check")) {
-                    std::string command_text = solve_reldir(child_repr);
-                    if (!command_text.empty()) {
-                        /* I've got the command */
-                        bool existance = check_existence(command_text);
-                        if (!existance) {
-                            return false;
-                        }
-                    } else {
-                        return false;
-                    }
-                }
-
                 if (!strcmp(child_repr->name(), INKSCAPE_EXTENSION_NS "helper_extension")) {
                     gchar const *helper = child_repr->firstChild()->content();
                     if (Inkscape::Extension::db.get(helper) == nullptr) {
@@ -929,22 +822,12 @@ int Script::execute (const std::list<std::string> &in_command,
     std::string script = interpreted ? in_command.back() : "";
     std::string working_directory = "";
 
-    // Use Glib::find_program_in_path instead of the equivalent
-    // Glib::spawn_* functionality, because _wspawnp is broken on Windows:
-    // it doesn't work when PATH contains Unicode directories
-    //
-    // Note:
-    //  - We already have an absolute path here for interpreted scripts, see Script::resolveInterpreterExecutable()
-    //  - For "normal" scripts we might not as we only use the rudimentary functionality in Script::solve_reldir().
-    //    However we already track dependencies down in a much more reliable way in Dependency::check(), so it might
-    //    make sense to eventually store and use that result instead of duplicating partial functionality here.
+    // We should always have an absolute path here:
+    //  - For interpreted scripts, see Script::resolveInterpreterExecutable()
+    //  - For "normal" scripts this should be done as part of the dependency checking, see Dependency::check()
     if (!Glib::path_is_absolute(program)) {
-        std::string found_program = Glib::find_program_in_path(program);
-        if (found_program.empty()) {
-            g_critical("Script::execute(): failed to locate program '%s' on PATH", program.c_str());
-            return 0;
-        }
-        program = found_program;
+        g_critical("Script::execute(): Got unexpected relative path '%s'. Please report a bug.", program.c_str());
+        return 0;
     }
     argv.push_back(program);
 
diff --git a/src/extension/implementation/script.h b/src/extension/implementation/script.h
index 9a3b40278..425198be3 100644
--- a/src/extension/implementation/script.h
+++ b/src/extension/implementation/script.h
@@ -76,8 +76,6 @@ private:
       */
     Gtk::Window *parent_window;
 
-    std::string solve_reldir(Inkscape::XML::Node *repr_in);
-    bool check_existence (std::string const& command);
     void copy_doc(Inkscape::XML::Node * olddoc, Inkscape::XML::Node * newdoc);
     void checkStderr (Glib::ustring const& filename, Gtk::MessageType type, Glib::ustring const& message);
 
diff --git a/src/extension/loader.cpp b/src/extension/loader.cpp
index a9270c43c..d409d09ba 100644
--- a/src/extension/loader.cpp
+++ b/src/extension/loader.cpp
@@ -56,7 +56,7 @@ Implementation::Implementation *Loader::load_implementation(Inkscape::XML::Docum
             
             // Deal with dependencies if we have them
             if (!strcmp(chname, "dependency")) {
-                Dependency dep = Dependency(child_repr);
+                Dependency dep = Dependency(child_repr, nullptr); // TODO: Why is "this" not an extension?
                 // try to load it
                 bool success = load_dependency(&dep);
                 if( !success ){