Split out ExecutableLocation (#2041)

* #include an absolute path didn't work - because paths were taken to always be relative.

* Split out ExecutableLocation.

* Fixes for changes to ExecutableLocation.

* Fix issues around Process on windows.

* Improve comments. Kick CI.

5 files changed, 167 insertions, 38 deletions

diff --git a/source/core/slang-command-line.cpp b/source/core/slang-command-line.cpp
index 419c031ab..973bb46d0 100644
--- a/source/core/slang-command-line.cpp
+++ b/source/core/slang-command-line.cpp
@@ -11,6 +11,63 @@
 
 namespace Slang {
 
+/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! ExecutableLocation !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */
+
+void ExecutableLocation::set(const String& dir, const String& name)
+{
+    if (dir.getLength() == 0)
+    {
+        set(name);
+    }
+    else
+    {
+        set(Path::combine(dir, name));
+    }
+}
+
+void ExecutableLocation::set(const String& nameOrPath)
+{
+    // See if input looks like a path
+    if (Path::hasPath(nameOrPath))
+    {
+        // If it is a path we may want to add a suffix
+        const auto suffix = Process::getExecutableSuffix();
+
+        if (suffix.getLength() == 0 || nameOrPath.endsWith(suffix))
+        {
+            setPath(nameOrPath);
+        }
+        else
+        {
+            // If on target that has suffix make sure name has the suffix
+            StringBuilder builder;
+            builder << nameOrPath;
+            builder << suffix;
+            setPath(builder.ProduceString());
+        }
+    }
+    else
+    {
+        // If we don't have a parent, we assume it is just a naem
+        setName(nameOrPath);
+    }
+}
+
+void ExecutableLocation::append(StringBuilder& out) const
+{
+    if (m_type == Type::Unknown)
+    {
+        out << "(unknown)";
+    }
+    else
+    {
+        auto escapeHandler = Process::getEscapeHandler();
+        StringEscapeUtil::appendMaybeQuoted(escapeHandler, m_pathOrName.getUnownedSlice(), out);
+    }
+}
+
+/* !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! CommandLine !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! */
+
 void CommandLine::addPrefixPathArg(const char* prefix, const String& path, const char* pathPostfix)
 {
     StringBuilder builder;
@@ -23,23 +80,29 @@ void CommandLine::addPrefixPathArg(const char* prefix, const String& path, const
     addArg(builder.ProduceString());
 }
 
-void CommandLine::setExecutable(const String& dir, const String& name)
+void CommandLine::append(StringBuilder& out) const
 {
-    StringBuilder builder;
-    Path::combineIntoBuilder(dir.getUnownedSlice(), name.getUnownedSlice(), builder);
-    builder << Process::getExecutableSuffix();
-    setExecutablePath(builder.ProduceString());
+    m_executableLocation.append(out);
+
+    if (m_args.getCount())
+    {
+        out << " ";
+        appendArgs(out);
+    }
 }
 
-void CommandLine::append(StringBuilder& out) const
+void CommandLine::appendArgs(StringBuilder& out) const
 {
     auto escapeHandler = Process::getEscapeHandler();
 
-    StringEscapeUtil::appendMaybeQuoted(escapeHandler, m_executable.getUnownedSlice(), out);
-
-    for (const auto& arg : m_args)
+    const Int argCount = m_args.getCount();
+    for (Index i = 0; i < argCount; ++i)
     {
-        out << " ";
+        const auto& arg = m_args[i];
+        if (i > 0)
+        {
+            out << " ";
+        }
         StringEscapeUtil::appendMaybeQuoted(escapeHandler, arg.getUnownedSlice(), out);
     }
 }
@@ -51,4 +114,11 @@ String CommandLine::toString() const
     return buf.ProduceString();
 }
 
+String CommandLine::toStringArgs() const
+{
+    StringBuilder buf;
+    appendArgs(buf);
+    return buf.ProduceString();
+}
+
 } // namespace Slang
diff --git a/source/core/slang-command-line.h b/source/core/slang-command-line.h
index 85ba1a331..f2007865f 100644
--- a/source/core/slang-command-line.h
+++ b/source/core/slang-command-line.h
@@ -7,15 +7,57 @@
 
 namespace Slang {
 
-struct CommandLine
+struct ExecutableLocation
 {
-    enum class ExecutableType
+    typedef ExecutableLocation ThisType;
+
+    enum Type
     {
-        Unknown,                    ///< The executable is not specified 
+        Unknown,                    ///< Not specified 
         Path,                       ///< The executable is set as a path (ie won't be searched for)
-        Filename,                   ///< The executable is set as a filename
+        Name,                       ///< The executable is passed as a name which will be searched for
     };
 
+        /// Set the executable path.
+        /// NOTE! On some targets the executable path *must* include an extension to be able to start as a process
+    void setPath(const String& path) { m_type = Type::Path; m_pathOrName = path; }
+
+        /// Set a filename (such that the path will be looked up)
+    void setName(const String& filename) { m_type = Type::Name; m_pathOrName = filename; }
+
+    void set(Type type, const String& pathOrName) { m_type = type; m_pathOrName = pathOrName; }
+
+        /// Set the executable path from a base directory and an executable name (no suffix such as '.exe' needed)
+    void set(const String& dir, const String& name);
+
+        /// Determines if it's a name or a path when it sets
+    void set(const String& nameOrPath);
+
+        /// Append as text to out. 
+    void append(StringBuilder& out) const;
+
+        /// Reset state to be same as ctor
+    void reset() { m_type = Type::Unknown; m_pathOrName = String(); }
+
+        /// Equality means exactly the same definition.
+        /// *NOT* that exactly the same executable is specified
+    bool operator==(const ThisType& rhs) const { return m_type == rhs.m_type && m_pathOrName == rhs.m_pathOrName;  }
+    bool operator!=(const ThisType& rhs) const { return !(*this == rhs); }
+
+    ExecutableLocation() {}
+    ExecutableLocation(const String& dir, const String& name) { set(dir, name);  }
+    ExecutableLocation(Type type, const String& pathOrName) : m_type(type), m_pathOrName(pathOrName) {}
+
+    explicit ExecutableLocation(const String& nameOrPath) { set(nameOrPath); }
+    
+    Type m_type = Type::Unknown;
+    String m_pathOrName;
+};
+
+struct CommandLine
+{
+    typedef CommandLine ThisType;
+
         /// Add args - assumed unescaped
     void addArg(const String& in) { m_args.add(in); }
     void addArgs(const String* args, Int argsCount) { for (Int i = 0; i < argsCount; ++i) addArg(args[i]); }
@@ -23,16 +65,6 @@ struct CommandLine
         /// Find the index of an arg which is exact match for slice
     SLANG_INLINE Index findArgIndex(const UnownedStringSlice& slice) const { return m_args.indexOf(slice); }
 
-        /// Set the executable path.
-        /// NOTE! On some targets the executable path *must* include an extension to be able to start as a process
-    void setExecutablePath(const String& path) { m_executableType = ExecutableType::Path; m_executable = path; }
-
-        /// Set the executable path from a base directory and an executable name (no suffix such as '.exe' needed)
-    void setExecutable(const String& dir, const String& name);
-
-        /// Set a filename (such that the path will be looked up
-    void setExecutableFilename(const String& filename) { m_executableType = ExecutableType::Filename; m_executable = filename; }
-
         /// For handling args where the switch is placed directly in front of the path 
     void addPrefixPathArg(const char* prefix, const String& path, const char* pathPostfix = nullptr);
 
@@ -42,17 +74,22 @@ struct CommandLine
         /// Reset to the initial state
     void reset() { *this = CommandLine();  }
 
+        /// Append the args
+    void appendArgs(StringBuilder& out) const;
+
         /// Append the command line to out
     void append(StringBuilder& out) const;
         /// convert into a string
     String toString() const;
 
-        /// Ctor
-    CommandLine():m_executableType(ExecutableType::Unknown) {}
+        /// Convert just the args to string
+    String toStringArgs() const;
+
+        /// Set an executable location
+    void setExecutableLocation(const ExecutableLocation& loc) { m_executableLocation = loc; }
 
-    ExecutableType m_executableType;    ///< How the executable is specified
-    String m_executable;                ///< Executable to run. Note that the executable is never escaped.
-    List<String> m_args;                ///< The arguments (Stored *unescaped*)
+    ExecutableLocation m_executableLocation;        ///< The executable location
+    List<String> m_args;                            ///< The arguments (Stored *unescaped*)
 };
 
 }
diff --git a/source/core/slang-io.h b/source/core/slang-io.h
index 50ce67784..de761cf4b 100644
--- a/source/core/slang-io.h
+++ b/source/core/slang-io.h
@@ -74,6 +74,11 @@ namespace Slang
         static Index findExtIndex(String const& path) { return findExtIndex(path.getUnownedSlice()); }
         static Index findExtIndex(UnownedStringSlice const& path);
 
+            /// True if isn't just a name (ie has any path separator)
+            /// Note this is no the same as having a 'parent' as '/thing' 'has a path', but it doesn't have a parent.
+        static bool hasPath(const UnownedStringSlice& path) { return findLastSeparatorIndex(path) >= 0; }
+        static bool hasPath(const String& path) { return findLastSeparatorIndex(path) >= 0; }
+
         static String replaceExt(const String& path, const char* newExt);
         static String getFileName(const String& path);
         static String getPathWithoutExt(const String& path);
diff --git a/source/core/unix/slang-unix-process.cpp b/source/core/unix/slang-unix-process.cpp
index 2c84ef08f..1f58f5725 100644
--- a/source/core/unix/slang-unix-process.cpp
+++ b/source/core/unix/slang-unix-process.cpp
@@ -355,8 +355,10 @@ static const int kCannotExecute = 126;
 {
     List<char const*> argPtrs;
 
+    const auto& exe = commandLine.m_executableLocation;
+
     // Add the command
-    argPtrs.add(commandLine.m_executable.getBuffer());
+    argPtrs.add(exe.m_pathOrName.getBuffer());
 
     // Add all the args - they don't need any explicit escaping 
     for (auto arg : commandLine.m_args)
@@ -413,9 +415,16 @@ static const int kCannotExecute = 126;
         // TODO(JS): Strictly speaking if m_executableType is 'Path' then we shouldn't be searching. Ie which
         // exec we use here should be dependent on the executable type.
 
-        // Path = execvp
-        // Filename = execv
-        ::execvp(argPtrs[0], (char* const*)&argPtrs[0]);
+        if (exe.m_type == ExecutableLocation::Type::Path)
+        {
+            // Use the specified path (ie don't search)
+            ::execv(argPtrs[0], (char* const*)&argPtrs[0]);
+        }
+        else
+        {
+            // Search for the executable
+            ::execvp(argPtrs[0], (char* const*)&argPtrs[0]);
+        }
 
         // If we get here, then `exec` failed
 
diff --git a/source/core/windows/slang-win-process.cpp b/source/core/windows/slang-win-process.cpp
index e0e38f2d4..215904938 100644
--- a/source/core/windows/slang-win-process.cpp
+++ b/source/core/windows/slang-win-process.cpp
@@ -453,12 +453,12 @@ void WinProcess::kill(int32_t returnCode)
         OSString pathBuffer;
         LPCWSTR path = nullptr;
 
-        if (commandLine.m_executableType == CommandLine::ExecutableType::Path)
+        const auto& exe = commandLine.m_executableLocation;
+        if (exe.m_type == ExecutableLocation::Type::Path)
         {
-            StringBuilder cmd;
-            StringEscapeUtil::appendMaybeQuoted(getEscapeHandler(), commandLine.m_executable.getUnownedSlice(), cmd);
-
-            pathBuffer = cmd.toWString();
+            // If it 'Path' specified we pass in as the lpApplicationName to limit
+            // searching.
+            pathBuffer = exe.m_pathOrName.toWString();
             path = pathBuffer.begin();
         }
 
@@ -479,6 +479,14 @@ void WinProcess::kill(int32_t returnCode)
             createFlags |= CREATE_SUSPENDED;
         }
 
+        // From docs:
+        // If both lpApplicationName and lpCommandLine are non-NULL, the null-terminated string pointed to by lpApplicationName
+        // specifies the module to execute, and the null-terminated string pointed to by lpCommandLine specifies the command line.
+
+        // JS:
+        // Somewhat confusingly this means that even if lpApplicationName is specified, it muse *ALSO* be included as the first
+        // whitespace delimited arg must *also* be the (possibly) quoted executable
+
         // https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createprocessa
         // `CreateProcess` requires write access to this, for some reason...
         BOOL success = CreateProcessW(