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