eidheim / tiny-process-library

A small platform independent library making it simple to create and stop new processes in C++, as well as writing to stdin and reading from stdout and stderr of a new process
MIT License
338 stars 73 forks source link

Passing Environment #30

Open 5cript opened 6 years ago

5cript commented 6 years ago

Please consider adding optional support for setting the environment variables on process creation. On Windows: lpEnvironment On Linux: execle instead of execl

eidheim commented 6 years ago

Feel free to create a pull request on this. I'm a bit busy in the following weeks, but I agree that this should be added.

5cript commented 6 years ago

Can't promise it, but I will do that when I'm in the mood for productivity.

I will need it in a hobby project anyway

5cript commented 6 years ago

How should those be passed? std::map <string_type, string_type>, std::unordered_map <...>, or something that satisfies "AssociativeContainer" (without using concepts ofc)?

Also, where to put the argument? because adding that inbetween some parameters would break existing programs, but adding it at the end would require users to specify "less interesting" parameters of the constructor.

I use this as an opportunity to say, that I think that passing everything to the constructor might be a less than ideal approach. I know this has "tiny" in the name, but maybe something like (rough example):

ProcessMaker bla {commandLine};
bla.path("/home/username");
bla.input([](auto const*, auto){}); 
bla.output([](auto const*, auto){});
bla.environment(/*some map*/);
Process process = bla.run();
eidheim commented 6 years ago

When using strings as keys, the default map type goto is std::unordered_map I would say.

The nice thing with constructors is that there can be potentially no unnecessary copies of the constructor arguments. I also had the std::thread-constructor in mind when creating this library. People are free to create wrapper classes to the somewhat messy constructors though:) By the way, By the way, this all reminded me to add std::move to the constructor member initializer lists (see https://github.com/eidheim/tiny-process-library/commit/e384e4cdceba87660082df76e380c1e9ad3c7a08).

I would create two more constructors like this:

--- a/process.hpp
+++ b/process.hpp
@@ -7,6 +7,7 @@
 #include <mutex>
 #include <thread>
 #include <memory>
+#include <unordered_map>
 #ifndef _WIN32
 #include <sys/wait.h>
 #endif
@@ -47,6 +48,13 @@ public:
           std::function<void(const char *bytes, size_t n)> read_stderr=nullptr,
           bool open_stdin=false,
           size_t buffer_size=131072) noexcept;
+  Process(const string_type &command,
+          const std::unordered_map<string_type, string_type> &environment,
+          const string_type &path=string_type(),
+          std::function<void(const char *bytes, size_t n)> read_stdout = nullptr,
+          std::function<void(const char *bytes, size_t n)> read_stderr = nullptr,
+          bool open_stdin = false,
+          size_t buffer_size = 131072) noexcept;
 #ifndef _WIN32
   /// Supported on Unix-like systems only.
   Process(std::function<void()> function,
@@ -54,6 +62,13 @@ public:
           std::function<void(const char *bytes, size_t n)> read_stderr=nullptr,
           bool open_stdin=false,
           size_t buffer_size=131072) noexcept;
+  /// Supported on Unix-like systems only.
+  Process(std::function<void()> function,
+          const std::unordered_map<string_type, string_type> &environment,
+          std::function<void(const char *bytes, size_t n)> read_stdout = nullptr,
+          std::function<void(const char *bytes, size_t n)> read_stderr = nullptr,
+          bool open_stdin = false,
+          size_t buffer_size = 131072) noexcept;
 #endif
   ~Process() noexcept;

Maybe later we can deprecate the old constructors. Or even instead add one more without path:)

5cript commented 6 years ago

The old ones cannot be deprecated when the environment is passed by const&, because an empty environment is just as valid as using the default one. (empty != ignore parameter).

eidheim commented 6 years ago

You are right, but I do not see multiple constructors as a problem here.