Launchpad itself

Merge lp:~stevenk/launchpad/db-spph-ancestry into lp:launchpad/db-devel

  1. db-spph-ancestry
  2. Merge into db-devel
Proposed by Steve Kowalik
Status: Merged
Approved by: Steve Kowalik
Approved revision: no longer in the source branch.
Merged at revision: 10030
Proposed branch: lp:~stevenk/launchpad/db-spph-ancestry
Merge into: lp:launchpad/db-devel
Diff against target: 172 lines (+48/-7)
7 files modified
database/schema/comments.sql (+2/-1)
database/schema/patch-2208-34-0.sql (+8/-0)
lib/canonical/launchpad/interfaces/_schema_circular_imports.py (+3/-0)
lib/lp/soyuz/interfaces/publishing.py (+9/-1)
lib/lp/soyuz/model/publishing.py (+7/-2)
lib/lp/soyuz/tests/test_publishing.py (+17/-2)
lib/lp/testing/factory.py (+2/-1)
To merge this branch: bzr merge lp:~stevenk/launchpad/db-spph-ancestry
Reviewer Review Type Date Requested Status
Jonathan Lange (community) Approve
Stuart Bishop (community) db Approve
Jeroen T. Vermeulen (community) code Approve
Review via email: mp+41943@code.launchpad.net

Commit message

[r=jml,jtv,stub][ui=none][bug=684101] Nail an ancestor attribute onto SourcePackagePublishingHistory.

Description of the change

Add an ancestor attribute onto SourcePackagePublishingHistory. This means we can explicitly set what the previous release of a publishing record is, which lays a foundation for traversing the database to find common versions for the derivation work, and as a win, allows us to massively refactor the domination routine to be a shadow of its former self.

At the moment, nothing makes use of this field, but I wanted to get it into this months rollout.

To post a comment you must log in.
Revision history for this message
Jeroen T. Vermeulen (jtv) wrote :

Nicely put!

The code looks good to me. Just a few points:

Looks like it was impossible to set the right schema for ancestor in lib/lp/soyuz/interfaces/publishing.py (diff line 49), so you had to set it to mere Interface instead. When you do that, fix things up in lib/canonical/launchpad/interfaces/_schema_circular_imports.py.

Also in lib/lp/soyuz/interfaces/publishing.py, diff line 61/62, the new parameter to newSourcePublication defaults to None but the method definition in the interface does not reflect that.

In lib/lp/soyuz/tests/test_publishing.py, I don't suppose TestSPPHModel could get away with a lighter test layer such as ZopelessDatabaseLayer? Probably not, but always nice to save a dozen seconds or so on test setup when you can.

Jeroen

review: Approve (code)
Revision history for this message
Jonathan Lange (jml) wrote :

Two questions:
  * "parent" is a more common term for "immediate ancestor". Why not use that?
  * In previous discussions, I'd heard mention of a DAG model for ancestry, rather than a simple tree model. Why did you decide to go with the tree?

Thanks,
jml

review: Needs Information
Revision history for this message
Steve Kowalik (stevenk) wrote :

The original implementation for this idea was going to be a new table called 'SourcePackageAncestry', which would have links to the distroseries, archive and pocket, and the spr, allowing us to traverse the list to find the base versions. While having this discussion with Julian and William, Julian had the idea that SourcePackagePublishingHistory has all this information, and we could add an ancestor column to SPPH to serve the same purpose. So, 'hysterical raisins', I guess.

The purpose of this field is not model the ways that packages relate to another one in a definitive way, its original intent was to allow us to traverse back up the tree to see the highest common version for the derivation work.

Revision history for this message
Jonathan Lange (jml) wrote :

OK. I strongly recommend renaming to 'parent' then, in order to be clearer.

I don't really get your second answer. You are using a tree rather than a DAG because you want to traverse back up the tree to see the highest common version. Why do you want to do that? It also doesn't answer the question about what happened to the discussion about using the DAG model.

Revision history for this message
Steve Kowalik (stevenk) wrote :

Because using a DAG for this purpose is, I believe, over-engineering the problem. We aren't using this to keep track of how publishing records relate to one another (for example, tracking copies between pockets in the Ubuntu archive, or between PPAs) -- for that we certainly would require a DAG. We are using a tree to model versioning information between publishing records.

For example:

foo (1.4-1ubuntu1) [lucid] --- foo (1.4-1ubuntu1) [karmic] --- foo (1.3-2) [jaunty] --- foo (1.2-1ubuntu1) [intrepid]

In this case, both of the parents for the publishing records for foo in both lucid and karmic would point to the jaunty version, and the parent of the jaunty version would be the version in intrepid.

As for your question for why we want to do this, I shall extend my example with this tree from the versions of foo in Debian:

foo (1.4-1) [unstable] --- foo (1.4-1) [testing] --- foo (1.3-2) [stable]

One of the issues we need to solve for derived distributions is to find the base version -- that is, the point where the packages diverged. Using the two examples together, we can see that the version of package in jaunty and stable are equal, and use that to draw out the changes made in Debian, and the changes made in Ubuntu and the changes made upstream, and are able to present these to the user.

Revision history for this message
Steve Kowalik (stevenk) wrote :

And a further comment, ancestor and ancestry is already in widespread use throughout the Soyuz codebase, so I'm keeping that up.

Revision history for this message
Stuart Bishop (stub) wrote :

I suspect we will need an index too or we will be sorry. It won't be used for the primary use case, but deleting spph records or traversing to child records will be painful without it.

CREATE INDEX sourcepackagepublishinghistory__ancestor__idx ON sourcepackagepublishinghistory(ancestor);

patch-2208-34-0.sql

review: Approve (db)
Revision history for this message
Julian Edwards (julian-edwards) wrote :

> OK. I strongly recommend renaming to 'parent' then, in order to be clearer.
>
> I don't really get your second answer. You are using a tree rather than a DAG
> because you want to traverse back up the tree to see the highest common
> version. Why do you want to do that? It also doesn't answer the question
> about what happened to the discussion about using the DAG model.

Source ancestry is a strong meme in the Soyuz code. I'd much rather the name was consistent with that than introducing a new name for the same concept.

Revision history for this message
Jonathan Lange (jml) wrote :

Thanks for the reply Steven.

I agree it's better to keep consistency with the existing bad name than introduce a new concept.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'database/schema/comments.sql'
2--- database/schema/comments.sql 2010-12-02 06:23:50 +0000
3+++ database/schema/comments.sql 2010-12-02 06:53:07 +0000
4@@ -1788,7 +1788,8 @@
5 COMMENT ON COLUMN SourcePackagePublishingHistory.pocket IS 'The pocket into which this record is published. The RELEASE pocket (zero) provides behaviour as normal. Other pockets may append things to the distroseries name such as the UPDATES pocket (-updates), the SECURITY pocket (-security) and the PROPOSED pocket (-proposed)';
6 COMMENT ON COLUMN SourcePackagePublishingHistory.removed_by IS 'Person responsible for the removal.';
7 COMMENT ON COLUMN SourcePackagePublishingHistory.removal_comment IS 'Reason why the publication was removed.';
8-COMMENT ON COLUMN SourcePackagePublishingHistory.archive IS 'The target archive for thi publishing record.';
9+COMMENT ON COLUMN SourcePackagePublishingHistory.archive IS 'The target archive for this publishing record.';
10+COMMENT ON COLUMN SourcePackagePublishingHistory.ancestor IS 'The source package record published immediately before this one.';
11
12 -- Packaging
13 COMMENT ON TABLE Packaging IS 'DO NOT JOIN THROUGH THIS TABLE. This is a set
14
15=== added file 'database/schema/patch-2208-34-0.sql'
16--- database/schema/patch-2208-34-0.sql 1970-01-01 00:00:00 +0000
17+++ database/schema/patch-2208-34-0.sql 2010-12-02 06:53:07 +0000
18@@ -0,0 +1,8 @@
19+SET client_min_messages=ERROR;
20+
21+ALTER TABLE SourcePackagePublishingHistory
22+ ADD COLUMN ancestor INTEGER REFERENCES SourcePackagePublishingHistory;
23+CREATE INDEX sourcepackagepublishinghistory__ancestor__idx ON sourcepackagepublishinghistory(ancestor);
24+
25+INSERT INTO LaunchpadDatabaseRevision VALUES (2208, 34, 0);
26+
27
28=== modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
29--- lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-12-01 19:12:00 +0000
30+++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py 2010-12-02 06:53:07 +0000
31@@ -277,6 +277,9 @@
32 IBinaryPackagePublishingHistory, 'archive', IArchive)
33 patch_reference_property(
34 ISourcePackagePublishingHistory, 'archive', IArchive)
35+patch_reference_property(
36+ ISourcePackagePublishingHistory, 'ancestor',
37+ ISourcePackagePublishingHistory)
38
39 # IArchive apocalypse.
40 patch_reference_property(IArchive, 'distribution', IDistribution)
41
42=== modified file 'lib/lp/soyuz/interfaces/publishing.py'
43--- lib/lp/soyuz/interfaces/publishing.py 2010-11-10 04:25:52 +0000
44+++ lib/lp/soyuz/interfaces/publishing.py 2010-12-02 06:53:07 +0000
45@@ -458,6 +458,12 @@
46 "package that has been published in the main distribution series, "
47 "if one exists, or None.")
48
49+ ancestor = Reference(
50+ Interface, # Really ISourcePackagePublishingHistory
51+ title=_('Ancestor'),
52+ description=_('The previous release of this source package.'),
53+ required=False, readonly=True)
54+
55 # Really IBinaryPackagePublishingHistory, see below.
56 @operation_returns_collection_of(Interface)
57 @export_read_operation()
58@@ -874,7 +880,7 @@
59 """
60
61 def newSourcePublication(archive, sourcepackagerelease, distroseries,
62- component, section, pocket):
63+ component, section, pocket, ancestor):
64 """Create a new `SourcePackagePublishingHistory`.
65
66 :param archive: An `IArchive`
67@@ -883,6 +889,8 @@
68 :param component: An `IComponent`
69 :param section: An `ISection`
70 :param pocket: A `PackagePublishingPocket`
71+ :param ancestor: A `ISourcePackagePublishingHistory` for the previous
72+ version of this publishing record
73
74 datecreated will be UTC_NOW.
75 status will be PackagePublishingStatus.PENDING
76
77=== modified file 'lib/lp/soyuz/model/publishing.py'
78--- lib/lp/soyuz/model/publishing.py 2010-12-01 11:26:57 +0000
79+++ lib/lp/soyuz/model/publishing.py 2010-12-02 06:53:07 +0000
80@@ -432,6 +432,9 @@
81 dbName="removed_by", foreignKey="Person",
82 storm_validator=validate_public_person, default=None)
83 removal_comment = StringCol(dbName="removal_comment", default=None)
84+ ancestor = ForeignKey(
85+ dbName="ancestor", foreignKey="SourcePackagePublishingHistory",
86+ default=None)
87
88 @property
89 def package_creator(self):
90@@ -1309,7 +1312,8 @@
91 return pub
92
93 def newSourcePublication(self, archive, sourcepackagerelease,
94- distroseries, component, section, pocket):
95+ distroseries, component, section, pocket,
96+ ancestor=None):
97 """See `IPublishingSet`."""
98 if archive.is_ppa:
99 # PPA component must always be 'main', so we override it
100@@ -1323,7 +1327,8 @@
101 component=component,
102 section=section,
103 status=PackagePublishingStatus.PENDING,
104- datecreated=UTC_NOW)
105+ datecreated=UTC_NOW,
106+ ancestor=ancestor)
107 # Import here to prevent import loop.
108 from lp.registry.model.distributionsourcepackage import (
109 DistributionSourcePackage)
110
111=== modified file 'lib/lp/soyuz/tests/test_publishing.py'
112--- lib/lp/soyuz/tests/test_publishing.py 2010-10-06 11:46:51 +0000
113+++ lib/lp/soyuz/tests/test_publishing.py 2010-12-02 06:53:07 +0000
114@@ -23,8 +23,8 @@
115 from canonical.testing.layers import (
116 DatabaseFunctionalLayer,
117 LaunchpadZopelessLayer,
118+ reconnect_stores,
119 )
120-from canonical.testing.layers import reconnect_stores
121 from lp.app.errors import NotFoundError
122 from lp.archivepublisher.config import Config
123 from lp.archivepublisher.diskpool import DiskPool
124@@ -57,7 +57,10 @@
125 BinaryPackagePublishingHistory,
126 SourcePackagePublishingHistory,
127 )
128-from lp.testing import TestCaseWithFactory
129+from lp.testing import (
130+ login_as,
131+ TestCaseWithFactory,
132+ )
133 from lp.testing.factory import LaunchpadObjectFactory
134 from lp.testing.fakemethod import FakeMethod
135
136@@ -1334,3 +1337,15 @@
137 series, bins[0].pocket, bins[0].archive)
138 self.checkOtherPublications(bins[0], bins)
139 self.checkOtherPublications(foreign_bins[0], foreign_bins)
140+
141+class TestSPPHModel(TestCaseWithFactory):
142+ """Test parts of the SourcePackagePublishingHistory model."""
143+
144+ layer = LaunchpadZopelessLayer
145+
146+ def testAncestry(self):
147+ """Ancestry can be traversed."""
148+ ancestor = self.factory.makeSourcePackagePublishingHistory()
149+ spph = self.factory.makeSourcePackagePublishingHistory(
150+ ancestor=ancestor)
151+ self.assertEquals(spph.ancestor.displayname, ancestor.displayname)
152
153=== modified file 'lib/lp/testing/factory.py'
154--- lib/lp/testing/factory.py 2010-12-01 11:26:57 +0000
155+++ lib/lp/testing/factory.py 2010-12-02 06:53:07 +0000
156@@ -2836,6 +2836,7 @@
157 dsc_format='1.0',
158 dsc_binaries='foo-bin',
159 sourcepackagerelease=None,
160+ ancestor=None,
161 ):
162 """Make a `SourcePackagePublishingHistory`."""
163 if distroseries is None:
164@@ -2879,7 +2880,7 @@
165 spph = getUtility(IPublishingSet).newSourcePublication(
166 archive, sourcepackagerelease, distroseries,
167 sourcepackagerelease.component, sourcepackagerelease.section,
168- pocket)
169+ pocket, ancestor)
170
171 naked_spph = removeSecurityProxy(spph)
172 naked_spph.status = status

Subscribers

People subscribed via source and target branches

to status/vote changes: