Slightly simplify the Harbormaster build log API

Summary:
Ref T5822. This prepares for inline compression and garbage collection of build logs.

This reduces the API surface area and removes a log from the "wait" step that would just log a message every 15 seconds. (If this is actually useful, I think we find a better way to communicate it.)

Test Plan:
Ran a build, saw a log:

{F1136691}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5822

Differential Revision: https://secure.phabricator.com/D15371

src/applications/harbormaster/step/HarbormasterWaitForPreviousBuildStepImplementation.php

@@ -31,24 +31,7 @@ final class HarbormasterWaitForPreviousBuildStepImplementation
     // Block until all previous builds of the same build plan have
     // finished.
     $plan = $build->getBuildPlan();
-
-    $existing_logs = id(new HarbormasterBuildLogQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withBuildTargetPHIDs(array($build_target->getPHID()))
-      ->execute();
-
-    if ($existing_logs) {
-      $log = head($existing_logs);
-    } else {
-      $log = $build->createLog($build_target, 'waiting', 'blockers');
-    }
-
     $blockers = $this->getBlockers($object, $plan, $build);
-    if ($blockers) {
-      $log->start();
-      $log->append(pht("Blocked by: %s\n", implode(',', $blockers)));
-      $log->finalize();
-    }
 
     if ($blockers) {
       throw new PhabricatorWorkerYieldException(15);

src/applications/harbormaster/storage/build/HarbormasterBuild.php

@@ -234,23 +234,6 @@ final class HarbormasterBuild extends HarbormasterDAO
     return ($this->getPlanAutoKey() !== null);
   }
 
-  public function createLog(
-    HarbormasterBuildTarget $build_target,
-    $log_source,
-    $log_type) {
-
-    $log_source = id(new PhutilUTF8StringTruncator())
-      ->setMaximumBytes(250)
-      ->truncateString($log_source);
-
-    $log = HarbormasterBuildLog::initializeNewBuildLog($build_target)
-      ->setLogSource($log_source)
-      ->setLogType($log_type)
-      ->save();
-
-    return $log;
-  }
-
   public function retrieveVariablesFromBuild() {
     $results = array(
       'buildable.diff' => null,

src/applications/harbormaster/storage/build/HarbormasterBuildLog.php

@@ -1,6 +1,7 @@
 <?php
 
-final class HarbormasterBuildLog extends HarbormasterDAO
+final class HarbormasterBuildLog
+  extends HarbormasterDAO
   implements PhabricatorPolicyInterface {
 
   protected $buildTargetPHID;
@@ -10,7 +11,6 @@ final class HarbormasterBuildLog extends HarbormasterDAO
   protected $live;
 
   private $buildTarget = self::ATTACHABLE;
-  private $start;
 
   const CHUNK_BYTE_LIMIT = 102400;
 
@@ -20,8 +20,8 @@ final class HarbormasterBuildLog extends HarbormasterDAO
   const ENCODING_TEXT = 'text';
 
   public function __destruct() {
-    if ($this->start) {
-      $this->finalize($this->start);
+    if ($this->getLive()) {
+      $this->closeBuildLog();
     }
   }
 
@@ -34,6 +34,35 @@ final class HarbormasterBuildLog extends HarbormasterDAO
       ->setLive(0);
   }
 
+  public function openBuildLog() {
+    if ($this->getLive()) {
+      throw new Exception(pht('This build log is already open!'));
+    }
+
+    return $this
+      ->setLive(1)
+      ->save();
+  }
+
+  public function closeBuildLog() {
+    if (!$this->getLive()) {
+      throw new Exception(pht('This build log is not open!'));
+    }
+
+    // TODO: Encode the log contents in a gzipped format.
+
+    $this->reload();
+
+    $start = $this->getDateCreated();
+    $now = PhabricatorTime::getNow();
+
+    return $this
+      ->setDuration($now - $start)
+      ->setLive(0)
+      ->save();
+  }
+
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,
@@ -73,26 +102,13 @@ final class HarbormasterBuildLog extends HarbormasterDAO
     return pht('Build Log');
   }
 
-  public function start() {
-    if ($this->getLive()) {
-      throw new Exception(
-        pht('Live logging has already started for this log.'));
-    }
-
-    $this->setLive(1);
-    $this->save();
-
-    $this->start = PhabricatorTime::getNow();
-
-    return time();
-  }
-
   public function append($content) {
     if (!$this->getLive()) {
-      throw new Exception(
-        pht('Start logging before appending data to the log.'));
+      throw new PhutilInvalidStateException('openBuildLog');
     }
-    if (strlen($content) === 0) {
+
+    $content = (string)$content;
+    if (!strlen($content)) {
       return;
     }
 
@@ -152,21 +168,6 @@ final class HarbormasterBuildLog extends HarbormasterDAO
     }
   }
 
-  public function finalize($start = 0) {
-    if (!$this->getLive()) {
-      // TODO: Clean up this API.
-      return;
-    }
-
-    // TODO: Encode the log contents in a gzipped format.
-    $this->reload();
-    if ($start > 0) {
-      $this->setDuration(time() - $start);
-    }
-    $this->setLive(0);
-    $this->save();
-  }
-
   public function getLogText() {
     // TODO: This won't cope very well if we're pulling like a 700MB
     // log file out of the DB. We should probably implement some sort

src/applications/harbormaster/storage/build/HarbormasterBuildTarget.php

@@ -256,9 +256,8 @@ final class HarbormasterBuildTarget extends HarbormasterDAO
 
     $log = HarbormasterBuildLog::initializeNewBuildLog($this)
       ->setLogSource($log_source)
-      ->setLogType($log_type);
-
-    $log->start();
+      ->setLogType($log_type)
+      ->openBuildLog();
 
     return $log;
   }

src/applications/harbormaster/worker/HarbormasterTargetWorker.php

@@ -90,13 +90,10 @@ final class HarbormasterTargetWorker extends HarbormasterWorker {
       $target->setDateCompleted(PhabricatorTime::getNow());
       $target->save();
     } catch (Exception $ex) {
-      phlog($ex);
-
       try {
-        $log = $build->createLog($target, 'core', 'exception');
-        $start = $log->start();
-        $log->append((string)$ex);
-        $log->finalize($start);
+        $log = $target->newLog('core', 'exception')
+          ->append($ex)
+          ->closeBuildLog();
       } catch (Exception $log_ex) {
         phlog($log_ex);
       }