launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/db-bug-768342 into lp:launchpad/db-devel with lp:~jtv/launchpad/db-bug-735621 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #768342 in Launchpad itself: "New generate-contents should log "live" apt-ftparchive output."
  https://bugs.launchpad.net/launchpad/+bug/768342

For more details, see:
https://code.launchpad.net/~jtv/launchpad/db-bug-768342/+merge/58685

= Summary =

The generate-contents script runs apt-ftparchive, which may take a long time.  It's not very nice to save up all its log output and dump it just once at the end: we want live output.


== Proposed fix ==

Luckily this is all neatly isolated in a function called "execute."  I replaced its subprocess.Popen implementation with one based on CommandSpawner (and its helpful friends for logging and result tracking).


== Pre-implementation notes ==

This was Julian's one gripe from testing the branch for bug 735621, currently landing, which is a requisit for this one.  We couldn't think of any meaningful tests for it, since I just re-implemented an existing, tested function.


== Implementation details ==

CommandSpawner is really meant for running multiple commands in parallel, but there's nothing against using it for just one.


== Tests ==

{{{
./bin/test -vvc lp.archivepublisher.tests.test_generate_contents
}}}


== Demo and Q/A ==

Run the generate-contents script on dogfood (or some other sizable archive) and watch its output.  It won't be exactly per-line granularity because of buffering, but there should be some output from apt-ftparchive before it completes.


= Launchpad lint =

This lint was previously present and is, as far as I can see, inevitable:

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/canonical/config/schema-lazr.conf
  lib/lp/archivepublisher/tests/test_generate_contents_files.py
  lib/lp/archivepublisher/scripts/generate_contents_files.py
  lib/canonical/launchpad/ftests/script.py
  cronscripts/publishing/gen-contents/apt_conf_dist.template
  cronscripts/publishing/gen-contents/Contents.top
  scripts/generate-contents-files.py
  cronscripts/publishing/gen-contents/apt_conf_header.template
  lib/lp/services/command_spawner.py

./lib/canonical/config/schema-lazr.conf
     536: Line exceeds 78 characters.
     619: Line exceeds 78 characters.
     995: Line exceeds 78 characters.
    1084: Line exceeds 78 characters.
./cronscripts/publishing/gen-contents/apt_conf_dist.template
       4: Line exceeds 78 characters.
       5: Line exceeds 78 characters.
./scripts/generate-contents-files.py
       8: '_pythonpath' imported but unused
-- 
https://code.launchpad.net/~jtv/launchpad/db-bug-768342/+merge/58685
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/db-bug-768342 into lp:launchpad/db-devel.

=== modified file 'lib/lp/archivepublisher/scripts/generate_contents_files.py'
--- lib/lp/archivepublisher/scripts/generate_contents_files.py	2011-04-21 14:28:35 +0000
+++ lib/lp/archivepublisher/scripts/generate_contents_files.py	2011-04-21 14:28:37 +0000
@@ -13,9 +13,13 @@
 from zope.component import getUtility
 
 from canonical.config import config
-from canonical.launchpad.ftests.script import run_command
 from lp.archivepublisher.config import getPubConfig
 from lp.registry.interfaces.distribution import IDistributionSet
+from lp.services.command_spawner import (
+    CommandSpawner,
+    OutputLineHandler,
+    ReturnCodeReceiver,
+    )
 from lp.services.scripts.base import (
     LaunchpadScript,
     LaunchpadScriptFailure,
@@ -63,18 +67,31 @@
     """Execute a shell command.
 
     :param logger: Output from the command will be logged here.
-    :param command_line: Command to execute, as a list of tokens.
+    :param command: Command to execute, as a string.
+    :param args: Optional list of arguments for `command`.
     :raises LaunchpadScriptFailure: If the command returns failure.
     """
-    if args is None:
-        description = command
-    else:
-        description = command + ' ' + ' '.join(args)
+    command_line = [command]
+    if args is not None:
+        command_line += args
+    description = ' '.join(command_line)
+
     logger.debug("Execute: %s", description)
-    retval, stdout, stderr = run_command(command, args)
-    logger.debug(stdout)
-    logger.warn(stderr)
-    if retval != 0:
+    # Some of these commands can take a long time.  Use CommandSpawner
+    # and friends to provide "live" log output.  Simpler ways of running
+    # commands tend to save it all up and then dump it at the end, or
+    # have trouble logging it as neat lines.
+    stderr_logger = OutputLineHandler(logger.warn)
+    stdout_logger = OutputLineHandler(logger.debug)
+    receiver = ReturnCodeReceiver()
+    spawner = CommandSpawner()
+    spawner.start(
+        command_line, completion_handler=receiver,
+        stderr_handler=stderr_logger, stdout_handler=stdout_logger)
+    spawner.complete()
+    stdout_logger.finalize()
+    stderr_logger.finalize()
+    if receiver.returncode != 0:
         raise LaunchpadScriptFailure(
             "Failure while running command: %s" % description)
 

=== modified file 'lib/lp/services/command_spawner.py'
--- lib/lp/services/command_spawner.py	2011-04-06 03:43:28 +0000
+++ lib/lp/services/command_spawner.py	2011-04-21 14:28:37 +0000
@@ -99,9 +99,9 @@
         :param stderr_handler: Callback to handle error output received from
             the sub-process.  Must take the output as its sole argument.  May
             be called any number of times as output comes in.
-        :param failure_handler: Callback to be invoked, exactly once, when the
-            sub-process exits.  Must take `command`'s numeric return code as
-            its sole argument.
+        :param completion_handler: Callback to be invoked, exactly once, when
+            the sub-process exits.  Must take `command`'s numeric return code
+            as its sole argument.
         """
         process = self._spawn(command)
         handlers = {
@@ -172,9 +172,9 @@
         try:
             output = pipe_file.read()
         except IOError, e:
+            # "Resource temporarily unavailable"--not an error really,
+            # just means there's nothing to read.
             if e.errno != errno.EAGAIN:
-                # "Resource temporarily unavailable"--not an error
-                # really, just means there's nothing to read.
                 raise
         else:
             if len(output) > 0: