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

Reply via email to