From 5226ec2ba3a252396d6e35b8d7df461eb2f485ee Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 15:29:22 +0000 Subject: [PATCH 1/7] std.process: Rename spawnProcessImpl to spawnProcessWin / Posix Improve code clarity and remove platform-specific "overloads" with functions with the same name but different argument lists. --- std/process.d | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/std/process.d b/std/process.d index af029e5e8..e180e3f8d 100644 --- a/std/process.d +++ b/std/process.d @@ -815,11 +815,11 @@ Pid spawnProcess(scope const(char[])[] args, { const commandLine = escapeShellArguments(args); const program = args.length ? args[0] : null; - return spawnProcessImpl(commandLine, program, stdin, stdout, stderr, env, config, workDir); + return spawnProcessWin(commandLine, program, stdin, stdout, stderr, env, config, workDir); } else version (Posix) { - return spawnProcessImpl(args, stdin, stdout, stderr, env, config, workDir); + return spawnProcessPosix(args, stdin, stdout, stderr, env, config, workDir); } else static assert(0); @@ -882,13 +882,13 @@ envz should be a zero-terminated array of zero-terminated strings on the form "var=value". */ version (Posix) -private Pid spawnProcessImpl(scope const(char[])[] args, - File stdin, - File stdout, - File stderr, - scope const string[string] env, - Config config, - scope const(char)[] workDir) +private Pid spawnProcessPosix(scope const(char[])[] args, + File stdin, + File stdout, + File stderr, + scope const string[string] env, + Config config, + scope const(char)[] workDir) @trusted // TODO: Should be @safe { import core.exception : RangeError; @@ -1217,14 +1217,14 @@ envz must be a pointer to a block of UTF-16 characters on the form "var1=value1\0var2=value2\0...varN=valueN\0\0". */ version (Windows) -private Pid spawnProcessImpl(scope const(char)[] commandLine, - scope const(char)[] program, - File stdin, - File stdout, - File stderr, - const string[string] env, - Config config, - scope const(char)[] workDir) +private Pid spawnProcessWin(scope const(char)[] commandLine, + scope const(char)[] program, + File stdin, + File stdout, + File stderr, + const string[string] env, + Config config, + scope const(char)[] workDir) @trusted { import core.exception : RangeError; @@ -1875,7 +1875,7 @@ Pid spawnShell(scope const(char)[] command, // See CMD.EXE /? for details. const commandLine = escapeShellFileName(shellPath) ~ ` ` ~ shellSwitch ~ ` "` ~ command ~ `"`; - return spawnProcessImpl(commandLine, shellPath, stdin, stdout, stderr, env, config, workDir); + return spawnProcessWin(commandLine, shellPath, stdin, stdout, stderr, env, config, workDir); } else version (Posix) { @@ -1883,7 +1883,7 @@ Pid spawnShell(scope const(char)[] command, args[0] = shellPath; args[1] = shellSwitch; args[2] = command; - return spawnProcessImpl(args, stdin, stdout, stderr, env, config, workDir); + return spawnProcessPosix(args, stdin, stdout, stderr, env, config, workDir); } else static assert(0); From cdd98245317cbdfcfe05cda8e4f81712ffedf8ea Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 15:34:39 +0000 Subject: [PATCH 2/7] std.process: Remove unnecessary @trusted --- std/process.d | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/std/process.d b/std/process.d index e180e3f8d..e8003d358 100644 --- a/std/process.d +++ b/std/process.d @@ -809,7 +809,7 @@ Pid spawnProcess(scope const(char[])[] args, const string[string] env = null, Config config = Config.none, scope const char[] workDir = null) - @trusted // TODO: Should be @safe + @safe { version (Windows) { @@ -1222,7 +1222,7 @@ private Pid spawnProcessWin(scope const(char)[] commandLine, File stdin, File stdout, File stderr, - const string[string] env, + scope const string[string] env, Config config, scope const(char)[] workDir) @trusted @@ -1865,7 +1865,7 @@ Pid spawnShell(scope const(char)[] command, Config config = Config.none, scope const(char)[] workDir = null, scope string shellPath = nativeShell) - @trusted // TODO: Should be @safe + @safe { version (Windows) { @@ -3046,7 +3046,7 @@ auto execute(scope const(char[])[] args, Config config = Config.none, size_t maxOutput = size_t.max, scope const(char)[] workDir = null) - @trusted //TODO: @safe + @safe { return executeImpl!pipeProcess(args, env, config, maxOutput, workDir); } @@ -3057,7 +3057,7 @@ auto execute(scope const(char)[] program, Config config = Config.none, size_t maxOutput = size_t.max, scope const(char)[] workDir = null) - @trusted //TODO: @safe + @safe { return executeImpl!pipeProcess(program, env, config, maxOutput, workDir); } @@ -3069,7 +3069,7 @@ auto executeShell(scope const(char)[] command, size_t maxOutput = size_t.max, scope const(char)[] workDir = null, string shellPath = nativeShell) - @trusted //TODO: @safe + @safe { return executeImpl!pipeShell(command, env, @@ -3087,6 +3087,7 @@ private auto executeImpl(alias pipeFunc, Cmd, ExtraPipeFuncArgs...)( size_t maxOutput = size_t.max, scope const(char)[] workDir = null, ExtraPipeFuncArgs extraArgs = ExtraPipeFuncArgs.init) + @trusted //TODO: @safe { import std.algorithm.comparison : min; import std.array : appender; From de8207f62d23bcb03c2c6ed72395b1b65d081f64 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 15:34:54 +0000 Subject: [PATCH 3/7] std.process: Remove unfixable TODO Environment.remove cannot be @safe due to its use of the C functions SetEnvironmentVariableW and unsetenv, and doesn't have sufficient functionality to encapsulate the non-@safe fragments. --- std/process.d | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/std/process.d b/std/process.d index e8003d358..eb7961804 100644 --- a/std/process.d +++ b/std/process.d @@ -322,7 +322,7 @@ static: multi-threaded programs. See e.g. $(LINK2 https://www.gnu.org/software/libc/manual/html_node/Environment-Access.html#Environment-Access, glibc). */ - void remove(scope const(char)[] name) @trusted nothrow @nogc // TODO: @safe + void remove(scope const(char)[] name) @trusted nothrow @nogc { version (Windows) SetEnvironmentVariableW(name.tempCStringW(), null); else version (Posix) core.sys.posix.stdlib.unsetenv(name.tempCString()); From 3b0aad98a1f1893c27aa19f452606bbaccb411f6 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 16:10:15 +0000 Subject: [PATCH 4/7] std.process: Refactor environment - Perform idup caching on all platforms (on Windows it also avoids eliding conversion to UTF-8) - Make getImpl more reusable - Reduce redundancy between get / opIndex --- std/process.d | 77 +++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 39 deletions(-) diff --git a/std/process.d b/std/process.d index eb7961804..ca06d8984 100644 --- a/std/process.d +++ b/std/process.d @@ -213,9 +213,7 @@ static: string opIndex(scope const(char)[] name) @safe { import std.exception : enforce; - string value; - enforce(getImpl(name, value), "Environment variable not found: "~name); - return value; + return get(name, null).enforce("Environment variable not found: "~name); } /** @@ -253,8 +251,8 @@ static: string get(scope const(char)[] name, string defaultValue = null) @safe { string value; - auto found = getImpl(name, value); - return found ? value : defaultValue; + getImpl(name, (result) { value = result ? cachedToString(result) : defaultValue; }); + return value; } /** @@ -446,8 +444,13 @@ static: } private: - // Retrieves the environment variable, returns false on failure. - bool getImpl(scope const(char)[] name, out string value) @trusted + version (Windows) alias OSChar = WCHAR; + else version (Posix) alias OSChar = char; + + // Retrieves the environment variable. Calls `sink` with a + // temporary buffer of OS characters, or `null` if the variable + // doesn't exist. + void getImpl(scope const(char)[] name, scope void delegate(const(OSChar)[]) @safe sink) @trusted { version (Windows) { @@ -468,15 +471,12 @@ private: { immutable err = GetLastError(); if (err == ERROR_ENVVAR_NOT_FOUND) - return false; + return sink(null); if (err != NO_ERROR) // Some other Windows error, throw. throw new WindowsException(err); } if (len <= 1) - { - value = ""; - return true; - } + return sink(""); buf.length = len; while (true) @@ -488,21 +488,15 @@ private: { immutable err = GetLastError(); if (err == NO_ERROR) // sucessfully read a 0-length variable - { - value = ""; - return true; - } + return sink(""); if (err == ERROR_ENVVAR_NOT_FOUND) // variable didn't exist - return false; + return sink(null); // some other windows error throw new WindowsException(err); } assert(lenRead != buf.length, "impossible according to msft docs"); if (lenRead < buf.length) // the buffer was long enough - { - value = toUTF8(buf[0 .. lenRead]); - return true; - } + return sink(buf[0 .. lenRead]); // resize and go around again, because the environment variable grew buf.length = lenRead; } @@ -512,26 +506,31 @@ private: import core.stdc.string : strlen; const vz = core.sys.posix.stdlib.getenv(name.tempCString()); - if (vz == null) return false; - auto v = vz[0 .. strlen(vz)]; - - // Cache the last call's result. - static string lastResult; - if (v.empty) - { - // Return non-null array for blank result to distinguish from - // not-present result. - lastResult = ""; - } - else if (v != lastResult) - { - lastResult = v.idup; - } - value = lastResult; - return true; - } + if (vz == null) return sink(null); + return sink(vz[0 .. strlen(vz)]); + } else static assert(0); } + + string cachedToString(C)(scope const(C)[] v) @safe + { + import std.algorithm.comparison : equal; + + // Cache the last call's result. + static string lastResult; + if (v.empty) + { + // Return non-null array for blank result to distinguish from + // not-present result. + lastResult = ""; + } + else if (!v.equal(lastResult)) + { + import std.conv : to; + lastResult = v.to!string; + } + return lastResult; + } } @safe unittest From cca9f214cc9297aefe69300ca84ecb0673ffca99 Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 16:18:37 +0000 Subject: [PATCH 5/7] std.process: Make isExecutable accept a range --- std/process.d | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/std/process.d b/std/process.d index ca06d8984..baa6844b0 100644 --- a/std/process.d +++ b/std/process.d @@ -108,6 +108,7 @@ version (Windows) import std.internal.cstring; import std.range.primitives; import std.stdio; +import std.traits : isSomeChar; version (OSX) version = Darwin; @@ -1454,7 +1455,8 @@ private string searchPathFor(scope const(char)[] executable) // Checks whether the file exists and can be executed by the // current user. version (Posix) -private bool isExecutable(scope const(char)[] path) @trusted nothrow @nogc //TODO: @safe +private bool isExecutable(R)(R path) @trusted nothrow @nogc +if (isInputRange!R && isSomeChar!(ElementEncodingType!R)) { return (access(path.tempCString(), X_OK) == 0); } From 5cc514835aadd3e9802cc92ad4dac54a705a0dbd Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 16:18:55 +0000 Subject: [PATCH 6/7] std.process: Improve searchPathFor - Make @safe - Reduce allocations --- std/process.d | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/std/process.d b/std/process.d index baa6844b0..c4265a4fb 100644 --- a/std/process.d +++ b/std/process.d @@ -1434,22 +1434,32 @@ version (Windows) @system unittest // (checking that it is in fact executable). version (Posix) private string searchPathFor(scope const(char)[] executable) - @trusted //TODO: @safe nothrow + @safe { import std.algorithm.iteration : splitter; - import std.conv : to; - import std.path : buildPath; + import std.conv : text; + import std.path : chainPath; - auto pathz = core.stdc.stdlib.getenv("PATH"); - if (pathz == null) return null; + string result; - foreach (dir; splitter(to!string(pathz), ':')) - { - auto execPath = buildPath(dir, executable); - if (isExecutable(execPath)) return execPath; - } + environment.getImpl("PATH", + (scope const(char)[] path) + { + if (!path) + return; - return null; + foreach (dir; splitter(path, ":")) + { + auto execPath = chainPath(dir, executable); + if (isExecutable(execPath)) + { + result = text(execPath); + return; + } + } + }); + + return result; } // Checks whether the file exists and can be executed by the From 05ab29770c71bbdbaec619eefc3b7c9b31ce64ae Mon Sep 17 00:00:00 2001 From: Vladimir Panteleev Date: Thu, 17 Dec 2020 16:43:28 +0000 Subject: [PATCH 7/7] std.process: Simplify error message generation on Windows Based on #7394 and as discussed in #7726. --- std/process.d | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/std/process.d b/std/process.d index c4265a4fb..877c7876c 100644 --- a/std/process.d +++ b/std/process.d @@ -1291,12 +1291,7 @@ private Pid spawnProcessWin(scope const(char)[] commandLine, if (!CreateProcessW(null, commandLine.tempCStringW().buffPtr, null, null, true, dwCreationFlags, envz, workDir.length ? pworkDir : null, &startinfo, &pi)) - { - if (GetLastError() == ERROR_FILE_NOT_FOUND) - throw new ProcessException(text("Executable file not found: ", program)); - else - throw ProcessException.newFromLastError("Failed to spawn new process"); - } + throw ProcessException.newFromLastError("Failed to spawn process \"" ~ cast(string) program ~ '"'); // figure out if we should close any of the streams if (!(config & Config.retainStdin ) && stdinFD > STDERR_FILENO