Issue 4540 - Add Platform Specific Branch Support to createNightlies.py

sitescripts/extensions/bin/createNightlies.py

 # This file is part of the Adblock Plus web scripts,
 # Copyright (C) 2006-2016 Eyeo GmbH
 #
 # Adblock Plus is free software: you can redistribute it and/or modify
 # it under the terms of the GNU General Public License version 3 as
 # published by the Free Software Foundation.
 #
 # Adblock Plus is distributed in the hope that it will be useful,
 # but WITHOUT ANY WARRANTY; without even the implied warranty of
 # MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 47 matching lines @@
       Performs the build process for an extension,
       generating changelogs and documentation.
     """
 
     def __init__(self, config):
         """
           Creates a NightlyBuild instance; we are simply
           recording the configuration settings here.
         """
         self.config = config
-        try:
-            self.branch = self.config.get('extensions', 'abp_{}_branch'.format(

[Vasily Kuznetsov, 2016/10/20 16:54:39]

As we discussed, `xxx_branch` seems nicer than `abp_xxx_branch` but feel free to
leave it like this if there are good reasons.

[kzar, 2016/10/21 08:17:19]

u1. I think bookmark is a better word to use than branch because that's what
we're talking about here. (Please also change the variable name.)
2. The names should be consistent with the other similar variables so
`abpxxx_bookmark.` (Note the lack of underscore after "abp".)

-                self.config.type))

[Jon Sonesen, 2016/10/20 07:55:54]

Not sure exactly how you want to implement the config, weather it is
abp{platform}_branch or just {platform_branch} I am happy to change it.

-        except ConfigParser.NoOptionError:
-            self.branch = 'master'
-
         self.revision = self.getCurrentRevision()
         try:
             self.previousRevision = config.latestRevision
         except:
             self.previousRevision = '0'
         self.buildNum = None
         self.tempdir = None
         self.outputFilename = None
         self.changelogFilename = None
 
     def hasChanges(self):
         return self.revision != self.previousRevision
 
     def getCurrentRevision(self):
         """
             retrieves the current revision ID from the repository
         """
         command = [
-            'hg', 'id', '-i', '-r', 'default', '-b', self.branch, '--config',

[Vasily Kuznetsov, 2016/10/20 16:54:39]

If I understand correctly it should be '-r', self.branch instead of '-r',
'default'. I have not run it though, so not completely sure about this one. Have
you tested it?

[Sebastian Noack, 2016/10/20 17:36:16]

As far as I understand, if we want to use bookmarks (and not actual branches),
the -b option should be omitted, and instead -r should specify the bookmark. But
Wladimir is more an expert for mercurial, if in doubt ask him.

[kzar, 2016/10/21 08:17:19]

I think that's correct, from the issue description: "If no such configuration
option is given, the default should be `master` (as opposed as `default` that we
use right now).".

So it should look like this:

command = [
 'hg', 'id', '-i', '-r', self.bookmark, '--config', 'defaults.id=',
 self.config.repository
]

+            'hg', 'id', '-i', '-r', self.config.revision, '--config',
             'defaults.id=', self.config.repository
         ]
         return subprocess.check_output(command).strip()
 
     def getCurrentBuild(self):
         """
             calculates the (typically numerical) build ID for the current build
         """
-        command = [
-            'hg', 'id', '-b', self.branch, '-n', '--config', 'defaults.id=',

[kzar, 2016/10/21 08:17:19]

Again this should be `'-r', self.bookmark`, but actually I'm not sure we should
even add that. With a command called getCurrentBuild I would expect to get a
build ID for the currently checked out revision, not for some other bookmark.

-            self.tempdir]
+        command = ['hg', 'id', '-n', '--config', 'defaults.id=', self.tempdir]
         build = subprocess.check_output(command).strip()
         if self.config.type == 'gecko':
             build += '-beta'
         return build
 
     def getChanges(self):
         """
           retrieve changes between the current and previous ("first") revision
         """
 
-        command = ['hg', 'log', '-R', self.tempdir, '-r', 'tip:0',
-                   '-b', self.branch, '-l', '50', '--encoding', 'utf-8',

[kzar, 2016/10/21 08:17:19]

I'm not 100% sure but I think this command is returning a log of the last 50
commits from the tip of the repository. I don't think this change makes sense
since we still do want to use the 'default' branch. Also like the
`getCurrentBuild` command above I'm not sure if we need to worry about bookmarks
here either.

[kzar, 2016/10/21 08:22:57]

Actually I think the second part of my comment was wrong. I think we do need to
worry about bookmarks here, I think we need to replace "tip" with the bookmark
name. (Since tip is the current HEAD of the repository, no matter if it's
currently checked out.)

[kzar, 2016/10/25 08:13:00]

Please could you add the `'-b', 'default'` back again, unless you removed it for
a reason?

-                   '--template', '{date|isodate}\\0{author|person}\\0{rev}\\0{desc}\\0\\0',
-                   '--config', 'defaults.log=']
+        command = [
+            'hg', 'log', '-R', self.tempdir, '-r',
+            'ancestors({})'.format(self.config.revision), '-l', '50',
+            '--encoding', 'utf-8', '--template',
+            '{date|isodate}\\0{author|person}\\0{rev}\\0{desc}\\0\\0',
+            '--config', 'defaults.log='
+        ]
         result = subprocess.check_output(command).decode('utf-8')
 
         for change in result.split('\x00\x00'):
             if change:
                 date, author, revision, description = change.split('\x00')
                 yield {'date': date, 'author': author, 'revision': revision, 'description': description}
 
     def copyRepository(self):
         """
           Create a repository copy in a temporary directory
         """
         self.tempdir = tempfile.mkdtemp(prefix=self.config.repositoryName)
-        command = ['hg', 'clone', '-q', self.config.repository, '-b',

[kzar, 2016/10/21 08:17:19]

I don't think change is required at all.

-                   self.branch,  '-u', 'default', self.tempdir]
+        command = ['hg', 'clone', '-q', self.config.repository,  '-u',
+                   self.config.revision, self.tempdir]
         subprocess.check_call(command)
 
         # Make sure to run ensure_dependencies.py if present
         depscript = os.path.join(self.tempdir, 'ensure_dependencies.py')
         if os.path.isfile(depscript):
             subprocess.check_call([sys.executable, depscript, '-q'])
 
     def writeChangelog(self, changes):
         """
           write the changelog file into the cloned repository
@@ skipping 448 matching lines @@
         except Exception as ex:
             logging.error('The build for %s failed:', repo)
             logging.exception(ex)
 
     file = open(nightlyConfigFile, 'wb')
     nightlyConfig.write(file)
 
 
 if __name__ == '__main__':
     main()