----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55547/ -----------------------------------------------------------
(Updated Feb. 14, 2017, 6:47 p.m.) Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu. Changes ------- Address Joseph's comments. 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 (updated) ----- 3rdparty/stout/include/Makefile.am 4bde2ef3f466ed91c6922b330f07f5d425398751 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 fc359159afcdb60e4406821e123b6358320b6df8 Diff: https://reviews.apache.org/r/55547/diff/ Testing ------- Thanks, Alex Clemmer