[NaCl SDK] getos.py now checks against the Cr-Commit-Position.

[NaCl SDK] getos.py now checks against the Cr-Commit-Position.

Previously, it only checked the major version number (this was a
regression as a result of switching to Git).

Also abstracted version checking to a testable CheckVersion function,
and added tests for ParseVersion and CheckVersion.

Also added better documentation everywhere about the format of the
--check-version option.

BUG=406993
Committed: https://crrev.com/4466a650fd2fc808860a136dc00431bf82152e68
Cr-Commit-Position: refs/heads/master@{#296618}

[Matt Giuca, 2014-08-27 00:56:39]

mgiuca@chromium.org changed reviewers:
+ binji@chromium.org, sbc@chromium.org

[Matt Giuca, 2014-08-27 00:56:39]

Hey guys,

Follow up to https://codereview.chromium.org/507673003/ to get a bit better
testing on getos.

[binji, 2014-08-27 01:05:02]

https://codereview.chromium.org/506863005/diff/1/native_client_sdk/src/tools/...
File native_client_sdk/src/tools/tests/getos_test.py (right):

https://codereview.chromium.org/506863005/diff/1/native_client_sdk/src/tools/...
native_client_sdk/src/tools/tests/getos_test.py:130: actual_version = (16,
'0a1b2c3d')
See https://codereview.chromium.org/507883003/

I don't think we want to use a git hash. After this lands, we should be able to
use the commit position as if it were an SVN revision.

Thanks for adding a test, though!

[Matt Giuca, 2014-08-27 01:17:50]

https://codereview.chromium.org/506863005/diff/1/native_client_sdk/src/tools/...
File native_client_sdk/src/tools/tests/getos_test.py (right):

https://codereview.chromium.org/506863005/diff/1/native_client_sdk/src/tools/...
native_client_sdk/src/tools/tests/getos_test.py:130: actual_version = (16,
'0a1b2c3d')
Oh OK, cool. I was expecting we would end up with the string of the format:
"Chrome Revision: GITHASH-COMMITPOSITION"

which is what you'd get if you used lastchange after those guys sort out
Cr-Commit-Position (see https://codereview.chromium.org/488733002, which has
since been reverted but will presumably be added back at some point).

So I was planning to parse the commit position out of that and use it in
CheckVersion. I'll wait for your other CL to land and then update these tests to
match.

[Matt Giuca, 2014-09-18 02:59:05]

PTAL (note that this no longer just adds testing, but also fixes the checking
behaviour).

Let me know if you want me to split the refactoring and the checking behaviour
into two CLs (I could do that but I didn't think it was worth it).

[Matt Giuca, 2014-09-22 00:56:44]

Ping.

[Sam Clegg, 2014-09-22 17:34:46]

lgtm

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
File native_client_sdk/src/tools/getos.py (right):

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
native_client_sdk/src/tools/getos.py:207: Cr-Commit-Position number.
I think docstrings use 2-space indentation like the code.

[binji, 2014-09-22 17:40:08]

The only concern I have here is that the "revision" number changes meaning
between trunk and a branch. For example, pepper_canary may have 39.295787. As
soon as 39 is branched, the revision number changes to be the number of commits
since the branch point: 39.1

[Matt Giuca, 2014-09-23 01:07:36]

On 2014/09/22 17:40:08, binji wrote:
> The only concern I have here is that the "revision" number changes meaning
> between trunk and a branch. For example, pepper_canary may have 39.295787. As
> soon as 39 is branched, the revision number changes to be the number of
commits
> since the branch point: 39.1

Hmm yeah I think this is a valid point (though I haven't tested it). Is there
any reason why we need to be able to check a minor version?

Perhaps it's best to only allow checking a major version. If you require
something after a particular point release (eg. you absolutely need 39.295787
and version 39.293123 will be broken), then the only people who are going to
break are people who are on an unstable release and haven't updated. Those
people will just have to suffer with broken functionality until they update,
which will be soon. There should never be any changes to a stable release that
are significant enough to mean the difference between a particular NaCl module
being supported or unsupported.

Thoughts? (If we agree to only check the major version, then I'll still go ahead
with this patch, but just as a cleanup / adding tests, not changing
functionality.)

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
File native_client_sdk/src/tools/getos.py (right):

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
native_client_sdk/src/tools/getos.py:207: Cr-Commit-Position number.
Yeah you're right. (I was looking at the style guide which has 4-space indents,
but that also has 4-space indents in the code. If the code is 2-space then so
should the docs be.)

[Sam Clegg, 2014-09-23 01:11:24]

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
File native_client_sdk/src/tools/getos.py (right):

https://codereview.chromium.org/506863005/diff/40001/native_client_sdk/src/to...
native_client_sdk/src/tools/getos.py:207: Cr-Commit-Position number.
On 2014/09/23 01:07:36, Matt Giuca wrote:
> Yeah you're right. (I was looking at the style guide which has 4-space
indents,
> but that also has 4-space indents in the code. If the code is 2-space then so
> should the docs be.)

Yes, the internal and external style guides differ on indentation for both
comments and code (annoying!)

[binji, 2014-09-23 22:55:28]

On 2014/09/23 01:07:36, Matt Giuca wrote:
> On 2014/09/22 17:40:08, binji wrote:
> > The only concern I have here is that the "revision" number changes meaning
> > between trunk and a branch. For example, pepper_canary may have 39.295787.
As
> > soon as 39 is branched, the revision number changes to be the number of
> commits
> > since the branch point: 39.1
> 
> Hmm yeah I think this is a valid point (though I haven't tested it). Is there
> any reason why we need to be able to check a minor version?

I don't think so. Maybe just for trunk builds... but even then I think it may
not be
necessary. Do you need this for naclports, Sam?

[Sam Clegg, 2014-09-23 23:07:36]

On 2014/09/23 22:55:28, binji wrote:
> On 2014/09/23 01:07:36, Matt Giuca wrote:
> > On 2014/09/22 17:40:08, binji wrote:
> > > The only concern I have here is that the "revision" number changes meaning
> > > between trunk and a branch. For example, pepper_canary may have 39.295787.
> As
> > > soon as 39 is branched, the revision number changes to be the number of
> > commits
> > > since the branch point: 39.1
> > 
> > Hmm yeah I think this is a valid point (though I haven't tested it). Is
there
> > any reason why we need to be able to check a minor version?
> 
> I don't think so. Maybe just for trunk builds... but even then I think it may
> not be
> necessary. Do you need this for naclports, Sam?

I can imagine it being nice to be able to require a certain revision.  I can't
remember the last time I used this though.

[Matt Giuca, 2014-09-24 08:11:08]

So can we have a decision?

[Sam Clegg, 2014-09-24 16:47:05]

On 2014/09/24 08:11:08, Matt Giuca wrote:
> So can we have a decision?

Seems to be that this is a good change either way.  We can add more information
to version (such as branch number) later if if we need it.

[binji, 2014-09-24 16:58:05]

OK, lgtm

[Matt Giuca, 2014-09-25 00:14:34]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-09-25 00:15:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/506863005/60001

[commit-bot: I haz the power, 2014-09-25 01:49:11]

Committed patchset #4 (id:60001) as dd750112e99a42c009e23b8262686faf0940298e

[commit-bot: I haz the power, 2014-09-25 01:49:50]

Patchset 4 (id:??) landed as
https://crrev.com/4466a650fd2fc808860a136dc00431bf82152e68
Cr-Commit-Position: refs/heads/master@{#296618}