Factor out lookupExecutable and other PATH improvments

This ended up motivating a good deal of other infra improvements in
order to get Windows right:

- `OsString` to complement `std::filesystem::path`

- env var code for working with the underlying `OsString`s

- Rename `PATHNG_LITERAL` to `OS_STR`

- `NativePathTrait` renamed to `OsPathTrait`, given a character template
  parameter until NixOS#9205 is complete.

Split `tests.cc` matching split of `util.{cc,hh}` last year.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>

.clang-format

@@ -31,3 +31,4 @@ AlwaysBreakBeforeMultilineStrings: true
 IndentPPDirectives: AfterHash
 PPIndentWidth: 2
 BinPackArguments: false
+BreakBeforeTernaryOperators: true

maintainers/flake-module.nix

@@ -275,7 +275,6 @@
             ''^src/libutil/current-process\.hh$''
             ''^src/libutil/english\.cc$''
             ''^src/libutil/english\.hh$''
-            ''^src/libutil/environment-variables\.cc$''
             ''^src/libutil/error\.cc$''
             ''^src/libutil/error\.hh$''
             ''^src/libutil/exit\.hh$''
@@ -357,7 +356,6 @@
             ''^src/libutil/util\.cc$''
             ''^src/libutil/util\.hh$''
             ''^src/libutil/variant-wrapper\.hh$''
-            ''^src/libutil/windows/environment-variables\.cc$''
             ''^src/libutil/windows/file-descriptor\.cc$''
             ''^src/libutil/windows/file-path\.cc$''
             ''^src/libutil/windows/processes\.cc$''
@@ -485,7 +483,6 @@
             ''^tests/unit/libutil/pool\.cc''
             ''^tests/unit/libutil/references\.cc''
             ''^tests/unit/libutil/suggestions\.cc''
-            ''^tests/unit/libutil/tests\.cc''
             ''^tests/unit/libutil/url\.cc''
             ''^tests/unit/libutil/xml-writer\.cc''
           ];

src/libutil/environment-variables.cc

@@ -1,20 +1,23 @@
 #include "util.hh"
 #include "environment-variables.hh"
 
