-----------------------------------------------------------
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
