Launchpad itself

Merge lp://staging/~cjwatson/launchpad/always-copy-packages-asynchronously-2 into lp://staging/launchpad

always-copy-packages-asynchronously-2
Merge into devel

Proposed by Colin Watson on 2012-10-29

Status:

Merged

Approved by:

William Grant on 2012-11-13

Approved revision:

no longer in the source branch.

Merged at revision:

16270

Proposed branch:

lp://staging/~cjwatson/launchpad/always-copy-packages-asynchronously-2

Merge into:

lp://staging/launchpad

Diff against target:

1049 lines (+210/-422)

8 files modified

lib/lp/registry/browser/distroseries.py (+1/-5)
lib/lp/registry/browser/tests/test_distroseries.py (+27/-12)
lib/lp/services/features/flags.py (+0/-6)
lib/lp/soyuz/browser/archive.py (+39/-123)
lib/lp/soyuz/browser/tests/archive-views.txt (+8/-59)
lib/lp/soyuz/browser/tests/test_package_copying_mixin.py (+1/-151)
lib/lp/soyuz/model/packagecopyjob.py (+8/-2)
lib/lp/soyuz/stories/ppa/xx-copy-packages.txt (+126/-64)

To merge this branch:

bzr merge lp://staging/~cjwatson/launchpad/always-copy-packages-asynchronously-2

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
William Grant	code	2012-10-29	Approve on 2012-11-13
Review via email: mp+131928@code.staging.launchpad.net

Commit message

Remove the old synchronous package copy path in favour of PackageCopyJobs.

Description of the change

== Summary ==

This is my second attempt at https://code.launchpad.net/~cjwatson/launchpad/always-copy-packages-asynchronously/+merge/131837; Curtis reverted the previous attempt on my behalf since it broke buildbot.

== Proposed fix ==

The complicated bit here was fixing xx-copy-packages.txt, which is a long and horrible doctest built around synchronous copies. The bulk of the fix is fairly mechanical: run copy jobs after each copy operation in the UI. However, to make this work well, I needed to make PCJs say what they're copying in their debug output, as otherwise there was no way to get an accurate idea of which binaries were copied; this seems likely to be occasionally useful elsewhere anyway.

I also noticed that it was a bit odd that the old synchronous path named the target archive in its notification while the new asynchronous path doesn't, and this inconvenienced xx-copy-packages.txt a bit since it wanted to follow that link. I therefore added this to the asynchronous notification.

== Tests ==

bin/test -vvct lp.registry.browser.tests.test_distroseries -t soyuz

== Demo and Q/A ==

Verify that copies between PPAs using the web UI still work.

Revision history for this message

Colin Watson (cjwatson) wrote on 2012-10-29:

Don't review this yet - I made another mistake ...

Revision history for this message

Colin Watson (cjwatson) wrote on 2012-11-12:

OK, this should be worth reviewing again now.

Revision history for this message

William Grant (wgrant) wrote on 2012-11-13:

Unless I'm forgetting something, this eliminates the penultimate delayed copy callsite, the last being the API's Archive.syncSource(s). Do you have a strategy for the final excision of that callsite and the significant volume of called code?

240 + if dest_url is None:
241 + dest_url = escape(
242 + canonical_url(dest_archive) + '/+packages', quote=True)
243 +
244 + if dest_display_name is None:
245 + dest_display_name = escape(dest_archive.displayname)

This will probably double escape them; structured() escapes parameters itself. I'd also avoid talking about the escaping in the docstring, as one wouldn't normally expect a string to be passed through unescaped unless the docstring explicitly asked for HTML.

708 + for copy in copied_publications:
709 + self.logger.debug(copy.displayname)

What does the logging around this look like? AFAICT the job doesn't log much else, so it may seem somewhat of a non sequitur in the output.

review: Approve (code)

Revision history for this message

Colin Watson (cjwatson) wrote on 2012-11-13:

On Tue, Nov 13, 2012 at 05:49:20AM -0000, William Grant wrote:
> Unless I'm forgetting something, this eliminates the penultimate
> delayed copy callsite, the last being the API's Archive.syncSource(s).
> Do you have a strategy for the final excision of that callsite and the
> significant volume of called code?

Not only a strategy but a pending branch that I'll be ready to submit
shortly after this lands.

> This will probably double escape them; structured() escapes parameters
> itself. I'd also avoid talking about the escaping in the docstring, as
> one wouldn't normally expect a string to be passed through unescaped
> unless the docstring explicitly asked for HTML.

This mistake was because compose_synchronous_copy_feedback used
structured() in a slightly different way (with %-formatting rather than
passing separate replacement arguments, so structured() wouldn't have
handled escaping there). Fixed.

> What does the logging around this look like? AFAICT the job doesn't
> log much else, so it may seem somewhat of a non sequitur in the
> output.

I added an extra line of logging to alleviate confusion. (Of course, if
we actually want to see it routinely we'll need to change the PCJ runner
to log at DEBUG; should this be at INFO instead, or is it fine as it
is?)