You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2017/01/15 21:44:11 UTC
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/
-----------------------------------------------------------
(Updated Jan. 15, 2017, 9:44 p.m.)
Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
Changes
-------
Updated typos I made copying from my Windows box. :(
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 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
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review165625
-----------------------------------------------------------
3rdparty/stout/include/stout/os/windows/environment.hpp (line 27)
<https://reviews.apache.org/r/55547/#comment237490>
Oh, this variable is apparently un-used. I can remove it before committing.
- Joseph Wu
On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:47 a.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 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
>
>
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review165576
-----------------------------------------------------------
Ship it!
A few minor nits below that I will fix before committing.
3rdparty/stout/include/stout/os/raw/environment.hpp (line 36)
<https://reviews.apache.org/r/55547/#comment237428>
I think the capitalization of `win32` should be `Win32`. That's how it appears in the MSDN docs :)
3rdparty/stout/include/stout/os/raw/environment.hpp (line 82)
<https://reviews.apache.org/r/55547/#comment237429>
Even if the condition is `ifndef`, we don't add an exclamation in the comment. The only exception is if we have a block like:
```
#if !defined(__WINDOWS__) || defined(__linux__)
```
In that case, the ending comment would be:
```
#endif // !__WINDOWS || __linux__
```
3rdparty/stout/include/stout/os/raw/environment.hpp (line 104)
<https://reviews.apache.org/r/55547/#comment237430>
Similar here.
3rdparty/stout/include/stout/os/windows/environment.hpp (line 24)
<https://reviews.apache.org/r/55547/#comment237432>
For code-reading sanity, I'm going to add this comment block:
```
// NOTE: The `GetEnvironmentStrings` call returns a pointer to a
// read-only block of memory with the following format (minus newlines):
// Var1=Value1\0
// Var2=Value2\0
// Var3=Value3\0
// ...
// VarN=ValueN\0\0
```
3rdparty/stout/include/stout/os/windows/environment.hpp (line 28)
<https://reviews.apache.org/r/55547/#comment237431>
Missing space after the `for`
3rdparty/stout/include/stout/os/windows/environment.hpp (line 30)
<https://reviews.apache.org/r/55547/#comment237433>
For code-reading sanity:
// Increment past the current environment string and null terminator.
- Joseph Wu
On Feb. 14, 2017, 10:47 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55547/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2017, 10:47 a.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 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
>
>
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
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
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Alex Clemmer <cl...@gmail.com>.
> On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/raw/environment.hpp, lines 108-120
> > <https://reviews.apache.org/r/55547/diff/2/?file=1605587#file1605587line108>
> >
> > 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
Yes, we should. This will probably happen when we start to transition away from the hand-rolled windows task launching in the Command Executor.
It probably is out of scope for this review, though, much as I'd like to do it now.
> On Jan. 24, 2017, 2:23 a.m., Joseph Wu wrote:
> > 3rdparty/stout/include/stout/os/windows/environment.hpp, lines 22-40
> > <https://reviews.apache.org/r/55547/diff/2/?file=1605588#file1605588line22>
> >
> > 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
Per our discussion, we have agreed to clean this up later as part of a refactoring of subprocess.
- Alex
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55547/#review162754
-----------------------------------------------------------
On Feb. 14, 2017, 6:47 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> 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.
>
>
> 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 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
>
>
Re: Review Request 55547: Windows: Standardize on win32 environment,
transition away from CRT.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
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
>
>