Merge lp:~robru/cupstream2distro/detect-new-commits-when-building into lp:cupstream2distro

[Robert Bruce Park]

Detect which packages have new commits to build.

[Robert Bruce Park (robru)]

TODO:

* write tests
* test in staging
* should probably also scan trunk revids
* should also include packages marked dirty.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1168
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/828/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/828/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1170
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/829/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/829/rebuild

review: Approve (continuous-integration)

[Robert Bruce Park (robru)]

Ok I think this is making sense, will just test in staging tomorrow before merging.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1172
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/830/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/830/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1173
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/831/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/831/rebuild

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1174
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-ci/832/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/cu2d-choo-choo-ci/832/rebuild

review: Approve (continuous-integration)

[Robert Bruce Park (robru)]

Staging instance has apparently imploded so I'm not able to test this, but I'm confident with the tests and will monitor production closely for regressions.

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/cu2d-choo-choo-autolanding/310/
Executed test runs:

review: Needs Fixing (continuous-integration)

=== modified file 'citrain/build.py'
--- citrain/build.py	2015-10-17 09:26:56 +0000
+++ citrain/build.py	2015-10-20 09:18:51 +0000
@@ -47,49 +47,30 @@
 
 sys.path.append(os.path.dirname(os.path.dirname(__file__)))
 
-from glob import glob
-from os.path import basename
-
 from citrain.recipes.base import missing
+from citrain.recipes.merge import check_for_unbuilt_revids
 from citrain.recipes.manager import Manager
-from cupstream2distro.silomanager import SiloState
+from cupstream2distro.silomanager import SiloState, get_dirty, splitter
 from cupstream2distro.errors import BuildError, CITrainError
-from cupstream2distro.utils import SILO_DIR, env, run_script
-
-
-def find_all_uploaded():
-    """Return all source package names and versions uploaded to the ppa.
-
-    :yields: Source package names that have been uploaded.
-    """
-    for uploaded in glob(SILO_DIR('*_source.ppa.upload')):
-        yield basename(uploaded).split('_')[0]
+from cupstream2distro.utils import env, run_script
 
 
 class BuildManager(Manager):
     """Honor PACKAGES_TO_REBUILD during building."""
 
-    def include_all(self):
-        """Ignore PACKAGES_TO_REBUILD and include all."""
-        self.choose_packages = super().choose_packages
-        self.types.clear()
-
     def choose_packages(self):
         """Decide which packages need to be built."""
-        self.names = set(env.PACKAGES_TO_REBUILD.split()
-                         or self.silo_state.all_projects)
+        all_projects = set(self.silo_state.all_projects)
+        self.names = set(splitter(env.PACKAGES_TO_REBUILD) or all_projects)
         self.names.update(missing(self.names))
         include_all = 'true' in (env.FORCE_REBUILD, env.WATCH_ONLY)
         if not (include_all or env.PACKAGES_TO_REBUILD):
-            # Don't rebuild existing builds without forcing.
-            previous = self.names & set(find_all_uploaded())
-            if previous:
-                logging.info('{} already built, skipping.'.format(
-                    ', '.join(previous)))
-                self.names -= previous
-        if self.names:
-            logging.info('Including {}.'.format(', '.join(self.names)))
-        else:
+            for source, merges in self.silo_state.mps.items():
+                if check_for_unbuilt_revids(source, merges) == []:
+                    logging.info(source + ' has no new commits, skipping.')
+                    self.names.discard(source)
+            self.names.update(set(get_dirty(env.SILONAME)) & all_projects)
+        if not self.names:
             raise BuildError(
                 'No packages are being considered. You probably want one of '
                 'PACKAGES_TO_REBUILD, FORCE_REBUILD, or WATCH_ONLY.')
@@ -106,8 +87,7 @@
         manager.do('validate')
         if env.WATCH_ONLY != 'true':
             manager.do('clean', 'collect', 'build', 'upload')
-        manager.include_all()
-        manager.do('watch', 'diff')
+        Manager(silo_state).do('watch', 'diff')
         silo_state.status = 'Packages built.'
     except CITrainError as err:
         logging.error(str(err))
 
=== modified file 'citrain/recipes/manager.py'
--- citrain/recipes/manager.py	2015-10-07 19:47:09 +0000
+++ citrain/recipes/manager.py	2015-10-20 09:18:51 +0000
@@ -54,6 +54,7 @@
             self.choose_packages()
             self.choose_classes()
             self.instantiate_build_objects()
+            logging.info('Including {}.'.format(', '.join(sorted(self.names))))
         for phase in phases:
             self.execute_phase(phase)
 
@@ -64,7 +65,6 @@
     def choose_packages(self):
         """Act on all silo packages."""
         self.names = set(self.silo_state.all_projects)
-        logging.info('Including {}.'.format(', '.join(self.names)))
 
     def choose_classes(self):
         """Decide which class to use for each individual package build."""
 
