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/10/25 00:55:47 UTC
Review Request 27175: Added support for handling 'rc' tag to Version.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/
-----------------------------------------------------------
Review request for mesos and Ben Mahler.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos-git
Description
-------
Now it can parse strings like "1.2.3-rc4". Other tags are still
discarded.
Updated os::release() to use Version::parse() instead of sscanf.
Diffs
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 5d3cbba289d8f9d57c2fd49dd6f72225bb2fb8c2
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
Diff: https://reviews.apache.org/r/27175/diff/
Testing
-------
Enhanced version tests and ran make check.
Thanks,
Kapil Arya
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/#review59259
-----------------------------------------------------------
Patch looks great!
Reviews applied: [27175]
All tests passed.
- Mesos ReviewBot
On Oct. 30, 2014, 8:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27175/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 8:32 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now it can parse strings like "1.2.3-rc4". Other tags are still
> discarded.
>
> Updated os::release() to use Version::parse() instead of sscanf.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
> 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
> 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
>
> Diff: https://reviews.apache.org/r/27175/diff/
>
>
> Testing
> -------
>
> Enhanced version tests and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/#review59791
-----------------------------------------------------------
Ship It!
Some misc. comments / notes.
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment101095>
Extra '-' here since you defined the base having -
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment101096>
Ideally this would be a constexpr. As written this is a variable length array sized at runtime (Unless/until the optimizer gets at it). But that is good enough for now.
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment101098>
So this is saying rc2.foobar is acceptble. Basically rc[:digit:] and the like has arbitrary text after it? And rc.5 isn't acceptable?
- Cody Maloney
On Oct. 30, 2014, 8:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27175/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 8:32 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now it can parse strings like "1.2.3-rc4". Other tags are still
> discarded.
>
> Updated os::release() to use Version::parse() instead of sscanf.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
> 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
> 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
>
> Diff: https://reviews.apache.org/r/27175/diff/
>
>
> Testing
> -------
>
> Enhanced version tests and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Kapil Arya <ka...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/
-----------------------------------------------------------
(Updated Oct. 30, 2014, 4:32 p.m.)
Review request for mesos and Ben Mahler.
Changes
-------
Addressed Cody's comments.
Bugs: MESOS-1987
https://issues.apache.org/jira/browse/MESOS-1987
Repository: mesos-git
Description
-------
Now it can parse strings like "1.2.3-rc4". Other tags are still
discarded.
Updated os::release() to use Version::parse() instead of sscanf.
Diffs (updated)
-----
3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
Diff: https://reviews.apache.org/r/27175/diff/
Testing
-------
Enhanced version tests and ran make check.
Thanks,
Kapil Arya
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Kapil Arya <ka...@mesosphere.io>.
> On Oct. 28, 2014, 11:17 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 73
> > <https://reviews.apache.org/r/27175/diff/1/?file=732995#file732995line73>
> >
> > If we go with semantic versioning:
> >
> > Rule 10: "Precedence for two pre-release versions with the same major, minor, and patch version MUST be determined by comparing each dot separated identifier from left to right until a difference is found as follows: identifiers consisting of only digits are compared numerically and identifiers with letters or hyphens are compared lexically in ASCII sort order. Numeric identifiers always have lower precedence than non-numeric identifiers."
> >
> > which this doesn't follow. If we want to pull out the structure of the naming we know / do (rc) then definitely we can/should.
> >
> > According to SemVer we should call this pre-release in general and we would then have a helper in Version which tells you whether or not there is a pre-release.
> >
> > It is likely that I'll use the pre-release versions for tracking mesos testing cluster builds for continuous integration (I can forsee the version string 0.20.1-2014.10.28.3+debug)
I have added a TODO for now. Eventually, it will need a rewrite to get closer to semantic versioning.
> On Oct. 28, 2014, 11:17 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 88
> > <https://reviews.apache.org/r/27175/diff/1/?file=732995#file732995line88>
> >
> > Shouldn't tag = UNKNOWN here?
Right now, if the tag is not "rc", it is considered release. I have added a TODO to extend them in future.
> On Oct. 28, 2014, 11:17 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 118
> > <https://reviews.apache.org/r/27175/diff/1/?file=732995#file732995line118>
> >
> > We really need some form of "more or less equal" / some level of the fields match (major + minor + patch). Most users shouldn't care about tags for checking to enable / disable features. If they are getting that specific day to day development will likely break them.
> >
> > I don't think this should compare tags, but I see the argument either way.
I think instead of saying "more or less equal", one can always say >= "0.21.0" or < "0.20.1" and so on. That kind of gives you the same features (but more accurate) as "more or less equal".
> On Oct. 28, 2014, 11:17 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp, line 141
> > <https://reviews.apache.org/r/27175/diff/1/?file=732995#file732995line141>
> >
> > The tag comparison here is incorrect. If a tag is given at all, that should always be lower level than tag == RELEASE. I'd much rather see that logic laid out here than being implicit in the ordering of the members of the enum.
> >
> > Same goes for operator >
Will do once we add support more labels/tags.
- Kapil
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/#review58954
-----------------------------------------------------------
On Oct. 30, 2014, 4:32 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27175/
> -----------------------------------------------------------
>
> (Updated Oct. 30, 2014, 4:32 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now it can parse strings like "1.2.3-rc4". Other tags are still
> discarded.
>
> Updated os::release() to use Version::parse() instead of sscanf.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp ec259cdee876c64f3e562aa77d4d52e964a173ab
> 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
> 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
>
> Diff: https://reviews.apache.org/r/27175/diff/
>
>
> Testing
> -------
>
> Enhanced version tests and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Cody Maloney <co...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/#review58954
-----------------------------------------------------------
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100180>
These TODO comments should be updted since this adds a 4th component if nothing else
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100184>
labelSplit?
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100185>
version? versionChunks? versionComponents? the name split isn't very helpful.
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100188>
If we go with semantic versioning:
Rule 10: "Precedence for two pre-release versions with the same major, minor, and patch version MUST be determined by comparing each dot separated identifier from left to right until a difference is found as follows: identifiers consisting of only digits are compared numerically and identifiers with letters or hyphens are compared lexically in ASCII sort order. Numeric identifiers always have lower precedence than non-numeric identifiers."
which this doesn't follow. If we want to pull out the structure of the naming we know / do (rc) then definitely we can/should.
According to SemVer we should call this pre-release in general and we would then have a helper in Version which tells you whether or not there is a pre-release.
It is likely that I'll use the pre-release versions for tracking mesos testing cluster builds for continuous integration (I can forsee the version string 0.20.1-2014.10.28.3+debug)
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100189>
Why not strings::lower(secondarySplit[0]) so that these cases condense?
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100190>
Shouldn't tag = UNKNOWN here?
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100191>
shouldn't the versionString contain the tag if one is specified?
Why isn't not passing in the original version string a programmer error / assertion failure?
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100192>
We really need some form of "more or less equal" / some level of the fields match (major + minor + patch). Most users shouldn't care about tags for checking to enable / disable features. If they are getting that specific day to day development will likely break them.
I don't think this should compare tags, but I see the argument either way.
3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp
<https://reviews.apache.org/r/27175/#comment100194>
The tag comparison here is incorrect. If a tag is given at all, that should always be lower level than tag == RELEASE. I'd much rather see that logic laid out here than being implicit in the ordering of the members of the enum.
Same goes for operator >
3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp
<https://reviews.apache.org/r/27175/#comment100195>
Can you add a test for release > rc/unknown tag?
- Cody Maloney
On Oct. 24, 2014, 10:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27175/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 10:55 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now it can parse strings like "1.2.3-rc4". Other tags are still
> discarded.
>
> Updated os::release() to use Version::parse() instead of sscanf.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 5d3cbba289d8f9d57c2fd49dd6f72225bb2fb8c2
> 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
> 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
>
> Diff: https://reviews.apache.org/r/27175/diff/
>
>
> Testing
> -------
>
> Enhanced version tests and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>
Re: Review Request 27175: Added support for handling 'rc' tag to
Version.
Posted by Mesos ReviewBot <de...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27175/#review58472
-----------------------------------------------------------
Patch looks great!
Reviews applied: [27175]
All tests passed.
- Mesos ReviewBot
On Oct. 24, 2014, 10:55 p.m., Kapil Arya wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27175/
> -----------------------------------------------------------
>
> (Updated Oct. 24, 2014, 10:55 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-1987
> https://issues.apache.org/jira/browse/MESOS-1987
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> Now it can parse strings like "1.2.3-rc4". Other tags are still
> discarded.
>
> Updated os::release() to use Version::parse() instead of sscanf.
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 5d3cbba289d8f9d57c2fd49dd6f72225bb2fb8c2
> 3rdparty/libprocess/3rdparty/stout/include/stout/version.hpp 090fcf09dd96538a8748cf4443d150911e2c0d27
> 3rdparty/libprocess/3rdparty/stout/tests/version_tests.cpp e8f8358f3c113b4e21e30cb5e9d6a3d229191484
>
> Diff: https://reviews.apache.org/r/27175/diff/
>
>
> Testing
> -------
>
> Enhanced version tests and ran make check.
>
>
> Thanks,
>
> Kapil Arya
>
>