launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04538
[Merge] lp:~jtv/launchpad/bug-820456 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-820456 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-820456/+merge/70982
= Summary =
This is one of those branches that really ought not to exist. We've been hunting for a bug that we probably haven't found the cause of. It's been driving Raphaël and me crazy. We found more bugs along the way.
And so here you have a diff that:
* Changes behaviour that matches the bug but is probably in a different code path.
* Fixes unrelated general bugginess.
* Cleans up unclear code that gave me trouble along the way.
The first change you'll see in the diff fixes the script that we wrote to opopulate the DistroSeriesDifference table initially, to generate DSDs even for packages that are identical between a derived distroseries and its parent. This is the behaviour that matches the bug but is probably in a different code path. It may seem pointless to fix this but the script is idempotent and in a pinch can always be re-run to provide missing DSDs.
Fast-forward through some cosmetic changes, to lib/lp/soyuz/model/distroseriesdifferencejob.py: I changed make_metadata to take ids rather than objects for its parameters. That lets us reuse it in one other piece of code that duplicated its functionality. The code was also a bit hard to read because it flaunted our python style guide in various ways.
The nested function composeJobInsertionTuple made the code harder to read in that it falsely suggested an end to the function it was in. Since this confusion cost us time while hunting for the real problem, I figure these little cleanups are worthwhile. It was also named according to the wrong convention. I made the new function return a tuple because it turned out to be easier to do the stringification in the caller.
Skip somre more uninteresting diff, and we get to may_require_job. You won't see that name; the diff happens not to include it but it's in the same file. Here's the snippet:
@@ -161,16 +167,9 @@
runner some unnecessary work, but we don't expect a bit of
unnecessary work to be a big problem.
"""
- if derived_series is None:
- return False
- dsp = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
- derived_series)
- if dsp.count() == 0:
- return False
- for parent in dsp:
- if parent.parent_series.distribution == derived_series.distribution:
- # Differences within a distribution are not tracked.
- return False
+ if parent_series.distribution == derived_series.distribution:
+ # Differences within a distribution are not tracked.
+ return False
The whole complicated thing about dsp was a workaround created when multi-parent derivation was thrown into the distro-derivation project as a new requirement. The fnuction did not get a parent series as one of its arguments, so it had no good way of figuring out whether it was being asked whether to create a DSD between series in the same distro or not. Instead, as a heuristic, it just looked if there was any parent at all in the same distribution. Things are simpler now.
(Support for None derived series is no longer needed either; we now need to iterate over children and parents where previously it might occasionally be convenient to pass a reference that might or might not be None).
Then there's more stuff that I feel is clearer if expressed more concisely: when generating a list of objects, use a list comprehension so that the reader doesn't need to work out the structure of the loop and reconstruct the purpose. Don't test that the number of waiting jobs should be exactly 1 in a method whose name suggests that it merely checks for a nonzero number. Don't hide test assertions away in a different part of the call stack than the thing you're testing; make it easy to gather the assertion's inputs and then express the whole assertion in one place. Don't assert list lengths when you can assert list contents, which will help debug failures.
== Tests ==
Er. I ran 'em all.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py
lib/lp/soyuz/model/distroseriesdifferencejob.py
lib/lp/registry/scripts/populate_distroseriesdiff.py
lib/lp/soyuz/tests/test_distroseriesdifferencejob.py
lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py
--
https://code.launchpad.net/~jtv/launchpad/bug-820456/+merge/70982
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-820456 into lp:launchpad.
=== modified file 'lib/lp/registry/scripts/populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/populate_distroseriesdiff.py 2011-07-22 09:01:03 +0000
+++ lib/lp/registry/scripts/populate_distroseriesdiff.py 2011-08-10 03:13:30 +0000
@@ -110,7 +110,6 @@
FROM (%(parent_query)s) AS parent
FULL OUTER JOIN (%(derived_query)s) AS derived
ON derived.sourcepackagename = parent.sourcepackagename
- WHERE derived.version IS DISTINCT FROM parent.version
""" % parameters
@@ -362,7 +361,7 @@
relationships = self.getDistroSeries()
for child in relationships:
for parent in relationships[child]:
- self.logger.info(
+ self.logger.info(
"%s %s with a parent of %s %s", child.distribution.name,
child.name, parent.distribution.name, parent.name)
=== modified file 'lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py'
--- lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-07-22 21:06:30 +0000
+++ lib/lp/registry/scripts/tests/test_populate_distroseriesdiff.py 2011-08-10 03:13:30 +0000
@@ -236,36 +236,47 @@
self.assertContentEqual(
[], Store.of(dsp.derived_series).execute(query))
- def test_does_not_find_identical_releases(self):
+ def test_finds_identical_releases(self):
dsp = self.makeDerivedDistroSeries()
spr = self.factory.makeSourcePackageRelease()
- self.makeSPPH(
+ parent_spph = self.makeSPPH(
distroseries=dsp.parent_series, sourcepackagerelease=spr)
- self.makeSPPH(
+ derived_spph = self.makeSPPH(
distroseries=dsp.derived_series, sourcepackagerelease=spr)
query = compose_sql_find_differences(
dsp.derived_series, dsp.parent_series)
self.assertContentEqual(
- [], Store.of(dsp.derived_series).execute(query))
+ [(
+ spr.sourcepackagename.id,
+ derived_spph.sourcepackagerelease.version,
+ parent_spph.sourcepackagerelease.version,
+ )],
+ Store.of(dsp.derived_series).execute(query))
- def test_ignores_releases_for_same_version(self):
+ def test_finds_releases_for_same_version(self):
dsp = self.makeDerivedDistroSeries()
derived_series = dsp.derived_series
version_string = self.factory.getUniqueString()
parent_series = dsp.parent_series
package = self.factory.makeSourcePackageName()
- self.makeSPPH(
+ derived_spph = self.makeSPPH(
distroseries=derived_series,
sourcepackagerelease=self.factory.makeSourcePackageRelease(
sourcepackagename=package, distroseries=derived_series,
version=version_string))
- self.makeSPPH(
+ parent_spph = self.makeSPPH(
distroseries=parent_series,
sourcepackagerelease=self.factory.makeSourcePackageRelease(
sourcepackagename=package, distroseries=parent_series,
version=version_string))
query = compose_sql_find_differences(derived_series, parent_series)
- self.assertContentEqual([], Store.of(derived_series).execute(query))
+ self.assertContentEqual(
+ [(
+ package.id,
+ derived_spph.sourcepackagerelease.version,
+ parent_spph.sourcepackagerelease.version,
+ )],
+ Store.of(derived_series).execute(query))
def test_finds_release_missing_in_derived_series(self):
dsp = self.makeDerivedDistroSeries()
@@ -408,6 +419,15 @@
expected.value,
self.selectDifferenceType(parent_version=1, derived_version=2))
+ def test_same_version_is_treated_as_resolved_different_versions(self):
+ # Synchronized packages get a DSD type of DIFFERENT_VERSIONS,
+ # though actually this is moot: the DSD will be marked as
+ # RESOLVED anyway.
+ expected = DistroSeriesDifferenceType.DIFFERENT_VERSIONS
+ self.assertEqual(
+ expected.value,
+ self.selectDifferenceType(parent_version=9, derived_version=9))
+
class TestFindDerivedSeries(TestCaseWithFactory, FactoryHelper):
"""Test finding of all derived `DistroSeries`."""
=== modified file 'lib/lp/soyuz/model/distroseriesdifferencejob.py'
--- lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-08-02 13:29:19 +0000
+++ lib/lp/soyuz/model/distroseriesdifferencejob.py 2011-08-10 03:13:30 +0000
@@ -8,14 +8,13 @@
'DistroSeriesDifferenceJob',
]
-from storm.expr import And
from zope.component import getUtility
from zope.interface import (
classProvides,
implements,
)
-from canonical.database.sqlbase import sqlvalues
+from canonical.database.sqlbase import quote
from canonical.launchpad.interfaces.lpstorm import (
IMasterStore,
IStore,
@@ -23,7 +22,6 @@
from lp.registry.interfaces.distroseriesdifference import (
IDistroSeriesDifferenceSource,
)
-from lp.registry.interfaces.distroseriesparent import IDistroSeriesParentSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.model.distroseries import DistroSeries
from lp.registry.model.distroseriesdifference import DistroSeriesDifference
@@ -48,11 +46,11 @@
FEATURE_FLAG_ENABLE_MODULE = u"soyuz.derived_series_jobs.enabled"
-def make_metadata(sourcepackagename, parent_series):
- """Return JSON metadata for a job on `sourcepackagename`."""
+def make_metadata(sourcepackagename_id, parent_series_id):
+ """Return JSON metadata for a job on `sourcepackagename_id`."""
return {
- 'sourcepackagename': sourcepackagename.id,
- 'parent_series': parent_series.id,
+ 'sourcepackagename': sourcepackagename_id,
+ 'parent_series': parent_series_id,
}
@@ -70,50 +68,58 @@
job = DistributionJob(
distribution=derived_series.distribution, distroseries=derived_series,
job_type=DistributionJobType.DISTROSERIESDIFFERENCE,
- metadata=make_metadata(sourcepackagename, parent_series))
+ metadata=make_metadata(sourcepackagename.id, parent_series.id))
IMasterStore(DistributionJob).add(job)
return DistroSeriesDifferenceJob(job)
+def compose_job_insertion_tuple(derived_series, parent_series,
+ sourcepackagename_id, job_id):
+ """Compose tuple for insertion into `DistributionJob`.
+
+ :param derived_series: Derived `DistroSeries`.
+ :param parent_series: Parent `DistroSeries`.
+ :param sourcepackagename_id: ID of `SourcePackageName`.
+ :param job_id: associated `Job` id.
+ :return: A tuple of: derived distribution id, derived distroseries id,
+ job type, job id, JSON data map.
+ """
+ json = DistributionJob.serializeMetadata(make_metadata(
+ sourcepackagename_id, parent_series.id))
+ return (
+ derived_series.distribution.id,
+ derived_series.id,
+ DistributionJobType.DISTROSERIESDIFFERENCE,
+ job_id,
+ json,
+ )
+
+
def create_multiple_jobs(derived_series, parent_series):
- """Create a `DistroSeriesDifferenceJob` for all the source packages in
- archive.
+ """Create `DistroSeriesDifferenceJob`s between parent and derived series.
:param derived_series: A `DistroSeries` that is assumed to be derived
from another one.
:param parent_series: A `DistroSeries` that is a parent of
`derived_series`.
+ :return: A list of newly-created `DistributionJob` ids.
"""
store = IStore(SourcePackageRelease)
source_package_releases = store.find(
SourcePackageRelease,
- And(
- SourcePackagePublishingHistory.sourcepackagerelease ==
- SourcePackageRelease.id,
- SourcePackagePublishingHistory.distroseries == derived_series.id,
- SourcePackagePublishingHistory.status.is_in(
- active_publishing_status)))
+ SourcePackagePublishingHistory.sourcepackagerelease ==
+ SourcePackageRelease.id,
+ SourcePackagePublishingHistory.distroseries == derived_series.id,
+ SourcePackagePublishingHistory.status.is_in(active_publishing_status))
nb_jobs = source_package_releases.count()
sourcepackagenames = source_package_releases.values(
SourcePackageRelease.sourcepackagenameID)
job_ids = Job.createMultiple(store, nb_jobs)
- def composeJobInsertionTuple(derived_series, parent_series,
- sourcepackagename, job_id):
- data = (
- derived_series.distribution.id, derived_series.id,
- DistributionJobType.DISTROSERIESDIFFERENCE, job_id,
- DistributionJob.serializeMetadata(
- {'sourcepackagename': sourcepackagename,
- 'parent_series': parent_series.id}))
- format_string = "(%s)" % ", ".join(["%s"] * len(data))
- return format_string % sqlvalues(*data)
-
- job_contents = [
- composeJobInsertionTuple(
- derived_series, parent_series, sourcepackagename, job_id)
- for job_id, sourcepackagename in
- zip(job_ids, sourcepackagenames)]
+ job_tuples = [
+ quote(compose_job_insertion_tuple(
+ derived_series, parent_series, sourcepackagename, job_id))
+ for job_id, sourcepackagename in zip(job_ids, sourcepackagenames)]
store = IStore(DistributionJob)
result = store.execute("""
@@ -121,7 +127,7 @@
distribution, distroseries, job_type, job, json_data)
VALUES %s
RETURNING id
- """ % ", ".join(job_contents))
+ """ % ", ".join(job_tuples))
return [job_id for job_id, in result]
@@ -132,7 +138,7 @@
# optimization. It's not actually disastrous to create
# redundant jobs occasionally.
json_metadata = DistributionJob.serializeMetadata(
- make_metadata(sourcepackagename, parent_series))
+ make_metadata(sourcepackagename.id, parent_series.id))
# Use master store because we don't like outdated information
# here.
@@ -161,16 +167,9 @@
runner some unnecessary work, but we don't expect a bit of
unnecessary work to be a big problem.
"""
- if derived_series is None:
- return False
- dsp = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
- derived_series)
- if dsp.count() == 0:
- return False
- for parent in dsp:
- if parent.parent_series.distribution == derived_series.distribution:
- # Differences within a distribution are not tracked.
- return False
+ if parent_series.distribution == derived_series.distribution:
+ # Differences within a distribution are not tracked.
+ return False
existing_jobs = find_waiting_jobs(
derived_series, sourcepackagename, parent_series)
return len(existing_jobs) == 0
@@ -204,23 +203,22 @@
PackagePublishingPocket.BACKPORTS,
PackagePublishingPocket.PROPOSED):
return
- jobs = []
- parent_series = derived_series.getParentSeries()
- # Create jobs for DSDs between the derived_series and its
- # parents.
- for parent in parent_series:
- if may_require_job(
- derived_series, sourcepackagename, parent):
- jobs.append(create_job(
- derived_series, sourcepackagename, parent))
+
+ # Create jobs for DSDs between the derived_series' parents and
+ # the derived_series itself.
+ parent_series_jobs = [
+ create_job(derived_series, sourcepackagename, parent)
+ for parent in derived_series.getParentSeries()
+ if may_require_job(derived_series, sourcepackagename, parent)]
+
# Create jobs for DSDs between the derived_series and its
# children.
- for child in derived_series.getDerivedSeries():
- if may_require_job(
- child, sourcepackagename, derived_series):
- jobs.append(create_job(
- child, sourcepackagename, derived_series))
- return jobs
+ derived_series_jobs = [
+ create_job(child, sourcepackagename, derived_series)
+ for child in derived_series.getDerivedSeries()
+ if may_require_job(child, sourcepackagename, derived_series)]
+
+ return parent_series_jobs + derived_series_jobs
@classmethod
def massCreateForSeries(cls, derived_series):
@@ -327,7 +325,7 @@
ds_diff = self.getMatchingDSD()
if ds_diff is None:
- ds_diff = getUtility(IDistroSeriesDifferenceSource).new(
+ getUtility(IDistroSeriesDifferenceSource).new(
self.distroseries, self.sourcepackagename, self.parent_series)
else:
ds_diff.update()
=== modified file 'lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py 2011-07-25 15:53:38 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialize_distroseries.py 2011-08-10 03:13:30 +0000
@@ -964,24 +964,20 @@
self.assertContentEqual(
[u'p1', u'p2'],
- sorted(
- [diff.source_package_name.name
- for diff in dsd_source.getForDistroSeries(child)]))
-
- def assertWaitingJobExists(self, series, name, parent_series):
- self._assertWaitingJobExists(series, name, parent_series)
-
- def assertWaitingJobDoesntExist(self, series, name, parent_series):
- self._assertWaitingJobExists(series, name, parent_series, False)
-
- def _assertWaitingJobExists(self, series, name, parent_series,
- exists=True):
- sourcepackagename = self.factory.getOrMakeSourcePackageName(name)
- self.assertEquals(
- 1 if exists else 0,
- len(
- find_waiting_jobs(
- series, sourcepackagename, parent_series)))
+ [
+ diff.source_package_name.name
+ for diff in dsd_source.getForDistroSeries(child)])
+
+ def getWaitingJobs(self, derived_series, package_name, parent_series):
+ """Get waiting jobs for given derived/parent series and package.
+
+ :return: A list (not a result set or any old iterable, but a list)
+ of `DistroSeriesDifferenceJob`.
+ """
+ sourcepackagename = self.factory.getOrMakeSourcePackageName(
+ package_name)
+ return list(find_waiting_jobs(
+ derived_series, sourcepackagename, parent_series))
def test_initialization_first_deriv_create_dsdjs(self):
# A first initialization of a series creates the creation
@@ -991,8 +987,8 @@
self.useFixture(FeatureFixture({FEATURE_FLAG_ENABLE_MODULE: 'on'}))
child = self._fullInitialize([parent1, parent2])
- self.assertWaitingJobExists(child, 'p1', parent1)
- self.assertWaitingJobExists(child, 'p2', parent2)
+ self.assertNotEqual([], self.getWaitingJobs(child, 'p1', parent1))
+ self.assertNotEqual([], self.getWaitingJobs(child, 'p2', parent1))
def test_initialization_post_first_deriv_create_dsdjs(self):
# Post-first initialization of a series with different parents
@@ -1008,10 +1004,12 @@
self._fullInitialize(
[prev_parent1, prev_parent2, parent3], child=child)
- self.assertWaitingJobExists(child, 'p1', prev_parent1)
- self.assertWaitingJobExists(child, 'p2', prev_parent2)
- self.assertWaitingJobExists(child, 'p2', parent3)
- self.assertWaitingJobDoesntExist(child, 'p3', parent3)
+ self.assertNotEqual(
+ [], self.getWaitingJobs(child, 'p1', prev_parent1))
+ self.assertNotEqual(
+ [], self.getWaitingJobs(child, 'p2', prev_parent2))
+ self.assertNotEqual([], self.getWaitingJobs(child, 'p2', parent3))
+ self.assertEqual([], self.getWaitingJobs(child, 'p3', parent3))
def test_initialization_compute_dsds_specific_packagesets(self):
# Post-first initialization of a series with specific
@@ -1033,8 +1031,9 @@
[prev_parent1, prev_parent2, parent3], child=child,
packagesets=(str(test1.id),))
- self.assertWaitingJobExists(child, 'p1', prev_parent1)
- self.assertWaitingJobDoesntExist(child, 'p11', prev_parent1)
- self.assertWaitingJobDoesntExist(child, 'p2', prev_parent2)
- self.assertWaitingJobExists(child, 'p1', parent3)
- self.assertWaitingJobDoesntExist(child, 'p3', parent3)
+ self.assertNotEqual(
+ [], self.getWaitingJobs(child, 'p1', prev_parent1))
+ self.assertEqual([], self.getWaitingJobs(child, 'p11', prev_parent1))
+ self.assertEqual([], self.getWaitingJobs(child, 'p2', prev_parent2))
+ self.assertNotEqual([], self.getWaitingJobs(child, 'p1', parent3))
+ self.assertEqual([], self.getWaitingJobs(child, 'p3', parent3))
=== modified file 'lib/lp/soyuz/tests/test_distroseriesdifferencejob.py'
--- lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-08-02 13:29:19 +0000
+++ lib/lp/soyuz/tests/test_distroseriesdifferencejob.py 2011-08-10 03:13:30 +0000
@@ -101,29 +101,24 @@
package = self.factory.makeSourcePackageName()
parent_series = self.factory.makeDistroSeries()
self.assertEqual(
- make_metadata(package, parent_series),
- make_metadata(package, parent_series))
+ make_metadata(package.id, parent_series.id),
+ make_metadata(package.id, parent_series.id))
def test_make_metadata_distinguishes_packages(self):
parent_series = self.factory.makeDistroSeries()
one_package = self.factory.makeSourcePackageName()
another_package = self.factory.makeSourcePackageName()
self.assertNotEqual(
- make_metadata(one_package, parent_series),
- make_metadata(another_package, parent_series))
+ make_metadata(one_package.id, parent_series.id),
+ make_metadata(another_package.id, parent_series.id))
def test_make_metadata_distinguishes_parents(self):
package = self.factory.makeSourcePackageName()
one_parent = self.factory.makeDistroSeries()
another_parent = self.factory.makeDistroSeries()
self.assertNotEqual(
- make_metadata(package, one_parent),
- make_metadata(package, another_parent))
-
- def test_may_require_job_accepts_none_derived_series(self):
- parent_series = self.factory.makeDistroSeriesParent().parent_series
- package = self.factory.makeSourcePackageName()
- self.assertFalse(may_require_job(None, package, parent_series))
+ make_metadata(package.id, one_parent.id),
+ make_metadata(package.id, another_parent.id))
def test_may_require_job_allows_new_jobs(self):
dsp = self.factory.makeDistroSeriesParent()
@@ -138,12 +133,6 @@
self.assertFalse(
may_require_job(dsp.derived_series, package, dsp.parent_series))
- def test_may_require_job_forbids_jobs_on_nonderived_series(self):
- sourcepackage = self.factory.makeSourcePackage()
- self.assertFalse(may_require_job(
- sourcepackage.distroseries, sourcepackage.sourcepackagename,
- None))
-
def test_may_require_job_forbids_jobs_for_intra_distro_derivation(self):
package = self.factory.makeSourcePackageName()
parent = self.factory.makeDistroSeries()
