On Fri, 2009-10-23 at 10:48 +0000, James Westby wrote: > Review: Resubmit > > 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) Well, this is written, and that isn't. By written I mean "I'm using it" :). I'd either watch all the PPA's, serially, or make the options incompatible. > Is the default for an Option action=store_true? Yes. > 37 + elif dput: > 38 + target_from_dput(dput) > > Is that just a check that the dput target is valid? valid-to-watch, yes - so that we don't upload something we can't watch. It can be bypassed of course, IFF we are not going to try to watch. > 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? Yes, as long as we've made a changelog entry. > Nice API, but it probably needs to take a list of PPAs given the above? I don't think so - a loop at this level is fine. > 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)? Personally, I'd loop, unless you really need 'fail as soon as any FTB. If you need that then yes, you need to push the triple down into the watch routine and split that up more. > 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. Feel free to make the text longer when you merge :) [round trips cost, so we should only round trip on merge changes when the submitter has to make changes anyhow]. > 73 + base, _, suffix = dput[4:].partition('/') > > I'd prefer "len('ppa:')" than '4'. I'd put a comment there then; calculating constants to do offsets is a waste of CPU - and after all the time figuring out how to make python run fast, I'm very much _not_ interested in unlearning the muscle memory. > 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 :-) it works :P > 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? Code Cody gave me. > 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? Its what Command.run needs > 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? I have no idea about credential management convention in the LP. AFAIK we only need read only access etc. Given that the oath files in .cache are readable by any application its a bit of a joke to use separate files... > 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? Cody's code. I don't know. > 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? Cody's code, probably should raise an exception. > Also, why does dput_from_ppa put the parts back together just to have > this take them apart again? Cody's code.. there is a theme here. I'm going to stop commenting until its something I added and can comment on. > 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? Or it disappears, yes. This probably should be changed to check time.time() for (say) 15 minutes. > 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. We're in a UI module, and Debian policy means package names are ascii, and finally all the status codes are ascii. Seems ok to me. > 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. What is a 3 part ppa name? > Overall I have no problem with this going in when fixed up, thanks > for your work. I'd appreciate it if you can go over this mail and categorise 'must' vs 'should' - most of the support code for LP API's is cargo culted and repeated in other lp tools - really it should be factored into launchpadlib itself, so I don't think polishing it here is a good use of anyones time. The only things I saw that I would consider a must are changing the early check on dput parsing, the better error message for 'not a ppa', and adding a timeout on not finding source records - everything else will communicate what is going on to the user adequately and either won't be made appreciably better, or could be made worse by changing. (For instance, changing the bare except will cause any number of unknown exceptions to float past - and if I had a test harness where I could diagnose those, I would be delighted to change it, but I don't, so other than making it 'except Exception:' [happy to do that]) I'd rather not touch it. -Rob