You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Kapil Arya <ka...@mesosphere.io> on 2014/09/18 00:50:57 UTC

Review Request 25755: Replace os::Release with Version.

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

Review request for mesos.


Repository: mesos-git


Description
-------

Version is now the preferred way for managing releases/versions.


Diffs
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
  src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 

Diff: https://reviews.apache.org/r/25755/diff/


Testing
-------

Ran make check


Thanks,

Kapil Arya

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/#review53764
-----------------------------------------------------------

Ship it!


Looks great! Thanks for following up.

I will split this patch for you across stout and mesos this time. We split our reviews across stout/libprocess/mesos because the former two are still considered separate libraries. :)

- Ben Mahler


On Sept. 17, 2014, 10:50 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 10:50 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
>   src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ben Mahler <be...@gmail.com>.

> On Sept. 17, 2014, 11:20 p.m., Ben Mahler wrote:
> > Whoops, looks like the test fails for me. Since we need to fix it, can you split the patch while you're at it?
> > 
> > I think the parsing needs to be more advanced to handle OSes with labels on the versions.
> 
> Kapil Arya wrote:
>     Not just labels, but also more than three components.  Should we change the Version to support arbitrary number of components before pushing this change through?  In this particular case, the version string has four components "2.6.44-LABEL.el5" and the current parser will fail even if there were no labels.

Yeah, longer term probably we should handle everything allowed by semantic versioning: http://semver.org/

I think the easiest thing for now is to just keep the sscanf in os.hpp and construct the Version with the parsed integers. That way, we can get this committed with a TODO on Version for parsing labels and build metadata.


- Ben


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


On Sept. 17, 2014, 10:50 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 10:50 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
>   src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Kapil Arya <ka...@mesosphere.io>.

> On Sept. 17, 2014, 7:20 p.m., Ben Mahler wrote:
> > Whoops, looks like the test fails for me. Since we need to fix it, can you split the patch while you're at it?
> > 
> > I think the parsing needs to be more advanced to handle OSes with labels on the versions.

Not just labels, but also more than three components.  Should we change the Version to support arbitrary number of components before pushing this change through?  In this particular case, the version string has four components "2.6.44-LABEL.el5" and the current parser will fail even if there were no labels.


- Kapil


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


On Sept. 17, 2014, 6:50 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 6:50 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
>   src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/#review53766
-----------------------------------------------------------


Whoops, looks like the test fails for me. Since we need to fix it, can you split the patch while you're at it?

I think the parsing needs to be more advanced to handle OSes with labels on the versions.


3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp
<https://reviews.apache.org/r/25755/#comment93576>

    This test fails on my machine:
    
    [ RUN      ] OsTest.release
    ../../../../3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp:270: Failure
    info: Version string has 4 components; maximum 3 components allowed
    
    $ uname -a
    <snip> 2.6.44-LABEL.el5 #1 SMP Tue May 13 15:46:59 PDT 2014 x86_64 x86_64 x86_64 GNU/Linux


- Ben Mahler


On Sept. 17, 2014, 10:50 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 10:50 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
>   src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/#review53875
-----------------------------------------------------------

Ship it!


Ship It!

- Ben Mahler


On Sept. 18, 2014, 3:04 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2014, 3:04 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 2a2feadd4513dead583e9e40ef31b3c57179fe59 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ben Mahler <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/#review53877
-----------------------------------------------------------



3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp
<https://reviews.apache.org/r/25755/#comment93738>

    I'll add a newline here before I commit, since it was lost during your change.


- Ben Mahler


On Sept. 18, 2014, 3:04 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 18, 2014, 3:04 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 2a2feadd4513dead583e9e40ef31b3c57179fe59 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 25755: Replace os::Release with Version.

Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/
-----------------------------------------------------------

(Updated Sept. 18, 2014, 11:04 a.m.)


Review request for mesos.


Repository: mesos-git


Description
-------

Version is now the preferred way for managing releases/versions.


Diffs (updated)
-----

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
  3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 2a2feadd4513dead583e9e40ef31b3c57179fe59 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 

Diff: https://reviews.apache.org/r/25755/diff/


Testing
-------

Ran make check


Thanks,

Kapil Arya

Re: Review Request 25755: Replace os::Release with Version.

Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25755/#review53763
-----------------------------------------------------------

Ship it!


Ship It!

- Ian Downes


On Sept. 17, 2014, 3:50 p.m., Kapil Arya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25755/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2014, 3:50 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Version is now the preferred way for managing releases/versions.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 02a1e5842110f87cb12a917e5dc5ccadb54faacb 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 84ef49c410a08d60f77131e05e8ecfc5ed825f38 
>   src/linux/perf.cpp 1a851a11abb1292eeb2267ed679c68fbb5aa8290 
> 
> Diff: https://reviews.apache.org/r/25755/diff/
> 
> 
> Testing
> -------
> 
> Ran make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>