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 2017/06/07 19:15:05 UTC

Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

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

Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.


Bugs: MESOS-7638
    https://issues.apache.org/jira/browse/MESOS-7638


Repository: mesos


Description
-------

This fixes tests that pass but emit error messages due to `false` not
existing as a command on Windows.


Diffs
-----

  src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 


Diff: https://reviews.apache.org/r/59891/diff/1/


Testing
-------

Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.


Thanks,

Andrew Schwartzmeyer

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.

> On June 7, 2017, 7:39 p.m., Michael Park wrote:
> > src/tests/health_check_tests.cpp
> > Lines 46-48 (patched)
> > <https://reviews.apache.org/r/59891/diff/1/?file=1744014#file1744014line46>
> >
> >     Do we need a macro here?
> >     
> >     Can we just do it inline? Something like:
> >     
> >     ```cpp
> >     const char *falseCommand =
> >     #ifndef __WINDOWS__
> >       "false"
> >     #else
> >       "cmd /C exit 1"
> >     #endif
> >       ;
> >       
> >       // The health check for this task will always fail, but the grace period of
> >       // 9999 seconds should mask the failures.
> >       vector<TaskInfo> tasks = populateTasks(
> >         SLEEP_COMMAND(2), falseCommand, offers.get()[0], 9999);
> >     ```

Almost did it that way, wasn't sure if it'd be the right style. Will change.


- Andrew


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


On June 7, 2017, 7:15 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59891/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 7:15 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7638
>     https://issues.apache.org/jira/browse/MESOS-7638
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fixes tests that pass but emit error messages due to `false` not
> existing as a command on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59891/diff/1/
> 
> 
> Testing
> -------
> 
> Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Michael Park <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59891/#review177210
-----------------------------------------------------------




src/tests/health_check_tests.cpp
Lines 46-48 (patched)
<https://reviews.apache.org/r/59891/#comment250758>

    Do we need a macro here?
    
    Can we just do it inline? Something like:
    
    ```cpp
    const char *falseCommand =
    #ifndef __WINDOWS__
      "false"
    #else
      "cmd /C exit 1"
    #endif
      ;
      
      // The health check for this task will always fail, but the grace period of
      // 9999 seconds should mask the failures.
      vector<TaskInfo> tasks = populateTasks(
        SLEEP_COMMAND(2), falseCommand, offers.get()[0], 9999);
    ```


- Michael Park


On June 7, 2017, 12:15 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59891/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 12:15 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7638
>     https://issues.apache.org/jira/browse/MESOS-7638
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fixes tests that pass but emit error messages due to `false` not
> existing as a command on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59891/diff/1/
> 
> 
> Testing
> -------
> 
> Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59891/#review178554
-----------------------------------------------------------


Ship it!




Ship It!

- Joseph Wu


On June 7, 2017, 12:50 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59891/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 12:50 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7638
>     https://issues.apache.org/jira/browse/MESOS-7638
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fixes tests that pass but emit error messages due to `false` not
> existing as a command on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59891/diff/2/
> 
> 
> Testing
> -------
> 
> Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Mesos Reviewbot Windows <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59891/#review177220
-----------------------------------------------------------



Patch looks great!

Reviews applied: [59891]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 7, 2017, 7:50 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59891/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 7:50 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7638
>     https://issues.apache.org/jira/browse/MESOS-7638
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fixes tests that pass but emit error messages due to `false` not
> existing as a command on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59891/diff/2/
> 
> 
> Testing
> -------
> 
> Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59891/#review177397
-----------------------------------------------------------



Patch looks great!

Reviews applied: [59891]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On June 7, 2017, 12:50 p.m., Andrew Schwartzmeyer wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59891/
> -----------------------------------------------------------
> 
> (Updated June 7, 2017, 12:50 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: MESOS-7638
>     https://issues.apache.org/jira/browse/MESOS-7638
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This fixes tests that pass but emit error messages due to `false` not
> existing as a command on Windows.
> 
> 
> Diffs
> -----
> 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59891/diff/2/
> 
> 
> Testing
> -------
> 
> Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 59891: Windows: Use `cmd /C exit 1` instead of `false`.

Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59891/
-----------------------------------------------------------

(Updated June 7, 2017, 7:50 p.m.)


Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.


Changes
-------

Change style.


Bugs: MESOS-7638
    https://issues.apache.org/jira/browse/MESOS-7638


Repository: mesos


Description
-------

This fixes tests that pass but emit error messages due to `false` not
existing as a command on Windows.


Diffs (updated)
-----

  src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 


Diff: https://reviews.apache.org/r/59891/diff/2/

Changes: https://reviews.apache.org/r/59891/diff/1-2/


Testing
-------

Trying to split out fixes I made in my fix-long-paths branch that can be applied sooner, so this has been tested quite thoroughly but with other patches.


Thanks,

Andrew Schwartzmeyer