----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51233/#review146706 -----------------------------------------------------------
Mostly naming and comment suggestions. But there's one major question near the bottom of this review. 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 67) <https://reviews.apache.org/r/51233/#comment213271> Comment suggestion: ``` // Retrieves system environment in a std::map. // These do not include environment variables from the current process. ``` 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 68) <https://reviews.apache.org/r/51233/#comment213283> I checked this: https://msdn.microsoft.com/en-us/library/windows/desktop/bb762270(v=vs.85).aspx Which says: > If th[e second] parameter is NULL, the returned environment block contains system variables only. s/getCUEnvironment/getSystemEnvironment/ 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 73) <https://reviews.apache.org/r/51233/#comment213282> Let's use descriptive variable names, here and everywhere. Maybe: s/cuEnv/userEnvironment/ 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 74) <https://reviews.apache.org/r/51233/#comment213279> There's a double space here. s/envPtr/environmentEntry/ 3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 76 - 78) <https://reviews.apache.org/r/51233/#comment213284> Suggested comment: ``` // Get the system environment. // The third parameter (bool) tells the function *not* to inherit // variables from the current process. ``` 3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 81 - 82) <https://reviews.apache.org/r/51233/#comment213280> No newline after the `if (...)` Also, the `envRet` seems to be unused elsewhere, so you can do: ``` if (!CreateEnvironmentBlock((LPVOID*)&envPtr, nullptr, FALSE)) { ... } ``` 3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 86 - 87) <https://reviews.apache.org/r/51233/#comment213285> Suggestion: ``` // Save the environment block in order to destroy it later. wchar_t* environmentBlock = environmentEntry; ``` 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 89) <https://reviews.apache.org/r/51233/#comment213286> Add a comment here, saying something like: ``` The environment block is a list of null-terminated strings, where the list is itself null-terminated. i.e. foo=1\0bar=2\0\0 ``` 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 128) <https://reviews.apache.org/r/51233/#comment213269> Add a comment above this line: https://github.com/apache/mesos/blob/3674c58ad5268c96c3868dda3d43375f0f7b02a7/src/slave/slave.cpp#L6319-L6323 And then `#ifdef` the `PATH` replacement there on Windows. s/cuEnv/systemEnvironment/ 3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 130 - 132) <https://reviews.apache.org/r/51233/#comment213291> Do we really want to return `None` here? This would make the subprocess inherit environment variables. And it sounds like a problem if the system environment is blank. What would be more appropriate to? 1) CHECK that the system environment is non-empty. 2) Ignore an empty system environment and stringify the `env`. 3rdparty/libprocess/include/process/windows/subprocess.hpp (line 134) <https://reviews.apache.org/r/51233/#comment213292> s/combEnv/combinedEnvironment/ - Joseph Wu On Aug. 19, 2016, 10:38 a.m., Daniel Pravat wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51233/ > ----------------------------------------------------------- > > (Updated Aug. 19, 2016, 10:38 a.m.) > > > Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, > Joseph Wu, and Michael Park. > > > Repository: mesos > > > Description > ------- > > Build a clean `Windows` environment for `mesos-executor`. > > > Diffs > ----- > > 3rdparty/libprocess/include/process/windows/subprocess.hpp > 6bb54c878fbdc4b53c9d4f3298530cbcf55d88b8 > > Diff: https://reviews.apache.org/r/51233/diff/ > > > Testing > ------- > > > Thanks, > > Daniel Pravat > >
