12 === modified file '__init__.py' 13 --- __init__.py 2009-09-25 19:50:30 +0000 14 +++ __init__.py 2009-10-23 07:30:27 +0000 15 @@ -249,17 +249,25 @@ 16 Option("key-id", type=str, short_name="k", 17 help="Sign the packages with the specified GnuPG key, " 18 "must be specified if you use --dput."), 19 + Option("watch-ppa", help="Watch the PPA the package was " 20 + "dput to and exit with 0 only if it builds and " 21 + "publishes successfully."), --dput is planned to become a list option, at the request of Neil who wants to put the package in multiple PPAs. How would this work if there were multiple PPAs to watch? Any chance we could make that change first? (bug 433454) Is the default for an Option action=store_true? 37 + elif dput: 38 + target_from_dput(dput) Is that just a check that the dput target is valid? For the reasons given below I'm not sure I like the restriction on every use, as this will break some people's use. I assume it is there as a "fail fast" check? 46 + if watch_ppa: 47 + from bzrlib.plugins.builder.ppa import watch 48 + target = target_from_dput(dput) 49 + return watch(target, self.package, base_branch.deb_version) Has self.package always been set to something reliable here? Nice API, but it probably needs to take a list of PPAs given the above? Also, it would be nice to parallelise the watches if you are uploading a bunch (cron usecase, rather than hook). We don't have code to use this but would it work to extend the API in that direction so that I could add that feature easily once we do (planned soon)? 52 def _add_changelog_entry(self, base_branch, basedir, distribution=None, 53 @@ -330,6 +342,7 @@ 54 "specified.") 55 if distribution is None: 56 distribution = "jaunty" 57 + self.package = package Ah, so I assume this is the self.package thing I asked about above answered? 65 +def target_from_dput(dput): 66 + """Convert a dput specification to a LP API specification. 67 + 68 + :param dput: A dput command spec like ppa:team-name. 69 + :return: A LP API target like team-name/ppa. 70 + """ 71 + if not dput.startswith('ppa:'): 72 + raise errors.BzrCommandError('not a ppa %s' % dput) I don't like that error. I don't mind only supporting ppa: targets, but there will be some people that have defined a specific target like my-ppa (or even 'ppa'). Having the error be clear about that, and perhaps even explaining how to use ppa: would be great. 73 + base, _, suffix = dput[4:].partition('/') I'd prefer "len('ppa:')" than '4'. 74 + if not suffix: 75 + suffix = 'ppa' 76 + return base + '/' + suffix I'm not entirely sure about the scheme used for ppas, but I'll assume this is correct as you have presumably used it :-) 83 === added file 'ppa.py' 84 --- ppa.py 1970-01-01 00:00:00 +0000 85 +++ ppa.py 2009-10-23 07:30:27 +0000 103 +from optparse import OptionParser 104 +import os 105 +import sys 106 +import time 107 + 108 +from launchpadlib.launchpad import ( 109 + Launchpad, STAGING_SERVICE_ROOT, EDGE_SERVICE_ROOT) Thought about using the stable service? Though you can't rely on the constant being defined. STAGING_SERVICE_ROOT is a leftover import from testing? 112 +def watch(target, package_name, version): 113 + """Watch a package build. 114 + 115 + :return: 0 once the package built and published completely ok or 2 116 + otherwise. 117 + """ Why this return code scheme? 118 + # See https://help.launchpad.net/API 119 + credentials = Credentials() 120 + oauth_file = os.path.expanduser('~/.cache/edge_oauth.txt') Should be something specific to bzr-builder shouldn't it? 121 + try: 122 + credentials.load(open(oauth_file)) 123 + launchpad = Launchpad(credentials, EDGE_SERVICE_ROOT) 124 + except: 125 + cachedir = os.path.expanduser("~/.launchpadlib/cache/") 126 + launchpad = Launchpad.get_token_and_login('get-build-status', EDGE_SERVICE_ROOT, cachedir) 127 + launchpad.credentials.save(file(oauth_file, "w")) I don't like the bare except. Later launchpadlib have methods to aid with this sort of thing (login_with IIRC), though I disagree with their defaults, and I'm not sure they can be overridden. I also don't like the file(), I always use a try...finally block with open. Why specify a cachedir in the except but not the try? 129 + try: 130 + owner_name, archive_name = target.split('/', 2) 131 + except ValueError: 132 + print "E: Failed to parse archive identifier." 133 + print "Syntax of target archive: /" 134 + sys.exit(1) sys.exit? Why not raise an exception? Also, why does dput_from_ppa put the parts back together just to have this take them apart again? 136 + owner = launchpad.people[owner_name] Do we want some KeyError checks and the like? 137 + archive = owner.getPPAByName(name=archive_name) 138 + end_states = ['failedtobuild', 'fullybuilt'] 139 + important_arches = ['amd64', 'i386', 'lpia', 'armel'] 140 + print "Waiting for", version, "of", package_name, "to build." Bare prints? Why not use bzrlib.trace? 141 + while True: 142 + sourceRecords = [s for s in 143 + archive.getPublishedSources(source_name=package_name)] 144 + # print [s.source_package_version for s in sourceRecords] 145 + sourceRecords = [s for s in sourceRecords 146 + if s.source_package_version == version] Why not use the version filter in getPublishedSources? 147 + if not sourceRecords: 148 + time.sleep(60) 149 + continue Infinite loop if there was a bad signature on the upload? 150 + pkg = sourceRecords[0] 151 + if pkg.status.lower() not in ('published', 'pending'): 152 + time.sleep(60) 153 + continue 154 + source_id = str(pkg.self).rsplit('/', 1)[1] 155 + buildSummaries = archive.getBuildSummariesForSourceIds( 156 + source_ids=[source_id])[source_id] 157 + if buildSummaries['status'] in end_states: 158 + break 159 + if buildSummaries['status'] == 'NEEDSBUILD': 160 + # We ignore non-virtual PPA architectures that are sparsely 161 + # supplied with buildds. 162 + missing = [] 163 + for build in buildSummaries['builds']: 164 + arch = build['arch_tag'] 165 + if arch in important_arches: 166 + missing.append(arch) 167 + if not missing: 168 + break 169 + extra = ', '.join(missing) 170 + else: 171 + extra = '' 172 + print "%s: %s" % (pkg.display_name, buildSummaries['status']), extra 173 + time.sleep(60) 174 + print "%s: %s" % (pkg.display_name, buildSummaries['status']) Bare prints again. 208 === added file 'tests/test_ppa.py' 209 --- tests/test_ppa.py 1970-01-01 00:00:00 +0000 210 +++ tests/test_ppa.py 2009-10-23 07:30:27 +0000 211 @@ -0,0 +1,31 @@ 212 +# bzr-builder: a bzr plugin to construct trees based on recipes 213 +# Copyright 2009 Canonical Ltd. 214 + 215 +# This program is free software: you can redistribute it and/or modify it 216 +# under the terms of the GNU General Public License version 3, as published 217 +# by the Free Software Foundation. 218 + 219 +# This program is distributed in the hope that it will be useful, but 220 +# WITHOUT ANY WARRANTY; without even the implied warranties of 221 +# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR 222 +# PURPOSE. See the GNU General Public License for more details. 223 + 224 +# You should have received a copy of the GNU General Public License along 225 +# with this program. If not, see . 226 + 227 +import os 228 + 229 +from bzrlib import workingtree 230 +from bzrlib.plugins.builder import target_from_dput 231 +from bzrlib.tests import ( 232 + TestCase, 233 + ) 234 + 235 + 236 +class TestTargetFromDPut(TestCase): 237 + 238 + def test_default_ppa(self): 239 + self.assertEqual('team-name/ppa', target_from_dput('ppa:team-name')) 240 + 241 + def test_named_ppa(self): 242 + self.assertEqual('team/ppa2', target_from_dput('ppa:team/ppa2')) There are no 3 part ppa names tested here. Overall I have no problem with this going in when fixed up, thanks for your work. Thanks, James