-extern char * * environ __attribute__((weak));
+extern char ** environ __attribute__((weak));
 
 namespace nix {
 
 std::optional<std::string> getEnv(const std::string & key)
 {
     char * value = getenv(key.c_str());
-    if (!value) return {};
+    if (!value)
+        return {};
     return std::string(value);
 }
 
-std::optional<std::string> getEnvNonEmpty(const std::string & key) {
+std::optional<std::string> getEnvNonEmpty(const std::string & key)
+{
     auto value = getEnv(key);
-    if (value == "") return {};
+    if (value == "")
+        return {};
     return value;
 }
 

src/libutil/environment-variables.hh

@@ -9,6 +9,7 @@
 #include <optional>
 
 #include "types.hh"
+#include "file-path.hh"
 
 namespace nix {
 
@@ -17,6 +18,11 @@ namespace nix {
  */
 std::optional<std::string> getEnv(const std::string & key);
 
+/**
+ * Like `getEnv`, but using `OsString` to avoid coercions.
+ */
+std::optional<OsString> getEnvOs(const OsString & key);
+
 /**
  * @return a non empty environment variable. Returns nullopt if the env
  * variable is set to ""
@@ -43,6 +49,11 @@ int unsetenv(const char * name);
  */
 int setEnv(const char * name, const char * value);
 
+/**
+ * Like `setEnv`, but using `OsString` to avoid coercions.
+ */
+int setEnvOs(const OsString & name, const OsString & value);
+
 /**
  * Clear the environment.
  */

src/libutil/executable-path.cc

@@ -0,0 +1,79 @@
+#include "environment-variables.hh"
+#include "executable-path.hh"
+#include "strings-inline.hh"
+#include "util.hh"
+#include "file-path-impl.hh"
+
+namespace nix {
+
+namespace fs = std::filesystem;
+
+constexpr static const OsStringView path_var_separator{
+    &ExecutablePath::separator,
+    1,
+};
+
+ExecutablePath ExecutablePath::load()
+{
+    // "If PATH is unset or is set to null, the path search is
+    // implementation-defined."
+    // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
+    return ExecutablePath::parse(getEnvOs(OS_STR("PATH")).value_or(OS_STR("")));
+}
+
+ExecutablePath ExecutablePath::parse(const OsString & path)
+{
+    auto strings = path.empty() ? (std::list<OsString>{})
+                                : basicSplitString<std::list<OsString>, OsString::value_type>(path, path_var_separator);
+
+    std::vector<fs::path> ret;
+    ret.reserve(strings.size());
+
+    std::transform(
+        std::make_move_iterator(strings.begin()),
+        std::make_move_iterator(strings.end()),
+        std::back_inserter(ret),
+        [](auto && str) {
+            return fs::path{
+                str.empty()
+                    // "A zero-length prefix is a legacy feature that
+                    // indicates the current working directory. It
+                    // appears as two adjacent <colon> characters
+                    // ("::"), as an initial <colon> preceding the rest
+                    // of the list, or as a trailing <colon> following
+                    // the rest of the list."
+                    // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
+                    ? OS_STR(".")
+                    : std::move(str),
+            };
+        });
+
+    return {ret};
+}
+
+OsString ExecutablePath::render() const
+{
+    std::vector<PathViewNG> path2;
+    for (auto & p : directories)
+        path2.push_back(p.native());
+    return basicConcatStringsSep(path_var_separator, path2);
+}
+
+std::optional<fs::path>
+ExecutablePath::find(const OsString & exe, std::function<bool(const fs::path &)> isExecutable) const
+{
+    // "If the pathname being sought contains a <slash>, the search
+    // through the path prefixes shall not be performed."
+    // https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
+    assert(OsPathTrait<fs::path::value_type>::rfindPathSep(exe) == exe.npos);
+
+    for (auto & dir : directories) {
+        auto candidate = dir / exe;
+        if (isExecutable(candidate))
+            return std::filesystem::canonical(candidate);
+    }
+
+    return std::nullopt;
+}
+
+} // namespace nix

src/libutil/executable-path.hh

@@ -0,0 +1,64 @@
+#pragma once
+///@file
+
+#include "file-system.hh"
+
+namespace nix {
+
+struct ExecutablePath
+{
+    std::vector<std::filesystem::path> directories;
+
+    constexpr static const OsString::value_type separator =
+#ifdef WIN32
+        L';'
+#else
+        ':'
+#endif
+        ;
+
+    /**
+     * Parse `path` into a list of paths.
+     *
+     * On Unix we split on `:`, on Windows we split on `;`.
+     *
+     * For Unix, this is according to the POSIX spec for `PATH`.
+     * https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
+     */
+    static ExecutablePath parse(const OsString & path);
+
+    /**
+     * Load the `PATH` environment variable and `parse` it.
+     */
+    static ExecutablePath load();
+
+    /**
+     * Opposite of `parse`
+     */
+    OsString render() const;
+
+    /**
+     * Search for an executable.
+     *
+     * For Unix, this is according to the POSIX spec for `PATH`.
+     * https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
+     *
+     * @param exe This must just be a name, and not contain any `/` (or
+     * `\` on Windows). in case it does, per the spec no lookup should
+     * be perfomed, and the path (it is not just a file name) as is.
+     * This is the caller's respsonsibility.
+     *
+     * This is a pure function, except for the default `isExecutable`
+     * argument, which uses the ambient file system to check if a file is
+     * executable (and exists).
+     *
+     * @return path to a resolved executable
+     */
+    std::optional<std::filesystem::path> find(
+        const OsString & exe,
+        std::function<bool(const std::filesystem::path &)> isExecutableFile = isExecutableFileAmbient) const;
+
+    bool operator==(const ExecutablePath &) const = default;
+};
+
+} // namespace nix

src/libutil/file-path-impl.hh

@@ -91,13 +91,10 @@ struct WindowsPathTrait
 };
 
 
-/**
- * @todo Revisit choice of `char` or `wchar_t` for `WindowsPathTrait`
- * argument.
- */
-using NativePathTrait =
+template<typename CharT>
+using OsPathTrait =
 #ifdef _WIN32
-    WindowsPathTrait<char>
+    WindowsPathTrait<CharT>
 #else
     UnixPathTrait
 #endif

src/libutil/file-path.hh

@@ -1,10 +1,10 @@
 #pragma once
 ///@file
 
-#include <optional>
 #include <filesystem>
 
 #include "types.hh"
+#include "os-string.hh"
 
 namespace nix {
 
@@ -22,39 +22,26 @@ typedef std::set<std::filesystem::path> PathSetNG;
  *
  * @todo drop `NG` suffix and replace the one in `types.hh`.
  */
-struct PathViewNG : std::basic_string_view<std::filesystem::path::value_type>
+struct PathViewNG : OsStringView
 {
-    using string_view = std::basic_string_view<std::filesystem::path::value_type>;
+    using string_view = OsStringView;
 
     using string_view::string_view;
 
     PathViewNG(const std::filesystem::path & path)
-        : std::basic_string_view<std::filesystem::path::value_type>(path.native())
+        : OsStringView{path.native()}
     { }
 
-    PathViewNG(const std::filesystem::path::string_type & path)
-        : std::basic_string_view<std::filesystem::path::value_type>(path)
+    PathViewNG(const OsString & path)
+        : OsStringView{path}
     { }
 
     const string_view & native() const { return *this; }
     string_view & native() { return *this; }
 };
 
-std::string os_string_to_string(PathViewNG::string_view path);
-
-std::filesystem::path::string_type string_to_os_string(std::string_view s);
-
 std::optional<std::filesystem::path> maybePath(PathView path);
 
 std::filesystem::path pathNG(PathView path);
 
-/**
- * Create string literals with the native character width of paths
- */
-#ifndef _WIN32
-# define PATHNG_LITERAL(s) s
-#else
-# define PATHNG_LITERAL(s) L ## s
-#endif
-
 }

src/libutil/file-system.cc

@@ -92,7 +92,7 @@ Path canonPath(PathView path, bool resolveSymlinks)
        arbitrary (but high) limit to prevent infinite loops. */
     unsigned int followCount = 0, maxFollow = 1024;
 
-    auto ret = canonPathInner<NativePathTrait>(
+    auto ret = canonPathInner<OsPathTrait<char>>(
         path,
         [&followCount, &temp, maxFollow, resolveSymlinks]
         (std::string & result, std::string_view & remaining) {
@@ -122,7 +122,7 @@ Path canonPath(PathView path, bool resolveSymlinks)
 
 Path dirOf(const PathView path)
 {
-    Path::size_type pos = NativePathTrait::rfindPathSep(path);
+    Path::size_type pos = OsPathTrait<char>::rfindPathSep(path);
     if (pos == path.npos)
         return ".";
     return fs::path{path}.parent_path().string();
@@ -135,10 +135,10 @@ std::string_view baseNameOf(std::string_view path)
         return "";
 
     auto last = path.size() - 1;
-    while (last > 0 && NativePathTrait::isPathSep(path[last]))
+    while (last > 0 && OsPathTrait<char>::isPathSep(path[last]))
         last -= 1;
 
-    auto pos = NativePathTrait::rfindPathSep(path, last);
+    auto pos = OsPathTrait<char>::rfindPathSep(path, last);
     if (pos == path.npos)
         pos = 0;
     else
@@ -569,7 +569,7 @@ void replaceSymlink(const Path & target, const Path & link)
 }
 
 void setWriteTime(
-    const std::filesystem::path & path,
+    const fs::path & path,
     time_t accessedTime,
     time_t modificationTime,
     std::optional<bool> optIsSymlink)
@@ -685,4 +685,17 @@ void moveFile(const Path & oldName, const Path & newName)
 
 //////////////////////////////////////////////////////////////////////
 
+bool isExecutableFileAmbient(const fs::path & exe) {
+    // Check file type, because directory being executable means
+    // something completely different.
+    return std::filesystem::is_regular_file(exe)
+        && access(exe.string().c_str(),
+#ifdef WIN32
+        0 // TODO do better
+#else
+        X_OK
+#endif
+        ) == 0;
+}
+
 }

src/libutil/file-system.hh

@@ -263,6 +263,12 @@ std::pair<AutoCloseFD, Path> createTempFile(const Path & prefix = "nix");
  */
 Path defaultTempDir();
 
+/**
+ * Interpret `exe` as a location in the ambient file system and return
+ * whether it exists AND is executable.
+ */
+bool isExecutableFileAmbient(const std::filesystem::path & exe);
+
 /**
  * Used in various places.
  */