[Release] Make release scripts aware of packed tags

tools/release/common_includes.py

 #!/usr/bin/env python
 # Copyright 2013 the V8 project authors. All rights reserved.
 # Redistribution and use in source and binary forms, with or without
 # modification, are permitted provided that the following conditions are
 # met:
 #
 #     * Redistributions of source code must retain the above copyright
 #       notice, this list of conditions and the following disclaimer.
 #     * Redistributions in binary form must reproduce the above
 #       copyright notice, this list of conditions and the following
@@ skipping 32 matching lines @@
 import urllib2
 
 from git_recipes import GitRecipesMixin
 from git_recipes import GitFailedException
 
 CHANGELOG_FILE = "ChangeLog"
 DAY_IN_SECONDS = 24 * 60 * 60
 PUSH_MSG_GIT_RE = re.compile(r".* \(based on (?P<git_rev>[a-fA-F0-9]+)\)$")
 PUSH_MSG_NEW_RE = re.compile(r"^Version \d+\.\d+\.\d+$")
 VERSION_FILE = os.path.join("include", "v8-version.h")
-VERSION_RE = re.compile(r"^\d+\.\d+\.\d+(?:\.\d+)?$")
+VERSION_RE_RAW = re.compile(r"^\d+\.\d+\.\d+(?:\.\d+)?$")
+VERSION_RE_TAGS = re.compile(r"^tags\/\d+\.\d+\.\d+(?:\.\d+)?$")
 
 # V8 base directory.
 V8_BASE = os.path.dirname(
     os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 
 
 def TextToFile(text, file_name):
   with open(file_name, "w") as f:
     f.write(text)
 
@@ skipping 536 matching lines @@
 
   # Takes a file containing the patch to apply as first argument.
   def ApplyPatch(self, patch_file, revert=False):
     try:
       self.GitApplyPatch(patch_file, revert)
     except GitFailedException:
       self.WaitForResolvingConflicts(patch_file)
 
   def GetVersionTag(self, revision):
     tag = self.Git("describe --tags %s" % revision).strip()
-    if VERSION_RE.match(tag):
-      return tag
-    else:
-      return None
+    return self._SanitizeVersionTag(tag)
 
   def GetRecentReleases(self, max_age):
     # Make sure tags are fetched.
     self.Git("fetch origin +refs/tags/*:refs/tags/*")
 
     # Current timestamp.
     time_now = int(self._side_effect_handler.GetUTCStamp())
 
     # List every tag from a given period.
     revisions = self.Git("rev-list --max-age=%d --tags" %
                          int(time_now - max_age)).strip()
 
     # Filter out revisions who's tag is off by one or more commits.
     return filter(lambda r: self.GetVersionTag(r), revisions.splitlines())
 
   def GetLatestVersion(self):
     # Use cached version if available.
     if self["latest_version"]:
       return self["latest_version"]
 
     # Make sure tags are fetched.
     self.Git("fetch origin +refs/tags/*:refs/tags/*")
-    version = sorted(filter(VERSION_RE.match, self.vc.GetTags()),
+
+    all_tags = self.vc.GetTags()
+    only_version_tags = []
+
+    # Remove tags/ prefix because of packed refs.
+    for current_tag in all_tags:
+      version_tag = self._SanitizeVersionTag(current_tag)
+      if version_tag != None:
+        only_version_tags.append(version_tag)
+
+    version = sorted(only_version_tags,
                      key=SortingKey, reverse=True)[0]
     self["latest_version"] = version
     return version
 
   def GetLatestRelease(self):
     """The latest release is the git hash of the latest tagged version.
 
     This revision should be rolled into chromium.
     """
     latest_version = self.GetLatestVersion()
@@ skipping 58 matching lines @@
       elif line.startswith("#define V8_BUILD_NUMBER"):
         line = re.sub("\d+$", self[prefix + "build"], line)
       elif line.startswith("#define V8_PATCH_LEVEL"):
         line = re.sub("\d+$", self[prefix + "patch"], line)
       elif (self[prefix + "candidate"] and
             line.startswith("#define V8_IS_CANDIDATE_VERSION")):
         line = re.sub("\d+$", self[prefix + "candidate"], line)
       output += "%s\n" % line
     TextToFile(output, version_file)
 
+  def _SanitizeVersionTag(self, tag):

[Michael Achenbach, 2015/11/05 13:08:38]

I'd prefer to just do this:

if tag.startswith('tags/'):
  return tag[len('tags/'):]
else:
  return tag

Then you can leave most of the other code as it is. Just prefix every use of
tags with _SanitizeVersionTag. And lists with map(_SanitizeVersionTag, tags)
WDYT?

Maybe make the method static to not get trouble with self.

[Michael Hablich, 2015/11/05 13:31:54]

I still need to check and filter for valid version tags => two loops instead of
one. With the current solution the validation does not need to be done in every
method.

I prefer leaving this as a private method because that is the current scope the
method is handling. If more is needed later on this can simply be refactored.
What do you mean with 'trouble with self'?

[Michael Achenbach, 2015/11/05 14:07:55]

I generally find functional things easier to read. E.g.:

map(_SanitizeVersionTag, tags)
-> a list of sanitized tags

filter(VERSION_RE.match, map(_SanitizeVersionTag, tags))
-> a list of sanitized tags that are versions

Code that has a local list and appends things to it under certain conditions is
harder to read IMO as you need to keep your eyes on different things.

Trouble with self: You might not be able to map a member function like above.
Better have it static, i.e. without self parameter. Or toplevel. The method
doesn't need to access self anyways.

[Michael Hablich, 2015/11/05 14:38:21]

Well, I converted the method to a function so it can be properly tested.

I disagree with 'map(_SanitizeVersionTag, tags)'. For me this is not easier to
read than 'only_version_tags = NormalizeVersionTags(all_tags)'. And it still
needs to loop two times over the list. See latest patchset.

+    if VERSION_RE_RAW.match(tag):
+      return tag
+    elif VERSION_RE_TAGS.match(tag):
+        return tag[5:]

[Michael Achenbach, 2015/11/05 13:08:38]

nit tag[len('tags/'):] might make the intention clearer here.

[Michael Hablich, 2015/11/05 13:31:54]

Acknowledged.

+    else:
+      return None
+
 
 class BootstrapStep(Step):
   MESSAGE = "Bootstapping v8 checkout."
 
   def RunStep(self):
     if os.path.realpath(self.default_cwd) == os.path.realpath(V8_BASE):
       self.Die("Can't use v8 checkout with calling script as work checkout.")
     # Directory containing the working v8 checkout.
     if not os.path.exists(self._options.work_dir):
       os.makedirs(self._options.work_dir)
@@ skipping 147 matching lines @@
     for (number, step_class) in enumerate([BootstrapStep] + step_classes):
       steps.append(MakeStep(step_class, number, self._state, self._config,
                             options, self._side_effect_handler))
     for step in steps[options.step:]:
       if step.Run():
         return 0
     return 0
 
   def Run(self, args=None):
     return self.RunSteps(self._Steps(), args)