You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2018/05/17 19:03:29 UTC
Review Request 67188: Windows: Implemented `os::shell` and enabled
tests.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67188/
-----------------------------------------------------------
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Bugs: MESOS-8931
https://issues.apache.org/jira/browse/MESOS-8931
Repository: mesos
Description
-------
Once implemented, the tests had to be fixed to use Windows commands,
and a test that ensures `os::shell` doesn't hang was added. The
implementation does not reuse `os::spawn` because it must read all the
child process's piped output before waiting on it to exit, otherwise
it will deadlock.
Diffs
-----
3rdparty/stout/include/stout/os/windows/shell.hpp 8da612af2888ff4d4d458ea5b16cdb08779b6f4c
3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed
Diff: https://reviews.apache.org/r/67188/diff/1/
Testing
-------
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67188: Windows: Implemented `os::shell` and
enabled tests.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On May 18, 2018, 11:07 a.m., John Kordich wrote:
> > 3rdparty/stout/include/stout/os/windows/shell.hpp
> > Lines 383 (patched)
> > <https://reviews.apache.org/r/67188/diff/1/?file=2024839#file2024839line383>
> >
> > Do we need to worry about quote escaping here?
Oh yeah... we do... Will fix.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67188/#review203435
-----------------------------------------------------------
On May 17, 2018, 12:03 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67188/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 12:03 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8931
> https://issues.apache.org/jira/browse/MESOS-8931
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once implemented, the tests had to be fixed to use Windows commands,
> and a test that ensures `os::shell` doesn't hang was added. The
> implementation does not reuse `os::spawn` because it must read all the
> child process's piped output before waiting on it to exit, otherwise
> it will deadlock.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/shell.hpp 8da612af2888ff4d4d458ea5b16cdb08779b6f4c
> 3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed
>
>
> Diff: https://reviews.apache.org/r/67188/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67188: Windows: Implemented `os::shell` and
enabled tests.
Posted by John Kordich via Review Board <no...@reviews.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67188/#review203435
-----------------------------------------------------------
Fix it, then Ship it!
3rdparty/stout/include/stout/os/windows/shell.hpp
Lines 383 (patched)
<https://reviews.apache.org/r/67188/#comment285651>
Do we need to worry about quote escaping here?
- John Kordich
On May 17, 2018, 7:03 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67188/
> -----------------------------------------------------------
>
> (Updated May 17, 2018, 7:03 p.m.)
>
>
> Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
>
>
> Bugs: MESOS-8931
> https://issues.apache.org/jira/browse/MESOS-8931
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Once implemented, the tests had to be fixed to use Windows commands,
> and a test that ensures `os::shell` doesn't hang was added. The
> implementation does not reuse `os::spawn` because it must read all the
> child process's piped output before waiting on it to exit, otherwise
> it will deadlock.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/os/windows/shell.hpp 8da612af2888ff4d4d458ea5b16cdb08779b6f4c
> 3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed
>
>
> Diff: https://reviews.apache.org/r/67188/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 67188: Windows: Implemented `os::shell` and
enabled tests.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67188/
-----------------------------------------------------------
(Updated May 21, 2018, 11 a.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Changes
-------
Added more unit tests.
Bugs: MESOS-8931
https://issues.apache.org/jira/browse/MESOS-8931
Repository: mesos
Description
-------
Once implemented, the tests had to be fixed to use Windows commands,
and a test that ensures `os::shell` doesn't hang was added. The
implementation does not reuse `os::spawn` because it must read all the
child process's piped output before waiting on it to exit, otherwise
it will deadlock.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/shell.hpp 8da612af2888ff4d4d458ea5b16cdb08779b6f4c
3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed
Diff: https://reviews.apache.org/r/67188/diff/3/
Changes: https://reviews.apache.org/r/67188/diff/2-3/
Testing
-------
Tested on Windows with
```
powershell -NoProfile -Command (Get-NetIPAddress -AddressFamily IPv4 -InterfaceAlias 'vEthernet (External)').IPAddress
```
Saved in `ip-command.txt` and then fed through `--ip_discovery_command=file://ip-command.txt`. It is otherwise very difficult to quote things correctly, as it goes through several layers of parsing (e.g. the flags loader tries to parse it as JSON if you use `{}` at all).
Thanks,
Andrew Schwartzmeyer
Re: Review Request 67188: Windows: Implemented `os::shell` and
enabled tests.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/67188/
-----------------------------------------------------------
(Updated May 18, 2018, 4:12 p.m.)
Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and Radhika Jandhyala.
Changes
-------
Used `CommandLineToArgvW` to split the string into discrete arguments instead of the buggy naive `split(" ")`.
Bugs: MESOS-8931
https://issues.apache.org/jira/browse/MESOS-8931
Repository: mesos
Description
-------
Once implemented, the tests had to be fixed to use Windows commands,
and a test that ensures `os::shell` doesn't hang was added. The
implementation does not reuse `os::spawn` because it must read all the
child process's piped output before waiting on it to exit, otherwise
it will deadlock.
Diffs (updated)
-----
3rdparty/stout/include/stout/os/windows/shell.hpp 8da612af2888ff4d4d458ea5b16cdb08779b6f4c
3rdparty/stout/tests/os_tests.cpp 419879a47eb14813b8c6cac8f4a3a3bb0b9e2bed
Diff: https://reviews.apache.org/r/67188/diff/2/
Changes: https://reviews.apache.org/r/67188/diff/1-2/
Testing
-------
Thanks,
Andrew Schwartzmeyer