----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55547/#review162754 -----------------------------------------------------------
I agree with this change, just a few questions/tweaks: 3rdparty/stout/include/stout/os/raw/environment.hpp (lines 107 - 119) <https://reviews.apache.org/r/55547/#comment234100> Should we also move away from functions like `os::execvpe`? If so, we would be able to completely exclude `stout/raw/environment.hpp` from the Windows headers. `Envp` is currently only used in one location: https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/src/slave/containerizer/mesos/launch.cpp#L659-L684 3rdparty/stout/include/stout/os/windows/environment.hpp (lines 22 - 40) <https://reviews.apache.org/r/55547/#comment234098> We should refactor this code along with some very similar parsing logic here: https://github.com/apache/mesos/blob/f1d0cdf1db2a28fa44f7aded0c3760636c0a51de/3rdparty/libprocess/include/process/windows/subprocess.hpp#L90-L114 3rdparty/stout/tests/os_tests.cpp (line 74) <https://reviews.apache.org/r/55547/#comment234099> Remove the disabled macro, since this is now ifdef'd - Joseph Wu On Jan. 15, 2017, 1:44 p.m., Alex Clemmer wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55547/ > ----------------------------------------------------------- > > (Updated Jan. 15, 2017, 1:44 p.m.) > > > Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. > > > Bugs: MESOS-5880 > https://issues.apache.org/jira/browse/MESOS-5880 > > > Repository: mesos > > > Description > ------- > > Windows currently exposes two APIs for managing a process's environment > variables: a CRT API, and a win32 API. This commit will transition Stout > to use only the win32 API, and retire its use of the CRT APIs. > > There are many reasons for this, for example: > > * Stout currently uses both the CRT and win32 APIs, but they are > incompatible, and this causes real bugs. For example, because > `os::setenv` uses the win32 API, but `os::environment` uses the CRT > API, it is possible to set an environment variable and then later not > see it reflected in the environment. In Mesos this causes many bugs, > such as when we expect to set `LIBPROCESS_PORT` in a parent, then > spawn a health checker, this port doesn't get picked up. > * The CRT API is very old, and essentially unmaintained. It should not > be used unless it is necessary. > * It is generally easier to mirror the most common POSIX semantics of > environment APIs with the win32 API than it is with the CRT API. For > example, the Windows CRT implementation of `setenv` will delete an > environment variable if you pass an empty string as value, instead of > setting the value to be an empty string, like most modern POSIX > implementations. On the other hand, the win32 equivalent, > `SetEnvironmentVariable`, does implement this behavior. > > The effort to standardize the Windows code paths essentially involves > two things: > > (1) Removing `os::raw::environment` from Stout's Windows API. > > `os::raw::environment` is not used by the Windows codepaths, and (for > reasons above) we want to avoid is accidental use of `environ` on > Windows in the future, as well. > > While it is possible to simply implement `os::raw::environment` using > the win32 API `GetEnvironmentStrings`, these return fundamentally > different types, so the allocation logic would become more complex, and > the semantics of the function would have to change. Either the user > would have to allocate a `char**` for the environment, or Stout would > have to manage a `static char**`, or the function would have to allocate > memory for the user to `free`. All of these are at odds with the POSIX > semantics, and since this API is only used on POSIX paths, there is no > real advantage in this line of inquiry. > > (2) Splitting up the implementation of `os::environment`. > > The POSIX `environ` and Windows `GetEnvironmentStrings` are > fundamentally different types, and require mostly different processing > logic to transform them to a `hashmap`. There is no real advantage in > convoluting this processing code to keep the code common between them. > > > Diffs > ----- > > 3rdparty/stout/include/Makefile.am 53d04a9b6c4a0d8b35d3c84ef24d619fdb8a2c82 > 3rdparty/stout/include/stout/os/environment.hpp > d8c34999829257bee80b0678f2ee096f4798c62b > 3rdparty/stout/include/stout/os/posix/environment.hpp PRE-CREATION > 3rdparty/stout/include/stout/os/raw/environment.hpp > b3e82ac8071b41748aeb098b7d5fcc210a1d3c43 > 3rdparty/stout/include/stout/os/windows/environment.hpp PRE-CREATION > 3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 > > Diff: https://reviews.apache.org/r/55547/diff/ > > > Testing > ------- > > > Thanks, > > Alex Clemmer > >
