Made it work on OS X. #16
Conversation
…rent#1004 is merged in.
| } | ||
| return *p; | ||
| } | ||
| #ifndef C5T_DLIB | ||
| C5T_ACTOR_MODEL_Interface& GetSingleton(); |
There was a problem hiding this comment.
Enabled by now-merged C5T/Current#1004, before this was silently OK on Linux, but failing on OS X.
| @@ -55,8 +55,10 @@ struct C5T_LOGGER_SINGLETON_Interface { | |||
| virtual C5T_LOGGER_Interface& operator[](std::string const& log_file_name) = 0; | |||
| }; | |||
|
|
|||
| #ifndef C5T_DLIB | |||
There was a problem hiding this comment.
Enabled by now-merged C5T/Current#1004, before this was silently OK on Linux, but failing on OS X.
| @@ -141,9 +141,9 @@ void C5T_POPEN2(std::vector<std::string> const& cmdline, | |||
| return std::thread( | |||
| [](std::function<void(const std::string)> cb_line, int read_fd, int terminate_fd) { | |||
| struct pollfd fds[2]; | |||
| fds[0].fd = read_fd; | |||
There was a problem hiding this comment.
Here the tweak works since I now check the termination signal first.
(Strictly speaking, there was no need to flip them in the args to ::poll(), just swapping the if-s would do.)
My best guess is that the true reason this code did not work before on OS X is that once the child process is terminated, ::poll() notifies about its stdout as well.
I did not explore much further, just made sure to give priority to the check that the ::waitpid() has already succeeded.
Tested afresh with https://github.com/dimacurrentai/exec_on_macos/.
| @@ -10,34 +10,34 @@ | |||
|
|
|||
| TEST(Popen2Test, Smoke) { | |||
| std::string result; | |||
| C5T_POPEN2({"/usr/bin/bash", "-c", "echo PASS"}, [&result](std::string const& line) { result = line; }); | |||
| C5T_POPEN2({"bash", "-c", "echo PASS"}, [&result](std::string const& line) { result = line; }); | |||
There was a problem hiding this comment.
And the last change was that /usr/bin/bash (which was incorrectly hardcoded) is not available on OS X. I've changed it into /bin/bash first, but then realized that just bash would do fine.
|
Merging now. |
Thanks @yarmaksergey, and ref. C5T/Current#1004.