From 11705f4a7b58778d82d564743fb68597863bdda1 Mon Sep 17 00:00:00 2001 From: Martin Pitt Date: Thu, 17 Nov 2016 12:33:26 +0100 Subject: [PATCH] Some PPA landing fixes The original commit had an unreliable policy info (the peer PPA group), as correct updating of friends' excuses depends on the processing order. - Also add the currently investigated package to the policy info. - Add a HTML excuse too, so that excuses.html actually shows the reason for blocking. - Don't invalidate other excuses multiple times. - Change the policy_info to always carry the friend information with it, as that's useful to know for accepted packages too. Adjust test_approve_ppa() accordingly. - Verify "reason" field in test_sourceppa_policy() - Add another integration test to ensure that this also applies to unbuilt packages, not just to rejection from other Policies. - Avoid FileNotFoundError, this does not yet exist in Python 3.2. --- policies/sourceppa.py | 36 +++++++++++++++++++++--------------- tests/test_autopkgtest.py | 29 ++++++++++++++++++++++++----- tests/test_sourceppa.py | 7 ++++++- 3 files changed, 51 insertions(+), 21 deletions(-) diff --git a/policies/sourceppa.py b/policies/sourceppa.py index 90a7ca8..578623b 100644 --- a/policies/sourceppa.py +++ b/policies/sourceppa.py @@ -70,16 +70,14 @@ class SourcePPAPolicy(BasePolicy): return '' def initialise(self, britney): - """Load and discover all source ppa data""" + """Load cached source ppa data""" super().initialise(britney) self.britney = britney - try: + if os.path.exists(self.filename): with open(self.filename, encoding='utf-8') as data: self.cache = json.load(data) self.log("Loaded cached source ppa data from %s" % self.filename) - except FileNotFoundError: - pass def apply_policy_impl(self, sourceppa_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse): """Reject package if any other package copied from same PPA is invalid""" @@ -87,22 +85,30 @@ class SourcePPAPolicy(BasePolicy): britney_excuses = self.britney.excuses version = source_data_srcdist.version sourceppa = self.lp_get_source_ppa(source_name, version) + if not sourceppa: + return PolicyVerdict.PASS + + shortppa = sourceppa.replace(LAUNCHPAD_URL, '') self.source_ppas_by_pkg[source_name][version] = sourceppa - if sourceppa: - # Check for other packages that might invalidate this one - for friend in self.pkgs_by_source_ppa[sourceppa]: - if not britney_excuses[friend].is_valid: - accept = False - self.pkgs_by_source_ppa[sourceppa].add(source_name) + sourceppa_info[source_name] = shortppa + # Check for other packages that might invalidate this one + for friend in self.pkgs_by_source_ppa[sourceppa]: + sourceppa_info[friend] = shortppa + if not britney_excuses[friend].is_valid: + accept = False + self.pkgs_by_source_ppa[sourceppa].add(source_name) + if not accept: # Invalidate all packages in this source ppa - shortppa = sourceppa.replace(LAUNCHPAD_URL, '') for friend in self.pkgs_by_source_ppa[sourceppa]: friend_exc = britney_excuses.get(friend, excuse) - friend_exc.is_valid = False - friend_exc.addreason('source-ppa') - self.log("Blocking %s because %s from %s" % (friend, source_name, shortppa)) - sourceppa_info[friend] = shortppa + if friend_exc.is_valid: + friend_exc.is_valid = False + friend_exc.addreason('source-ppa') + friend_exc.policy_info['source-ppa'] = sourceppa_info + self.log("Blocking %s because %s from %s" % (friend, source_name, shortppa)) + friend_exc.addhtml("Blocking because %s from the same PPA %s is invalid" % + (friend, shortppa)) return PolicyVerdict.REJECTED_PERMANENTLY return PolicyVerdict.PASS diff --git a/tests/test_autopkgtest.py b/tests/test_autopkgtest.py index b8afaae..f8a9a66 100755 --- a/tests/test_autopkgtest.py +++ b/tests/test_autopkgtest.py @@ -2042,8 +2042,9 @@ class T(TestBase): # Tests for source ppa grouping ################################################################ - def test_sourceppa(self): - '''Packages from same source PPA should be grouped.''' + def test_sourceppa_policy(self): + '''Packages from same source PPA get rejected for failed peer policy''' + self.sourceppa_cache['green'] = {'2': 'team/ubuntu/ppa'} self.sourceppa_cache['red'] = {'2': 'team/ubuntu/ppa'} with open(os.path.join(self.data.path, 'data/series-proposed/Blocks'), 'w') as f: @@ -2052,12 +2053,30 @@ class T(TestBase): exc = self.do_test( [('green', {'Version': '2'}, 'autopkgtest'), ('red', {'Version': '2'}, 'autopkgtest')], - {'green': (False, {'green': {'i386': 'RUNNING-ALWAYSFAIL', 'amd64': 'RUNNING-ALWAYSFAIL'}})}, - {'green': [('is-candidate', False)], - 'red': [('is-candidate', False)]} + {'green': (False, {'green': {'i386': 'RUNNING-ALWAYSFAIL', 'amd64': 'RUNNING-ALWAYSFAIL'}}), + 'red': (False, {'red': {'i386': 'RUNNING-ALWAYSFAIL', 'amd64': 'RUNNING-ALWAYSFAIL'}})}, + {'green': [('reason', 'block')], + 'red': [('reason', 'source-ppa')]} )[1] self.assertEqual(exc['red']['policy_info']['source-ppa'], {'red': 'team/ubuntu/ppa', 'green': 'team/ubuntu/ppa'}) + def test_sourceppa_missingbuild(self): + '''Packages from same source PPA get rejected for failed peer FTBFS''' + + self.sourceppa_cache['green'] = {'2': 'team/ubuntu/ppa'} + self.sourceppa_cache['red'] = {'2': 'team/ubuntu/ppa'} + + self.data.add_src('green', True, {'Version': '2', 'Testsuite': 'autopkgtest'}) + self.data.add('libgreen1', True, {'Version': '2', 'Source': 'green', 'Architecture': 'i386'}, add_src=False) + + exc = self.do_test( + [('red', {'Version': '2'}, 'autopkgtest')], + {'green': (False, {}), 'red': (False, {})}, + {'green': [('missing-builds', {'on-architectures': ['amd64', 'arm64', 'armhf', 'powerpc', 'ppc64el'], + 'on-unimportant-architectures': []})], + 'red': [('reason', 'source-ppa')]} + )[1] + self.assertEqual(exc['red']['policy_info']['source-ppa'], {'red': 'team/ubuntu/ppa', 'green': 'team/ubuntu/ppa'}) if __name__ == '__main__': unittest.main() diff --git a/tests/test_sourceppa.py b/tests/test_sourceppa.py index d1e5aa5..35ffd5f 100755 --- a/tests/test_sourceppa.py +++ b/tests/test_sourceppa.py @@ -31,10 +31,14 @@ class FakeOptions: class FakeExcuse: ver = ('1.0', '2.0') is_valid = True + policy_info = {} def addreason(self, reason): """Ignore reasons.""" + def addhtml(self, reason): + """Ignore reasons.""" + class FakeBritney: def __init__(self): @@ -96,13 +100,14 @@ class T(unittest.TestCase): def test_approve_ppa(self): """Approve packages by their PPA.""" + shortppa = 'team/ubuntu/ppa' pol = SourcePPAPolicy(FakeOptions) pol.filename = CACHE_FILE pol.initialise(FakeBritney()) output = {} for pkg in ('pal', 'buddy', 'friend', 'noppa'): self.assertEqual(pol.apply_policy_impl(output, None, pkg, None, FakeData, FakeExcuse), PolicyVerdict.PASS) - self.assertEqual(output, {}) + self.assertEqual(output, dict(pal=shortppa, buddy=shortppa, friend=shortppa)) def test_reject_ppa(self): """Reject packages by their PPA."""