[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

Review URL: https://codereview.chromium.org/506863005

Cr-Commit-Position: refs/heads/master@{#296618}

native_client_sdk/src/tools/common.mk

@@ -204,6 +204,9 @@ TC_PATH := $(abspath $(NACL_SDK_ROOT)/toolchain)
 
 #
 # Check for required minimum SDK version.
+# A makefile can declare NACL_SDK_VERSION_MIN of the form "<major>.<position>",
+# where <major> is the major Chromium version number, and <position> is the
+# Chromium Cr-Commit-Position number. eg. "39.295386".
 #
 ifdef NACL_SDK_VERSION_MIN
   VERSION_CHECK:=$(shell $(GETOS) --check-version=$(NACL_SDK_VERSION_MIN) 2>&1)

native_client_sdk/src/tools/getos.py

@@ -84,6 +84,7 @@ def GetSDKVersion():
 
   try:
     version = int(version)
+    revision = int(revision)
   except ValueError:
     raise Error("error parsing SDK README: %s" % readme)
 
@@ -183,6 +184,10 @@ def GetNaClArch(platform):
 
 
 def ParseVersion(version):
+  """Parses a version number of the form '<major>.<position>'.
+
+  <position> is the Cr-Commit-Position number.
+  """
   if '.' in version:
     version = version.split('.')
   else:
@@ -194,6 +199,22 @@ def ParseVersion(version):
     raise Error('error parsing SDK version: %s' % version)
 
 
+def CheckVersion(required_version):
+  """Determines whether the current SDK version meets the required version.
+
+  Args:
+    required_version: (major, position) pair, where position is the
+    Cr-Commit-Position number.
+
+  Raises:
+    Error: The SDK version is older than required_version.
+  """
+  version = GetSDKVersion()[:2]
+  if version < required_version:
+    raise Error("SDK version too old (current: %d.%d, required: %d.%d)"
+           % (version[0], version[1], required_version[0], required_version[1]))
+
+
 def main(args):
   parser = optparse.OptionParser()
   parser.add_option('--arch', action='store_true',
@@ -210,8 +231,10 @@ def main(args):
   parser.add_option('--sdk-commit-position', action='store_true',
       help='Print commit position of the NaCl SDK.')
   parser.add_option('--check-version',
+      metavar='MAJOR.POSITION',
       help='Check that the SDK version is at least as great as the '
-           'version passed in.')
+           'version passed in. MAJOR is the major version number and POSITION '
+           'is the Cr-Commit-Position number.')
 
   options, _ = parser.parse_args(args)
 
@@ -238,15 +261,7 @@ def main(args):
     out = GetSDKVersion()[2]
   elif options.check_version:
     required_version = ParseVersion(options.check_version)
-    version = GetSDKVersion()
-    # We currently ignore the revision and just check the major version number.
-    # Currently, version[1] is just a Git hash, which cannot be compared.
-    # TODO(mgiuca): Compare the minor revision numbers (which should be
-    # Cr-Commit-Position values), when http://crbug.com/406783 is fixed.
-    # Then Cr-Commit-Position should be available: see http://crbug.com/406993.
-    if version[0] < required_version[0]:
-      raise Error("SDK version too old (current: %s, required: %s)"
-             % (version[0], required_version[0]))
+    CheckVersion(required_version)
     out = None
 
   if out:

native_client_sdk/src/tools/tests/getos_test.py

@@ -117,15 +117,51 @@ class TestGetosWithTempdir(TestCaseExtended):
 
   def testGetSDKVersion(self):
     """correctly parses README to find SDK version."""
-    expected_version = (16, '196', 'f00baacabba6e-refs/heads/master@{#100}')
+    expected_version = (16, 100, 'f00baacabba6e-refs/heads/master@{#100}')
     with open(os.path.join(self.tempdir, 'README'), 'w') as out:
-      out.write('Version: %s\n' % expected_version[0])
-      out.write('Chrome Revision: %s\n' % expected_version[1])
+      out.write('Version: %d\n' % expected_version[0])
+      out.write('Chrome Revision: %d\n' % expected_version[1])
       out.write('Chrome Commit Position: %s\n' % expected_version[2])
 
     version = getos.GetSDKVersion()
     self.assertEqual(version, expected_version)
 
+  def testParseVersion(self):
+    """correctly parses a version given to --check-version."""
+    check_version_string = '15.100'
+    self.assertEquals((15, 100), getos.ParseVersion(check_version_string))
+
+  def testCheckVersion(self):
+    """correctly rejects SDK versions earlier than the required one."""
+    actual_version = (16, 100, 'f00baacabba6e-refs/heads/master@{#100}')
+    with open(os.path.join(self.tempdir, 'README'), 'w') as out:
+      out.write('Version: %d\n' % actual_version[0])
+      out.write('Chrome Revision: %d\n' % actual_version[1])
+      out.write('Chrome Commit Position: %s\n' % actual_version[2])
+
+    required_version = (15, 150)
+    getos.CheckVersion(required_version)
+
+    required_version = (16, 99)
+    getos.CheckVersion(required_version)
+
+    required_version = (16, 100)
+    getos.CheckVersion(required_version)
+
+    required_version = (16, 101)
+    self.assertRaisesRegexp(
+        getos.Error,
+        r'SDK version too old \(current: 16.100, required: 16.101\)',
+        getos.CheckVersion,
+        required_version)
+
+    required_version = (17, 50)
+    self.assertRaisesRegexp(
+        getos.Error,
+        r'SDK version too old \(current: 16.100, required: 17.50\)',
+        getos.CheckVersion,
+        required_version)
+
 
 if __name__ == '__main__':
   unittest.main()