Merge lp://staging/~lifeless/bzr-builder/blocking into lp://staging/~james-w/bzr-builder/trunk-old

This adds the ability to block on a build till its done - useful for integration with test builders.

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 launch...

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 co...

On Fri, 2009-10-23 at 20:09 +0000, Robert Collins wrote:

> 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.

Done these things.

-Rob

On Fri Oct 23 20:09:09 UTC 2009 Robert Collins wrote:
> 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.

I consider most of them to be a 'must', as you are asking me to maintain
the code. I wouldn't block on supporting code that wasn't written yet though.

If you don't want to make the changes then please say so and I will happily
do the work myself when I have some time.

Thanks,

James

I've got this in production now, and had to make further changes so its using trace.note now.