=== modified file 'citrain/recipes/merge.py'
--- citrain/recipes/merge.py	2015-10-19 22:13:22 +0000
+++ citrain/recipes/merge.py	2015-10-20 09:18:51 +0000
@@ -77,6 +77,21 @@
         raise PrepError('Not all MPs targeting same branch.')
 
 
+def check_for_unbuilt_revids(source, merges):
+    """Return a list of MPs that have new commits since last build."""
+    unbuilt = []
+    logging.info('Checking {} for new commits...'.format(source))
+    # TODO: check also the merge target revids, which only just started being
+    # recorded and need time to appear in production.
+    for merge in merges:
+        url = merge.web_link
+        revid = Rev(source, url).read()
+        if merge.source_branch.last_scanned_id != revid:
+            unbuilt.append(url)
+            logging.info('New commits: ' + url)
+    return unbuilt
+
+
 class Merge(BuildBase):
     """Build a source package from a series of merge proposals.
 
@@ -86,18 +101,6 @@
     all_mps = {}
     merges = None
 
-    def check_for_unbuilt_revids(self):
-        """Return a list of MPs that have new commits since last build."""
-        unbuilt = []
-        logging.info('Checking {} for new commits...'.format(self.name))
-        for merge in self.merges:
-            url = merge.web_link
-            revid = Rev(self.name, url).read()
-            if merge.source_branch.last_scanned_id != revid:
-                unbuilt.append(url)
-                logging.error('New commits: ' + url)
-        return unbuilt
-
     def enforce_merge_states(self, acceptable):
         """Block when merges have bad states."""
         # (\/) (°,,,°) (\/)
@@ -128,8 +131,9 @@
         """
         logging.info('Preparing merges for {}.'.format(self.name))
         merges = self.merges = reorder_branches(self.merges)
-        target = merges[0].target_branch.web_link
-        self.get_branch(target)
+        target = merges[0].target_branch
+        Rev(self.name, target.web_link).write(target.last_scanned_id)
+        self.get_branch(target.web_link)
         trunk_version = self.get_package_version()
         DotProject.soft_save(self.name, dest=trunk_version)
         for merge in merges:
@@ -238,7 +242,7 @@
 
     def unbuilt_phase(self):
         """Block publication of merges that have new commits."""
-        if self.check_for_unbuilt_revids():
+        if check_for_unbuilt_revids(self.name, self.merges):
             self.failures.add(self.name + ' has new, unbuilt commits.')
 
     def merge_phase(self):
 
=== modified file 'tests/unit/__init__.py'
--- tests/unit/__init__.py	2015-09-22 06:53:49 +0000
+++ tests/unit/__init__.py	2015-10-20 09:18:51 +0000
@@ -144,6 +144,7 @@
             self.script.Archive = Mock()
             self.script.Branch = Mock()
             self.script.DotProject = Mock()
+            self.script.Manager = Mock()
             self.script.Package = Mock()
             self.script.Rev = Mock()
             self.script.SiloState = Mock()
 
