Re: Review Request 55037: Added default PATH value in `launch.cpp` for Windows case.

[Joseph Wu <jo...@mesosphere.io>]

> On Dec. 27, 2016, 6:12 a.m., Till Toenshoff wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 148
> > <https://reviews.apache.org/r/55037/diff/1/?file=1592009#file1592009line148>
> >
> >     By not adding `syswow64` we are excluding 32bit runnables, is this intentional and documented? Is this still a thing on windows?

Actually, in the current implementation of subprocess on Windows, any user-specified environment variables that conflict with system environment variables will be overridden at subprocess launch.  This means you can specify a `PATH=nowhere` and we'll just ignore it.

At some point, we may loosen this behavior, as we only did this because the test coverage of the Windows code at the time was nil, and a bunch of other changes kept breaking the environment variables on Windows :)


- Joseph


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55037/#review160161
-----------------------------------------------------------


On Dec. 26, 2016, 1:53 a.m., Alex Clemmer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55037/
> -----------------------------------------------------------
> 
> (Updated Dec. 26, 2016, 1:53 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, John Kordich, and Joseph Wu.
> 
> 
> Bugs: MESOS-6839
>     https://issues.apache.org/jira/browse/MESOS-6839
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Currently, when the containerizer launches a process with the POSIX
> launcher, it will check if the `$PATH` environment variable (or `%PATH%`
> in the case of Windows is set in the `LaunchInfo`. If it is not, we
> supply a default path. Unfortunately, this default path is specific to
> POSIX. In many of our tests, this causes many of our tests to be unable
> to find Windows-standard executables like `ping`, and subsequently fail.
> 
> This commit will introduce a function, `default_path` that returns a
> sensible default path for both POSIX and Windows. Since the Windows
> implementation of this depends on the configuration of the host running
> the containerizer (rather than, say, the one creating the `TaskInfo`),
> we choose to implement this in `launch.cpp` instead of Stout, where a
> user could mistakenly call it and expect the same output on all hosts.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/launch.cpp e482ab8bdfc358f695b87cda72ca59fb64cd8c4d 
> 
> Diff: https://reviews.apache.org/r/55037/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>