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