=== modified file 'tests/unit/test_recipe_merge.py'
--- tests/unit/test_recipe_merge.py	2015-10-19 22:13:22 +0000
+++ tests/unit/test_recipe_merge.py	2015-10-20 09:18:51 +0000
@@ -242,6 +242,8 @@
             ])
         authors = merge.get_remote_branch_authors()
         self.assertEqual(rev_mock.mock_calls, [
+            call('a', merge1.target_branch.web_link),
+            call().write(merge1.target_branch.last_scanned_id),
             call('a', merge1.web_link),
             call().write(merge1.source_branch.last_scanned_id),
             call('a', merge2.web_link),
 
=== modified file 'tests/unit/test_script_build.py'
--- tests/unit/test_script_build.py	2015-10-17 09:26:56 +0000
+++ tests/unit/test_script_build.py	2015-10-20 09:18:51 +0000
@@ -56,52 +56,40 @@
         self.series = Mock()
         self.ppa = Mock()
 
-    def test_find_all_uploaded(self):
-        """Find everything that has been uploaded from this silo."""
-        self.script.glob.return_value = [
-            '/path/to/foo_0.1_source.ppa.upload',
-            '/path/to/bar_0.2_source.ppa.upload',
-        ]
-        self.assertEqual(
-            sorted(self.script.find_all_uploaded()), ['bar', 'foo'])
-
-    def test_find_all_uploaded_multiple_series(self):
-        """We can handle uploading multiple series from a single silo."""
-        self.script.glob.return_value = [
-            '/path/to/foo_0.1_source.ppa.upload',
-            '/path/to/foo_0.1~utopic_source.ppa.upload',
-            '/path/to/bar_0.2_source.ppa.upload',
-            '/path/to/bar_0.2~utopic_source.ppa.upload',
-        ]
-        self.assertEqual(
-            sorted(self.script.find_all_uploaded()),
-            ['bar', 'bar', 'foo', 'foo'])
-        self.assertEqual(
-            set(self.script.find_all_uploaded()), set(['foo', 'bar']))
-
-    def test_buildmanager_include_all(self):
-        """Correctly switch to watching all packages."""
-        bman = self.script.BuildManager(Mock())
-        bman.types['foo'] = 'bar'
-        self.assertEqual(
-            bman.choose_packages.__doc__,
-            'Decide which packages need to be built.')
-        bman.include_all()
-        self.assertEqual(
-            bman.choose_packages.__doc__,
-            'Act on all silo packages.')
-        self.assertFalse(bman.types)
-
     def test_buildmanager_choose_packages(self):
         """Determine what packages to act upon."""
         silo_state = Mock(
-            all_projects=['a', 'b', 'c'], mps=['a'], sources=['b', 'c'])
-        self.script.env.PACKAGES_TO_REBUILD = ''
-        self.script.env.FORCE_REBUILD = 'false'
-        self.script.find_all_uploaded = Mock(return_value=['c'])
-        bman = self.script.BuildManager(silo_state)
-        bman.choose_packages()
-        self.assertEqual(bman.names, set(['a', 'b']))
+            all_projects=list('abcd'),
+            mps=dict(
+                c=[Mock(
+                    source_branch=Mock(last_scanned_id=None),
+                    web_link='foo/123')],
+                d=[Mock(
+                    source_branch=Mock(last_scanned_id='stale'),
+                    web_link='foo/789')]),
+            sources=list('abcd'))
+        self.script.env.PACKAGES_TO_REBUILD = ''
+        self.script.env.FORCE_REBUILD = 'false'
+        bman = self.script.BuildManager(silo_state)
+        bman.choose_packages()
+        self.assertEqual(bman.names, {'a', 'b', 'd'})
+
+    def test_buildmanager_choose_packages_dirty(self):
+        """Include dirty packages by default."""
+        self.script.get_dirty = Mock(return_value=list('abcde'))
+        silo_state = Mock(
+            all_projects=list('abcd'),
+            mps=dict(
+                c=[Mock(
+                    source_branch=Mock(last_scanned_id=None),
+                    web_link='foo/123')]),
+            sources=list('abcd'))
+        self.script.env.PACKAGES_TO_REBUILD = ''
+        self.script.env.FORCE_REBUILD = 'false'
+        bman = self.script.BuildManager(silo_state)
+        bman.choose_packages()
+        self.script.get_dirty.assert_called_once_with(self.tempdir)
+        self.assertEqual(bman.names, set('abcd'))
 
     def test_buildmanager_choose_packages_twins(self):
         """Always add twins for all builds."""
@@ -116,10 +104,14 @@
     def test_buildmanager_choose_packages_none_found(self):
         """Fail when no packages to act upon."""
         silo_state = Mock(
-            all_projects=['a', 'b', 'c'], mps=['a'], sources=['b', 'c'])
+            all_projects=['a'],
+            mps=dict(a=[Mock(
+                source_branch=Mock(last_scanned_id=None),
+                web_link='foo/123')]),
+            sources=[])
         self.script.env.PACKAGES_TO_REBUILD = ''
         self.script.env.FORCE_REBUILD = 'false'
-        self.script.find_all_uploaded = Mock(return_value=list('abc'))
+        self.script.env.WATCH_ONLY = 'false'
         bman = self.script.BuildManager(silo_state)
         with self.assertRaisesRegexp(BuildError, 'No packages are being'):
             bman.choose_packages()
@@ -132,13 +124,13 @@
         bman = self.script.BuildManager = Mock()
         self.assertEqual(self.script.main(bman), 0)
         bman.assert_called_once_with(silo_state)
-        self.assertEqual(
-            bman.return_value.mock_calls, [
-                call.do('validate'),
-                call.do('clean', 'collect', 'build', 'upload'),
-                call.include_all(),
-                call.do('watch', 'diff'),
-            ])
+        self.assertEqual(bman.return_value.mock_calls, [
+            call.do('validate'),
+            call.do('clean', 'collect', 'build', 'upload'),
+        ])
+        self.assertEqual(self.script.Manager.return_value.mock_calls, [
+            call.do('watch', 'diff'),
+        ])
         self.assertEqual(silo_state.status, 'Packages built.')
         silo_state.mark_dirty.assert_called_once_with()
         silo_state.save_config.assert_called_once_with()
@@ -150,12 +142,12 @@
         bman = self.script.BuildManager = Mock()
         self.assertEqual(self.script.main(bman), 0)
         bman.assert_called_once_with(silo_state)
-        self.assertEqual(
-            bman.return_value.mock_calls, [
-                call.do('validate'),
-                call.include_all(),
-                call.do('watch', 'diff'),
-            ])
+        self.assertEqual(bman.return_value.mock_calls, [
+            call.do('validate'),
+        ])
+        self.assertEqual(self.script.Manager.return_value.mock_calls, [
+            call.do('watch', 'diff'),
+        ])
         silo_state.save_config.assert_called_once_with()
 
     def test_main_error(self):