Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Passing Environment #30

Open
5cript opened this issue Feb 13, 2018 · 6 comments
Open

Passing Environment #30

5cript opened this issue Feb 13, 2018 · 6 comments

Comments

@5cript
Copy link

5cript commented Feb 13, 2018

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

@eidheim
Copy link
Owner

eidheim commented Feb 13, 2018

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
Copy link
Author

5cript commented Feb 14, 2018

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
Copy link
Author

5cript commented Feb 16, 2018

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
Copy link
Owner

eidheim commented Feb 22, 2018

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 e384e4c).

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
Copy link
Author

5cript commented Feb 22, 2018

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
Copy link
Owner

eidheim commented Feb 27, 2018

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants