Launchpad itself

Merge lp://staging/~gary/launchpad/honor_revisions into lp://staging/launchpad

  1. honor_revisions
  2. Merge into devel
Proposed by Gary Poster
Status: Merged
Approved by: Francis J. Lacoste
Approved revision: no longer in the source branch.
Merged at revision: not available
Proposed branch: lp://staging/~gary/launchpad/honor_revisions
Merge into: lp://staging/launchpad
Diff against target: 329 lines (+131/-46)
3 files modified
lib/devscripts/sourcecode.py (+77/-18)
lib/devscripts/tests/test_sourcecode.py (+35/-9)
utilities/sourcedeps.conf (+19/-19)
To merge this branch: bzr merge lp://staging/~gary/launchpad/honor_revisions
Reviewer Review Type Date Requested Status
Tom Haddon Approve
Francis J. Lacoste (community) Approve
Review via email: mp+15227@code.staging.launchpad.net
To post a comment you must log in.
Revision history for this message
Gary Poster (gary) wrote :
Download full text (3.4 KiB)

Hi.

This branch changes the sourcecode update script to honor revision numbers. It also adds revision numbers to sourcedeps.conf. This change is important to allow us to move production-devel to buildbot.

An early plan for this branch was to remove our sourcecode update script entirely in favor of config-manager. However, our current script already handles our "optional" special cases of canonical-identity-provider and shipit. It also has more informative output than config-manager. Finally, in my tests, it was twice as fast locally for an update of launchpad's dependencies than config-manager was (one minute for the current script, and two minutes for config-manager). I'm not sure why config-manager was slower; there are probably good reasons for why. However, our update script is specific to our needs and does noticeably better with them, so I stuck with it.

Updating the script to honor revision numbers was pretty easy, especially with the config-manager example to work from for bzrlib guidance.

I chose to use the same syntax for specifying revisions that config-manager used, because config-manager represented a precedent that actually matters to our team (it is used for deployment), and I didn't have a strong preference for another syntax.

I chose to switch to using whitespace for delimiting the values in the configuration file (rather than "=") because it was very easy, especially to accommodate the config-manager revision syntax (which uses a "=").

I kept to about the same level of automated tests as we had before: tests for the parser, but not for the meat of the code, using bzrlib. I would feel better about stubbing out bzrlib using something like Gustavo's Mocker, but what I have is expedient and has precedent.

To test, first you can run ``./bin/test -t test_sourcecode``. Then (for test or QA), run ``./utilities/update-sourcecode``. If you have updated your sourcecode lately, you should see a number of updates that report "No change". To verify that I specified all of the most recent revisions of these branches, run ``./utilities/update-sourcecode --tip``. This should again report "No change" for each branch. To verify that it does what you expect in other contexts, you can remove a sourcecode branch and rerun ``./utilities/update-sourcecode``, to see it build a branch; and you can modify revision numbers in ``./utiltities/sourcedeps.conf`` to see if the changes you expect happen (reverting and updating).

I'm asking Tom Haddon to review this because I'm going to suggest that deployment use this script to get the sourcecode branches. Then, for production, the change will be to insert a new step 2 into the following rough process: (1) run config-manager to check out the correct launchpad branch and the download-cache, then (2) run this script with no arguments to get the sourcecode branches, and then (3) run make as usual.

This is instead of what we had discussed before, which was to insert steps 2 and 3 in this version of the process: (1) run config-manager to check out the correct launchpad branch and the download-cache, then (2) run some launchpad script that generates another config-manager file, then (3) reru...

Read more...

Revision history for this message
Francis J. Lacoste (flacoste) wrote :

  review approve
  status approved

Hi Gary,

This is excellent. My only comment on the code is that you three instances of
if not tip and revision:

Not a big deal, but raises my three strikes and refactor alarm. Maybe you
could modify get_revision_id to return revision and revision_name and use that
throughout?

Anyway, take it or leave it.

On November 24, 2009, Gary Poster wrote:
> This branch changes the sourcecode update script to honor revision
> numbers. It also adds revision numbers to sourcedeps.conf. This change
> is important to allow us to move production-devel to buildbot.
>

I'd like to point out that this will also allow us to get rid of running the
whole Launchpad test suite on all of the branches still managed by PQM (apart
ShipIt and c-i-p) We can only run their test suite and then land the branch
updating the revision spec in the Launchpad tree using the regular mechanism.

--
Francis J. Lacoste
<email address hidden>

review: Approve
Revision history for this message
Tom Haddon (mthaddon) wrote :

Looks good. We'll need to do a bit of work to integrate this into our build steps, but since we can do that at any time after it lands, I don't see any problem with this landing.

review: Approve
Revision history for this message
Gary Poster (gary) wrote :

> ...My only comment on the code is that you three instances of
> if not tip and revision:
>
> Not a big deal, but raises my three strikes and refactor alarm. Maybe you
> could modify get_revision_id to return revision and revision_name and use that
> throughout?

Thanks, yes. I have made a change that does something similar.

> I'd like to point out that this will also allow us to get rid of running the
> whole Launchpad test suite on all of the branches still managed by PQM (apart
> ShipIt and c-i-p) We can only run their test suite and then land the branch
> updating the revision spec in the Launchpad tree using the regular mechanism.

Good point! Cool.

Gary

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'lib/devscripts/sourcecode.py'
2--- lib/devscripts/sourcecode.py 2009-09-30 11:56:28 +0000
3+++ lib/devscripts/sourcecode.py 2009-11-25 16:35:33 +0000
4@@ -19,6 +19,7 @@
5 from bzrlib.bzrdir import BzrDir
6 from bzrlib.errors import BzrError
7 from bzrlib.plugin import load_plugins
8+from bzrlib.revisionspec import RevisionSpec
9 from bzrlib.trace import report_exception
10 from bzrlib.transport import get_transport
11 from bzrlib.workingtree import WorkingTree
12@@ -37,12 +38,28 @@
13 for line in file_handle:
14 if line.startswith('#'):
15 continue
16- yield [token.strip() for token in line.split('=')]
17+ yield line.split()
18
19
20 def interpret_config_entry(entry):
21 """Interpret a single parsed line from the config file."""
22- return entry[0], entry[1], len(entry) > 2
23+ branch_name = entry[0]
24+ components = entry[1].split(';revno=')
25+ branch_url = components[0]
26+ if len(components) == 1:
27+ revision = None
28+ else:
29+ assert len(components) == 2, 'Bad branch URL: ' + entry[1]
30+ revision = components[1] or None
31+ if len(entry) > 2:
32+ assert len(entry) == 3 and entry[2].lower() == 'optional', (
33+ 'Bad configuration line: should be space delimited values of '
34+ 'sourcecode directory name, branch URL [, "optional"]\n' +
35+ ' '.join(entry))
36+ optional = True
37+ else:
38+ optional = False
39+ return branch_name, branch_url, revision, optional
40
41
42 def interpret_config(config_entries, public_only):
43@@ -55,9 +72,10 @@
44 """
45 config = {}
46 for entry in config_entries:
47- branch_name, branch_url, optional = interpret_config_entry(entry)
48+ branch_name, branch_url, revision, optional = interpret_config_entry(
49+ entry)
50 if not optional or not public_only:
51- config[branch_name] = (branch_url, optional)
52+ config[branch_name] = (branch_url, revision, optional)
53 return config
54
55
56@@ -99,10 +117,33 @@
57 for branch in BzrDir.find_branches(transport))
58
59
60+def get_revision_id(revision, from_branch, tip=False):
61+ """Return revision id for a revision number and a branch.
62+
63+ If the revision is empty, the revision_id will be None.
64+
65+ If ``tip`` is True, the revision value will be ignored.
66+ """
67+ if not tip and revision:
68+ spec = RevisionSpec.from_string(revision)
69+ return spec.as_revision_id(from_branch)
70+ # else return None
71+
72+
73+def _format_revision_name(revision, tip=False):
74+ """Formatting helper to return human-readable identifier for revision.
75+
76+ If ``tip`` is True, the revision value will be ignored.
77+ """
78+ if not tip and revision:
79+ return 'revision %s' % (revision,)
80+ else:
81+ return 'tip'
82+
83 def get_branches(sourcecode_directory, new_branches,
84- possible_transports=None):
85+ possible_transports=None, tip=False):
86 """Get the new branches into sourcecode."""
87- for project, (branch_url, optional) in new_branches.iteritems():
88+ for project, (branch_url, revision, optional) in new_branches.iteritems():
89 destination = os.path.join(sourcecode_directory, project)
90 try:
91 remote_branch = Branch.open(
92@@ -115,28 +156,33 @@
93 raise
94 possible_transports.append(
95 remote_branch.bzrdir.root_transport)
96- print 'Getting %s from %s' % (project, branch_url)
97+ print 'Getting %s from %s at %s' % (
98+ project, branch_url, _format_revision_name(revision, tip))
99 # If the 'optional' flag is set, then it's a branch that shares
100 # history with Launchpad, so we should share repositories. Otherwise,
101 # we should avoid sharing repositories to avoid format
102 # incompatibilities.
103 force_new_repo = not optional
104+ revision_id = get_revision_id(revision, remote_branch, tip)
105 remote_branch.bzrdir.sprout(
106- destination, create_tree_if_local=True,
107+ destination, revision_id=revision_id, create_tree_if_local=True,
108 source_branch=remote_branch, force_new_repo=force_new_repo,
109 possible_transports=possible_transports)
110
111
112 def update_branches(sourcecode_directory, update_branches,
113- possible_transports=None):
114+ possible_transports=None, tip=False):
115 """Update the existing branches in sourcecode."""
116 if possible_transports is None:
117 possible_transports = []
118 # XXX: JonathanLange 2009-11-09: Rather than updating one branch after
119 # another, we could instead try to get them in parallel.
120- for project, (branch_url, optional) in update_branches.iteritems():
121+ for project, (branch_url, revision, optional) in (
122+ update_branches.iteritems()):
123+ # Update project from branch_url.
124 destination = os.path.join(sourcecode_directory, project)
125- print 'Updating %s' % (project,)
126+ print 'Updating %s to %s' % (
127+ project, _format_revision_name(revision, tip)),
128 local_tree = WorkingTree.open(destination)
129 try:
130 remote_branch = Branch.open(
131@@ -149,9 +195,19 @@
132 raise
133 possible_transports.append(
134 remote_branch.bzrdir.root_transport)
135- local_tree.pull(
136- remote_branch, overwrite=True,
137+ revision_id = get_revision_id(revision, remote_branch, tip)
138+ result = local_tree.pull(
139+ remote_branch, stop_revision=revision_id, overwrite=True,
140 possible_transports=possible_transports)
141+ if result.old_revno == result.new_revno:
142+ print '(No change)'
143+ else:
144+ if result.old_revno < result.new_revno:
145+ change = 'Updated'
146+ else:
147+ change = 'Reverted'
148+ print '(%s from %s to %s)' % (
149+ change, result.old_revno, result.new_revno)
150
151
152 def remove_branches(sourcecode_directory, removed_branches):
153@@ -166,7 +222,7 @@
154
155
156 def update_sourcecode(sourcecode_directory, config_filename, public_only,
157- dry_run):
158+ tip, dry_run):
159 """Update the sourcecode."""
160 config_file = open(config_filename)
161 config = interpret_config(parse_config_file(config_file), public_only)
162@@ -179,8 +235,8 @@
163 print 'Branches to update:', updated.keys()
164 print 'Branches to remove:', list(removed)
165 else:
166- get_branches(sourcecode_directory, new, possible_transports)
167- update_branches(sourcecode_directory, updated, possible_transports)
168+ get_branches(sourcecode_directory, new, possible_transports, tip)
169+ update_branches(sourcecode_directory, updated, possible_transports, tip)
170 remove_branches(sourcecode_directory, removed)
171
172
173@@ -201,8 +257,11 @@
174 '--public-only', action='store_true',
175 help='Only fetch/update the public sourcecode branches.')
176 parser.add_option(
177+ '--tip', action='store_true',
178+ help='Ignore revision constraints for all branches and pull tip')
179+ parser.add_option(
180 '--dry-run', action='store_true',
181- help='Only fetch/update the public sourcecode branches.')
182+ help='Do nothing, but report what would have been done.')
183 options, args = parser.parse_args(args)
184 root = get_launchpad_root()
185 if len(args) > 1:
186@@ -220,5 +279,5 @@
187 load_plugins()
188 update_sourcecode(
189 sourcecode_directory, config_filename,
190- options.public_only, options.dry_run)
191+ options.public_only, options.tip, options.dry_run)
192 return 0
193
194=== modified file 'lib/devscripts/tests/test_sourcecode.py'
195--- lib/devscripts/tests/test_sourcecode.py 2009-11-21 00:34:12 +0000
196+++ lib/devscripts/tests/test_sourcecode.py 2009-11-25 16:35:33 +0000
197@@ -34,7 +34,7 @@
198 def test_single_value(self):
199 # Parsing a file containing a single key=value pair returns a sequence
200 # containing the (key, value) as a list.
201- config_file = self.makeFile("key=value")
202+ config_file = self.makeFile("key value")
203 self.assertEqual(
204 [['key', 'value']], list(parse_config_file(config_file)))
205
206@@ -45,7 +45,7 @@
207
208 def test_optional_value(self):
209 # Lines in the config file can have a third optional entry.
210- config_file = self.makeFile('key=value=optional')
211+ config_file = self.makeFile('key value optional')
212 self.assertEqual(
213 [['key', 'value', 'optional']],
214 list(parse_config_file(config_file)))
215@@ -53,7 +53,7 @@
216 def test_whitespace_stripped(self):
217 # Any whitespace around any of the tokens in the config file are
218 # stripped out.
219- config_file = self.makeFile(' key = value = optional ')
220+ config_file = self.makeFile(' key value optional ')
221 self.assertEqual(
222 [['key', 'value', 'optional']],
223 list(parse_config_file(config_file)))
224@@ -70,22 +70,22 @@
225 def test_key_value(self):
226 # A (key, value) pair without a third optional value is returned in
227 # the configuration as a dictionary entry under 'key' with '(value,
228- # False)' as its value.
229+ # None, False)' as its value.
230 config = interpret_config([['key', 'value']], False)
231- self.assertEqual({'key': ('value', False)}, config)
232+ self.assertEqual({'key': ('value', None, False)}, config)
233
234 def test_key_value_public_only(self):
235 # A (key, value) pair without a third optional value is returned in
236 # the configuration as a dictionary entry under 'key' with '(value,
237- # False)' as its value when public_only is true.
238+ # None, False)' as its value when public_only is true.
239 config = interpret_config([['key', 'value']], True)
240- self.assertEqual({'key': ('value', False)}, config)
241+ self.assertEqual({'key': ('value', None, False)}, config)
242
243 def test_key_value_optional(self):
244 # A (key, value, optional) entry is returned in the configuration as a
245 # dictionary entry under 'key' with '(value, True)' as its value.
246 config = interpret_config([['key', 'value', 'optional']], False)
247- self.assertEqual({'key': ('value', True)}, config)
248+ self.assertEqual({'key': ('value', None, True)}, config)
249
250 def test_key_value_optional_public_only(self):
251 # A (key, value, optional) entry is not returned in the configuration
252@@ -93,7 +93,33 @@
253 config = interpret_config([['key', 'value', 'optional']], True)
254 self.assertEqual({}, config)
255
256-
257+ def test_key_value_revision(self):
258+ # A (key, value) pair without a third optional value when the
259+ # value has a suffix of ``;revno=[REVISION]`` is returned in the
260+ # configuration as a dictionary entry under 'key' with '(value,
261+ # None, False)' as its value.
262+ config = interpret_config([['key', 'value;revno=45']], False)
263+ self.assertEqual({'key': ('value', '45', False)}, config)
264+
265+ def test_key_value_revision(self):
266+ # A (key, value) pair without a third optional value when the
267+ # value has multiple suffixes of ``;revno=[REVISION]`` raises an
268+ # error.
269+ self.assertRaises(
270+ AssertionError,
271+ interpret_config, [['key', 'value;revno=45;revno=47']], False)
272+
273+ def test_too_many_values(self):
274+ # A line with too many values raises an error.
275+ self.assertRaises(
276+ AssertionError,
277+ interpret_config, [['key', 'value', 'optional', 'extra']], False)
278+
279+ def test_bad_optional_value(self):
280+ # A third value that is not the "optional" string raises an error.
281+ self.assertRaises(
282+ AssertionError,
283+ interpret_config, [['key', 'value', 'extra']], False)
284
285 class TestPlanUpdate(unittest.TestCase):
286 """Tests for how to plan the update."""
287
288=== modified file 'utilities/sourcedeps.conf'
289--- utilities/sourcedeps.conf 2009-10-31 12:07:16 +0000
290+++ utilities/sourcedeps.conf 2009-11-25 16:35:33 +0000
291@@ -1,19 +1,19 @@
292-bzr-git=lp:~launchpad-pqm/bzr-git/devel/
293-bzr-loom=lp:~launchpad-pqm/bzr-loom/trunk/
294-bzr-svn=lp:~launchpad-pqm/bzr-svn/devel/
295-dulwich=lp:~launchpad-pqm/dulwich/devel/
296-launchpad-loggerhead=lp:~launchpad-pqm/launchpad-loggerhead/devel/
297-loggerhead=lp:~launchpad-pqm/loggerhead/devel/
298-mailman=lp:~launchpad-pqm/mailman/2.1/
299-pygpgme=lp:~launchpad-pqm/pygpgme/devel/
300-subunit=lp:~launchpad-pqm/subunit/trunk/
301-subvertpy=lp:~launchpad-pqm/subvertpy/trunk/
302-testresources=lp:~launchpad-pqm/testresources/dev/
303-testtools=lp:~launchpad-pqm/testtools/trunk/
304-twisted=lp:~launchpad-pqm/twisted/trunk/
305-cscvs=lp:~launchpad-pqm/launchpad-cscvs/devel/
306-lpreview=lp:~launchpad-pqm/bzr-lpreview/devel/
307-pygettextpo=lp:~launchpad/pygettextpo/trunk/
308-old_xmlplus=lp:~launchpad/dtdparser/trunk/
309-canonical-identity-provider=lp:~launchpad-pqm/canonical-identity-provider/trunk/=optional
310-shipit=lp:~launchpad-pqm/shipit/trunk/=optional
311+bzr-git lp:~launchpad-pqm/bzr-git/devel;revno=248
312+bzr-loom lp:~launchpad-pqm/bzr-loom/trunk;revno=47
313+bzr-svn lp:~launchpad-pqm/bzr-svn/devel;revno=2706
314+cscvs lp:~launchpad-pqm/launchpad-cscvs/devel;revno=430
315+dulwich lp:~launchpad-pqm/dulwich/devel;revno=414
316+launchpad-loggerhead lp:~launchpad-pqm/launchpad-loggerhead/devel;revno=54
317+loggerhead lp:~launchpad-pqm/loggerhead/devel;revno=173
318+lpreview lp:~launchpad-pqm/bzr-lpreview/devel;revno=23
319+mailman lp:~launchpad-pqm/mailman/2.1;revno=976
320+old_xmlplus lp:~launchpad/dtdparser/trunk;revno=4
321+pygettextpo lp:~launchpad/pygettextpo/trunk;revno=23
322+pygpgme lp:~launchpad-pqm/pygpgme/devel;revno=48
323+subunit lp:~launchpad-pqm/subunit/trunk;revno=61
324+subvertpy lp:~launchpad-pqm/subvertpy/trunk;revno=2040
325+testresources lp:~launchpad-pqm/testresources/dev;revno=16
326+testtools lp:~launchpad-pqm/testtools/trunk;revno=14
327+twisted lp:~launchpad-pqm/twisted/trunk;revno=17
328+canonical-identity-provider lp:~launchpad-pqm/canonical-identity-provider/trunk;revno=8899 optional
329+shipit lp:~launchpad-pqm/shipit/trunk;revno=8900 optional