From 7aab7e80af28f2faadb93cfc3b63a70cc8d59629 Mon Sep 17 00:00:00 2001 From: Mike Riley Date: Sun, 25 Feb 2018 20:12:30 +0000 Subject: [PATCH 01/39] Provide default values for table view properties which are `count`ed Summary: PHP 7.2 has changed the behavior of `count`, you must provide an array or `Countable` as a parameter, otherwise a warning is generated. These two class members are counted during rendering, and are commonly left as null properties. https://wiki.php.net/rfc/counting_non_countables Test Plan: Browsed around my install and stopped seeing `count(): Parameter must be an array or an object that implements Countable at [AphrontTableView.php:153]` everywhere. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D19140 --- src/view/control/AphrontTableView.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/view/control/AphrontTableView.php b/src/view/control/AphrontTableView.php index 176d246718..3cca7f89e5 100644 --- a/src/view/control/AphrontTableView.php +++ b/src/view/control/AphrontTableView.php @@ -4,7 +4,7 @@ final class AphrontTableView extends AphrontView { protected $data; protected $headers; - protected $shortHeaders; + protected $shortHeaders = array(); protected $rowClasses = array(); protected $columnClasses = array(); protected $cellClasses = array(); @@ -21,7 +21,7 @@ final class AphrontTableView extends AphrontView { protected $sortParam; protected $sortSelected; protected $sortReverse; - protected $sortValues; + protected $sortValues = array(); private $deviceReadyTable; public function __construct(array $data) { From cd4c4dc2ff9e6c096c011077bef61359657fa39f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 18:01:51 -0800 Subject: [PATCH 02/39] Add `bin/harbormaster write-log` to write some arbitrary content into a new Harbormaster log Summary: Ref T13088. This is currently minimal but the modify-execute development loop on build logs is extremely long without it. Test Plan: Ran `echo hi | ./bin/harbormaster write-log --target 12345`, saw the log show up in the web UI. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19130 --- src/__phutil_library_map__.php | 2 + ...HarbormasterManagementWriteLogWorkflow.php | 62 +++++++++++++++++++ 2 files changed, 64 insertions(+) create mode 100644 src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 4455ad139a..2d610818e5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1311,6 +1311,7 @@ phutil_register_library_map(array( 'HarbormasterManagementRestartWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRestartWorkflow.php', 'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php', 'HarbormasterManagementWorkflow' => 'applications/harbormaster/management/HarbormasterManagementWorkflow.php', + 'HarbormasterManagementWriteLogWorkflow' => 'applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php', 'HarbormasterMessageType' => 'applications/harbormaster/engine/HarbormasterMessageType.php', 'HarbormasterObject' => 'applications/harbormaster/storage/HarbormasterObject.php', 'HarbormasterOtherBuildStepGroup' => 'applications/harbormaster/stepgroup/HarbormasterOtherBuildStepGroup.php', @@ -6614,6 +6615,7 @@ phutil_register_library_map(array( 'HarbormasterManagementRestartWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementWorkflow' => 'PhabricatorManagementWorkflow', + 'HarbormasterManagementWriteLogWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterMessageType' => 'Phobject', 'HarbormasterObject' => 'HarbormasterDAO', 'HarbormasterOtherBuildStepGroup' => 'HarbormasterBuildStepGroup', diff --git a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php new file mode 100644 index 0000000000..d84a9ed510 --- /dev/null +++ b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php @@ -0,0 +1,62 @@ +setName('write-log') + ->setExamples('**write-log** --target __id__ [__options__]') + ->setSynopsis(pht('Write a new Harbormaster build log.')) + ->setArguments( + array( + array( + 'name' => 'target', + 'param' => 'id', + 'help' => pht( + 'Build Target ID to attach the log to.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $target_id = $args->getArg('target'); + if (!$target_id) { + throw new PhutilArgumentUsageException( + pht('Choose a build target to attach the log to with "--target".')); + } + + $target = id(new HarbormasterBuildTargetQuery()) + ->setViewer($viewer) + ->withIDs(array($target_id)) + ->executeOne(); + if (!$target) { + throw new PhutilArgumentUsageException( + pht( + 'Unable to load build target "%s".', + $target_id)); + } + + + $log = HarbormasterBuildLog::initializeNewBuildLog($target); + $log->openBuildLog(); + + echo tsprintf( + "%s\n", + pht('Reading log from stdin...')); + + $content = file_get_contents('php://stdin'); + $log->append($content); + + $log->closeBuildLog(); + + echo tsprintf( + "%s\n", + pht('Done.')); + + return 0; + } + +} From 32c6b649dd211f3b2316750371c27dffe7342631 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 18:14:59 -0800 Subject: [PATCH 03/39] Move Harbormaster log compression to the worker task queue Summary: Depends on D19130. Ref T13088. Currently, when a build log is closed we compress it in the same process. Separate this out into a dedicated worker since the plan is to do a lot more work during finalization, none of which needs to happen inline during builds (or, particuarly, inline during a Conduit call for API writes in the future). Test Plan: Ran `bin/harbormaster write-log --trace`, saw compression run inline. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19131 --- src/__phutil_library_map__.php | 2 + ...HarbormasterManagementWriteLogWorkflow.php | 7 ++- .../storage/build/HarbormasterBuildLog.php | 19 ++++--- .../worker/HarbormasterLogWorker.php | 50 +++++++++++++++++++ 4 files changed, 71 insertions(+), 7 deletions(-) create mode 100644 src/applications/harbormaster/worker/HarbormasterLogWorker.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2d610818e5..d43f386009 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1306,6 +1306,7 @@ phutil_register_library_map(array( 'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'applications/harbormaster/step/HarbormasterLeaseWorkingCopyBuildStepImplementation.php', 'HarbormasterLintMessagesController' => 'applications/harbormaster/controller/HarbormasterLintMessagesController.php', 'HarbormasterLintPropertyView' => 'applications/harbormaster/view/HarbormasterLintPropertyView.php', + 'HarbormasterLogWorker' => 'applications/harbormaster/worker/HarbormasterLogWorker.php', 'HarbormasterManagementArchiveLogsWorkflow' => 'applications/harbormaster/management/HarbormasterManagementArchiveLogsWorkflow.php', 'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php', 'HarbormasterManagementRestartWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRestartWorkflow.php', @@ -6610,6 +6611,7 @@ phutil_register_library_map(array( 'HarbormasterLeaseWorkingCopyBuildStepImplementation' => 'HarbormasterBuildStepImplementation', 'HarbormasterLintMessagesController' => 'HarbormasterController', 'HarbormasterLintPropertyView' => 'AphrontView', + 'HarbormasterLogWorker' => 'HarbormasterWorker', 'HarbormasterManagementArchiveLogsWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementRestartWorkflow' => 'HarbormasterManagementWorkflow', diff --git a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php index d84a9ed510..b20204670b 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php @@ -39,7 +39,6 @@ final class HarbormasterManagementWriteLogWorkflow $target_id)); } - $log = HarbormasterBuildLog::initializeNewBuildLog($target); $log->openBuildLog(); @@ -50,6 +49,12 @@ final class HarbormasterManagementWriteLogWorkflow $content = file_get_contents('php://stdin'); $log->append($content); + echo tsprintf( + "%s\n", + pht('Write completed. Closing log...')); + + PhabricatorWorker::setRunAllTasksInProcess(true); + $log->closeBuildLog(); echo tsprintf( diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 290b288c6c..2826f445de 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -52,17 +52,24 @@ final class HarbormasterBuildLog throw new Exception(pht('This build log is not open!')); } - if ($this->canCompressLog()) { - $this->compressLog(); - } - $start = $this->getDateCreated(); $now = PhabricatorTime::getNow(); - return $this + $this ->setDuration($now - $start) ->setLive(0) ->save(); + + PhabricatorWorker::scheduleTask( + 'HarbormasterLogWorker', + array( + 'logPHID' => $this->getPHID(), + ), + array( + 'objectPHID' => $this->getPHID(), + )); + + return $this; } @@ -201,7 +208,7 @@ final class HarbormasterBuildLog return implode('', $full_text); } - private function canCompressLog() { + public function canCompressLog() { return function_exists('gzdeflate'); } diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php new file mode 100644 index 0000000000..a55420bc2f --- /dev/null +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -0,0 +1,50 @@ +getViewer(); + + $data = $this->getTaskData(); + $log_phid = idx($data, 'logPHID'); + + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withPHIDs(array($log_phid)) + ->executeOne(); + if (!$log) { + throw new PhabricatorWorkerPermanentFailureException( + pht('Invalid build log PHID "%s".', $log_phid)); + } + + $phid_key = PhabricatorHash::digestToLength($log_phid, 14); + $lock_key = "build.log({$phid_key})"; + $lock = PhabricatorGlobalLock::newLock($lock_key); + + try { + $lock->lock(); + } catch (PhutilLockException $ex) { + throw new PhabricatorWorkerYieldException(15); + } + + $caught = null; + try { + $this->finalizeBuildLog($log); + } catch (Exception $ex) { + $caught = $ex; + } + + $lock->unlock(); + + if ($caught) { + throw $caught; + } + } + + private function finalizeBuildLog(HarbormasterBuildLog $log) { + if ($log->canCompressLog()) { + $log->compressLog(); + } + } + +} From 8a2604cf06a8f3d2178c1311b8aa25917ad27a1d Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 18:22:59 -0800 Subject: [PATCH 04/39] Add a "filePHID" to HarbormasterBuildLog and copy logs into Files during finalization Summary: Depends on D19131. Ref T13088. During log finalization, stream the log into Files to support "Download Log", archive to Files, and API access. Test Plan: Ran `write-log` and `rebuild-log`, saw Files objects generate with log content and appropriate permissions. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19132 --- .../autopatches/20180222.log.01.filephid.sql | 2 + src/__phutil_library_map__.php | 2 + ...rbormasterManagementRebuildLogWorkflow.php | 54 ++++++++++++++ ...HarbormasterManagementWriteLogWorkflow.php | 8 ++- .../phid/HarbormasterBuildLogPHIDType.php | 2 + .../storage/build/HarbormasterBuildLog.php | 26 ++++--- .../HarbormasterBuildLogChunkIterator.php | 12 +++- .../worker/HarbormasterLogWorker.php | 70 ++++++++++++++++--- 8 files changed, 153 insertions(+), 23 deletions(-) create mode 100644 resources/sql/autopatches/20180222.log.01.filephid.sql create mode 100644 src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php diff --git a/resources/sql/autopatches/20180222.log.01.filephid.sql b/resources/sql/autopatches/20180222.log.01.filephid.sql new file mode 100644 index 0000000000..a7ef2f2b3e --- /dev/null +++ b/resources/sql/autopatches/20180222.log.01.filephid.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildlog + ADD filePHID VARBINARY(64); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d43f386009..f22f7f6730 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1309,6 +1309,7 @@ phutil_register_library_map(array( 'HarbormasterLogWorker' => 'applications/harbormaster/worker/HarbormasterLogWorker.php', 'HarbormasterManagementArchiveLogsWorkflow' => 'applications/harbormaster/management/HarbormasterManagementArchiveLogsWorkflow.php', 'HarbormasterManagementBuildWorkflow' => 'applications/harbormaster/management/HarbormasterManagementBuildWorkflow.php', + 'HarbormasterManagementRebuildLogWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php', 'HarbormasterManagementRestartWorkflow' => 'applications/harbormaster/management/HarbormasterManagementRestartWorkflow.php', 'HarbormasterManagementUpdateWorkflow' => 'applications/harbormaster/management/HarbormasterManagementUpdateWorkflow.php', 'HarbormasterManagementWorkflow' => 'applications/harbormaster/management/HarbormasterManagementWorkflow.php', @@ -6614,6 +6615,7 @@ phutil_register_library_map(array( 'HarbormasterLogWorker' => 'HarbormasterWorker', 'HarbormasterManagementArchiveLogsWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementBuildWorkflow' => 'HarbormasterManagementWorkflow', + 'HarbormasterManagementRebuildLogWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementRestartWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementUpdateWorkflow' => 'HarbormasterManagementWorkflow', 'HarbormasterManagementWorkflow' => 'PhabricatorManagementWorkflow', diff --git a/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php new file mode 100644 index 0000000000..8644bb4515 --- /dev/null +++ b/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php @@ -0,0 +1,54 @@ +setName('rebuild-log') + ->setExamples('**rebuild-log** --id __id__ [__options__]') + ->setSynopsis( + pht( + 'Rebuild the file and summary for a log. This is primarily '. + 'intended to make it easier to develop new log summarizers.')) + ->setArguments( + array( + array( + 'name' => 'id', + 'param' => 'id', + 'help' => pht('Log to rebuild.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $log_id = $args->getArg('id'); + if (!$log_id) { + throw new PhutilArgumentUsageException( + pht('Choose a build log to rebuild with "--id".')); + } + + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withIDs(array($log_id)) + ->executeOne(); + if (!$log) { + throw new PhutilArgumentUsageException( + pht( + 'Unable to load build log "%s".', + $log_id)); + } + + PhabricatorWorker::setRunAllTasksInProcess(true); + $log->scheduleRebuild(true); + + echo tsprintf( + "%s\n", + pht('Done.')); + + return 0; + } + +} diff --git a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php index b20204670b..367fe4fc78 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php @@ -7,14 +7,16 @@ final class HarbormasterManagementWriteLogWorkflow $this ->setName('write-log') ->setExamples('**write-log** --target __id__ [__options__]') - ->setSynopsis(pht('Write a new Harbormaster build log.')) + ->setSynopsis( + pht( + 'Write a new Harbormaster build log. This is primarily intended '. + 'to make development and testing easier.')) ->setArguments( array( array( 'name' => 'target', 'param' => 'id', - 'help' => pht( - 'Build Target ID to attach the log to.'), + 'help' => pht('Build Target ID to attach the log to.'), ), )); } diff --git a/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php index c0fba81c43..dd110556b1 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php @@ -31,6 +31,8 @@ final class HarbormasterBuildLogPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $build_log = $objects[$phid]; + + $handle->setName(pht('Build Log %d', $build_log->getID())); } } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 2826f445de..ca8f0bb9d5 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -9,12 +9,13 @@ final class HarbormasterBuildLog protected $logType; protected $duration; protected $live; + protected $filePHID; private $buildTarget = self::ATTACHABLE; private $rope; private $isOpen; - const CHUNK_BYTE_LIMIT = 102400; + const CHUNK_BYTE_LIMIT = 1048576; public function __construct() { $this->rope = new PhutilRope(); @@ -60,18 +61,22 @@ final class HarbormasterBuildLog ->setLive(0) ->save(); - PhabricatorWorker::scheduleTask( - 'HarbormasterLogWorker', - array( - 'logPHID' => $this->getPHID(), - ), - array( - 'objectPHID' => $this->getPHID(), - )); + $this->scheduleRebuild(false); return $this; } + public function scheduleRebuild($force) { + PhabricatorWorker::scheduleTask( + 'HarbormasterLogWorker', + array( + 'logPHID' => $this->getPHID(), + 'force' => $force, + ), + array( + 'objectPHID' => $this->getPHID(), + )); + } protected function getConfiguration() { return array( @@ -85,6 +90,7 @@ final class HarbormasterBuildLog 'duration' => 'uint32?', 'live' => 'bool', + 'filePHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_buildtarget' => array( @@ -180,7 +186,7 @@ final class HarbormasterBuildLog public function newChunkIterator() { return id(new HarbormasterBuildLogChunkIterator($this)) - ->setPageSize(32); + ->setPageSize(8); } private function loadLastChunkInfo() { diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLogChunkIterator.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLogChunkIterator.php index 754248cc67..514ad7013c 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLogChunkIterator.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLogChunkIterator.php @@ -5,6 +5,7 @@ final class HarbormasterBuildLogChunkIterator private $log; private $cursor; + private $asString; private $min = 0; private $max = PHP_INT_MAX; @@ -27,6 +28,11 @@ final class HarbormasterBuildLogChunkIterator return $this; } + public function setAsString($as_string) { + $this->asString = $as_string; + return $this; + } + protected function loadPage() { if ($this->cursor > $this->max) { return array(); @@ -43,7 +49,11 @@ final class HarbormasterBuildLogChunkIterator $this->cursor = last($results)->getID() + 1; } - return $results; + if ($this->asString) { + return mpull($results, 'getChunkDisplayText'); + } else { + return $results; + } } } diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index a55420bc2f..8eef7a6b6e 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -8,15 +8,6 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $data = $this->getTaskData(); $log_phid = idx($data, 'logPHID'); - $log = id(new HarbormasterBuildLogQuery()) - ->setViewer($viewer) - ->withPHIDs(array($log_phid)) - ->executeOne(); - if (!$log) { - throw new PhabricatorWorkerPermanentFailureException( - pht('Invalid build log PHID "%s".', $log_phid)); - } - $phid_key = PhabricatorHash::digestToLength($log_phid, 14); $lock_key = "build.log({$phid_key})"; $lock = PhabricatorGlobalLock::newLock($lock_key); @@ -29,6 +20,25 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $caught = null; try { + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withPHIDs(array($log_phid)) + ->executeOne(); + if (!$log) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Invalid build log PHID "%s".', + $log_phid)); + } + + if ($log->getLive()) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Log "%s" is still live. Logs can not be finalized until '. + 'they have closed.', + $log_phid)); + } + $this->finalizeBuildLog($log); } catch (Exception $ex) { $caught = $ex; @@ -42,9 +52,51 @@ final class HarbormasterLogWorker extends HarbormasterWorker { } private function finalizeBuildLog(HarbormasterBuildLog $log) { + $viewer = $this->getViewer(); + + $data = $this->getTaskData(); + $is_force = idx($data, 'force'); + if ($log->canCompressLog()) { $log->compressLog(); } + + if ($is_force) { + $file_phid = $log->getFilePHID(); + if ($file_phid) { + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if ($file) { + id(new PhabricatorDestructionEngine()) + ->destroyObject($file); + } + $log + ->setFilePHID(null) + ->save(); + } + } + + if (!$log->getFilePHID()) { + $iterator = $log->newChunkIterator() + ->setAsString(true); + + $source = id(new PhabricatorIteratorFileUploadSource()) + ->setName('harbormaster-log-'.$log->getID().'.log') + ->setViewPolicy(PhabricatorPolicies::POLICY_NOONE) + ->setMIMEType('application/octet-stream') + ->setIterator($iterator); + + $file = $source->uploadFile(); + + $file->attachToObject($log->getPHID()); + + $log + ->setFilePHID($file->getPHID()) + ->save(); + } + } } From 9b4295ed6051aff6bcef406db3ea75b9f41e32eb Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 19:16:45 -0800 Subject: [PATCH 05/39] Add a very basic standalone view for build logs with a "Download Log" button Summary: Depends on D19132. Ref T13088. This implements an extremely skeletal dedicated log page with a more-or-less functional "Download Log" button. Test Plan: Downloaded a recent log. Tried to download an old (un-finalized) log, couldn't. Used `bin/harbormaster write-log` to get a convenient standalone link to a log. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19133 --- src/__phutil_library_map__.php | 6 ++ .../PhabricatorHarbormasterApplication.php | 4 ++ ...HarbormasterBuildLogDownloadController.php | 62 +++++++++++++++++++ .../HarbormasterBuildLogViewController.php | 44 +++++++++++++ ...HarbormasterManagementWriteLogWorkflow.php | 7 ++- .../phid/HarbormasterBuildLogPHIDType.php | 4 +- .../storage/build/HarbormasterBuildLog.php | 5 ++ .../view/HarbormasterBuildLogView.php | 45 ++++++++++++++ 8 files changed, 175 insertions(+), 2 deletions(-) create mode 100644 src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php create mode 100644 src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php create mode 100644 src/applications/harbormaster/view/HarbormasterBuildLogView.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f22f7f6730..451305d7fb 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1227,8 +1227,11 @@ phutil_register_library_map(array( 'HarbormasterBuildLog' => 'applications/harbormaster/storage/build/HarbormasterBuildLog.php', 'HarbormasterBuildLogChunk' => 'applications/harbormaster/storage/build/HarbormasterBuildLogChunk.php', 'HarbormasterBuildLogChunkIterator' => 'applications/harbormaster/storage/build/HarbormasterBuildLogChunkIterator.php', + 'HarbormasterBuildLogDownloadController' => 'applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php', 'HarbormasterBuildLogPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php', 'HarbormasterBuildLogQuery' => 'applications/harbormaster/query/HarbormasterBuildLogQuery.php', + 'HarbormasterBuildLogView' => 'applications/harbormaster/view/HarbormasterBuildLogView.php', + 'HarbormasterBuildLogViewController' => 'applications/harbormaster/controller/HarbormasterBuildLogViewController.php', 'HarbormasterBuildMessage' => 'applications/harbormaster/storage/HarbormasterBuildMessage.php', 'HarbormasterBuildMessageQuery' => 'applications/harbormaster/query/HarbormasterBuildMessageQuery.php', 'HarbormasterBuildPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildPHIDType.php', @@ -6511,8 +6514,11 @@ phutil_register_library_map(array( ), 'HarbormasterBuildLogChunk' => 'HarbormasterDAO', 'HarbormasterBuildLogChunkIterator' => 'PhutilBufferedIterator', + 'HarbormasterBuildLogDownloadController' => 'HarbormasterController', 'HarbormasterBuildLogPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildLogView' => 'AphrontView', + 'HarbormasterBuildLogViewController' => 'HarbormasterController', 'HarbormasterBuildMessage' => array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', diff --git a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php index ed164b2d3b..b4f62d8e22 100644 --- a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php +++ b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php @@ -96,6 +96,10 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication { 'circleci/' => 'HarbormasterCircleCIHookController', 'buildkite/' => 'HarbormasterBuildkiteHookController', ), + 'log/' => array( + 'view/(?P\d+)/' => 'HarbormasterBuildLogViewController', + 'download/(?P\d+)/' => 'HarbormasterBuildLogDownloadController', + ), ), ); } diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php new file mode 100644 index 0000000000..6bdd04edc3 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php @@ -0,0 +1,62 @@ +getRequest(); + $viewer = $request->getUser(); + + $id = $request->getURIData('id'); + + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$log) { + return new Aphront404Response(); + } + + $cancel_uri = $log->getURI(); + $file_phid = $log->getFilePHID(); + + if (!$file_phid) { + return $this->newDialog() + ->setTitle(pht('Log Not Finalized')) + ->appendParagraph( + pht( + 'Logs must be fully written and processed before they can be '. + 'downloaded. This log is still being written or processed.')) + ->addCancelButton($cancel_uri, pht('Wait Patiently')); + } + + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if (!$file) { + return $this->newDialog() + ->setTitle(pht('Unable to Load File')) + ->appendParagraph( + pht( + 'Unable to load the file for this log. The file may have been '. + 'destroyed.')) + ->addCancelButton($cancel_uri); + } + + $size = $file->getByteSize(); + + return $this->newDialog() + ->setTitle(pht('Download Build Log')) + ->appendParagraph( + pht( + 'This log has a total size of %s. If you insist, you may '. + 'download it.', + phutil_tag('strong', array(), phutil_format_bytes($size)))) + ->setDisableWorkflowOnSubmit(true) + ->addSubmitButton(pht('Download Log')) + ->setSubmitURI($file->getDownloadURI()) + ->addCancelButton($cancel_uri, pht('Done')); + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php new file mode 100644 index 0000000000..084c48d1f1 --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php @@ -0,0 +1,44 @@ +getRequest(); + $viewer = $request->getUser(); + + $id = $request->getURIData('id'); + + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$log) { + return new Aphront404Response(); + } + + $page_title = pht('Build Log %d', $log->getID()); + + $log_view = id(new HarbormasterBuildLogView()) + ->setViewer($viewer) + ->setBuildLog($log); + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb(pht('Build Logs')) + ->addTextCrumb($page_title) + ->setBorder(true); + + $page_header = id(new PHUIHeaderView()) + ->setHeader($page_title); + + $page_view = id(new PHUITwoColumnView()) + ->setHeader($page_header) + ->setFooter($log_view); + + return $this->newPage() + ->setTitle($page_title) + ->setCrumbs($crumbs) + ->appendChild($page_view); + } + +} diff --git a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php index 367fe4fc78..b680b382aa 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementWriteLogWorkflow.php @@ -44,9 +44,14 @@ final class HarbormasterManagementWriteLogWorkflow $log = HarbormasterBuildLog::initializeNewBuildLog($target); $log->openBuildLog(); + echo tsprintf( + "%s\n\n __%s__\n\n", + pht('Opened a new build log:'), + PhabricatorEnv::getURI($log->getURI())); + echo tsprintf( "%s\n", - pht('Reading log from stdin...')); + pht('Reading log content from stdin...')); $content = file_get_contents('php://stdin'); $log->append($content); diff --git a/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php b/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php index dd110556b1..a2fa58fe79 100644 --- a/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php +++ b/src/applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php @@ -32,7 +32,9 @@ final class HarbormasterBuildLogPHIDType extends PhabricatorPHIDType { foreach ($handles as $phid => $handle) { $build_log = $objects[$phid]; - $handle->setName(pht('Build Log %d', $build_log->getID())); + $handle + ->setName(pht('Build Log %d', $build_log->getID())) + ->setURI($build_log->getURI()); } } diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index ca8f0bb9d5..17249313eb 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -290,6 +290,11 @@ final class HarbormasterBuildLog ->save(); } + public function getURI() { + $id = $this->getID(); + return "/harbormaster/log/view/{$id}/"; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/view/HarbormasterBuildLogView.php b/src/applications/harbormaster/view/HarbormasterBuildLogView.php new file mode 100644 index 0000000000..7543524285 --- /dev/null +++ b/src/applications/harbormaster/view/HarbormasterBuildLogView.php @@ -0,0 +1,45 @@ +log = $log; + return $this; + } + + public function getBuildLog() { + return $this->log; + } + + public function render() { + $viewer = $this->getViewer(); + $log = $this->getBuildLog(); + $id = $log->getID(); + + $header = id(new PHUIHeaderView()) + ->setViewer($viewer) + ->setHeader(pht('Build Log %d', $id)); + + $download_uri = "/harbormaster/log/download/{$id}/"; + + $download_button = id(new PHUIButtonView()) + ->setTag('a') + ->setHref($download_uri) + ->setIcon('fa-download') + ->setDisabled(!$log->getFilePHID()) + ->setWorkflow(true) + ->setText(pht('Download Log')); + + $header->addActionLink($download_button); + + $box_view = id(new PHUIObjectBoxView()) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setHeader($header) + ->appendChild('...'); + + return $box_view; + } + +} From e920e2b143a2d12b1bbafabcc7ec0a282e54e322 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 05:12:57 -0800 Subject: [PATCH 06/39] Implement DestructibleInterface on BuildLog Summary: Depends on D19133. Ref T13088. Allows build logs to be formally destroyed, cleaning up their chunks and file data. Test Plan: - Used `bin/remove destroy` to destroy a log, verified chunks and files were removed. - Used `bin/harbormaster rebuild-log` to force a log to rebuild, verified files were destroyed and regenerated. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19134 --- src/__phutil_library_map__.php | 1 + .../storage/build/HarbormasterBuildLog.php | 54 ++++++++++++++++++- .../worker/HarbormasterLogWorker.php | 15 +----- 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 451305d7fb..be9cb258cc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -6511,6 +6511,7 @@ phutil_register_library_map(array( 'HarbormasterBuildLog' => array( 'HarbormasterDAO', 'PhabricatorPolicyInterface', + 'PhabricatorDestructibleInterface', ), 'HarbormasterBuildLogChunk' => 'HarbormasterDAO', 'HarbormasterBuildLogChunkIterator' => 'PhutilBufferedIterator', diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 17249313eb..8b8fac48f2 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -2,7 +2,9 @@ final class HarbormasterBuildLog extends HarbormasterDAO - implements PhabricatorPolicyInterface { + implements + PhabricatorPolicyInterface, + PhabricatorDestructibleInterface { protected $buildTargetPHID; protected $logSource; @@ -317,7 +319,55 @@ final class HarbormasterBuildLog public function describeAutomaticCapability($capability) { return pht( - "Users must be able to see a build target to view it's build log."); + 'Users must be able to see a build target to view its build log.'); + } + + +/* -( PhabricatorDestructibleInterface )----------------------------------- */ + + + public function destroyObjectPermanently( + PhabricatorDestructionEngine $engine) { + $this->destroyFile($engine); + $this->destroyChunks(); + $this->delete(); + } + + public function destroyFile(PhabricatorDestructionEngine $engine = null) { + if (!$engine) { + $engine = new PhabricatorDestructionEngine(); + } + + $file_phid = $this->getFilePHID(); + if ($file_phid) { + $viewer = $engine->getViewer(); + $file = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs(array($file_phid)) + ->executeOne(); + if ($file) { + $engine->destroyObject($file); + } + } + + $this->setFilePHID(null); + + return $this; + } + + public function destroyChunks() { + $chunk = new HarbormasterBuildLogChunk(); + $conn = $chunk->establishConnection('w'); + + // Just delete the chunks directly so we don't have to pull the data over + // the wire for large logs. + queryfx( + $conn, + 'DELETE FROM %T WHERE logID = %d', + $chunk->getTableName(), + $this->getID()); + + return $this; } diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index 8eef7a6b6e..615781da66 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -62,20 +62,7 @@ final class HarbormasterLogWorker extends HarbormasterWorker { } if ($is_force) { - $file_phid = $log->getFilePHID(); - if ($file_phid) { - $file = id(new PhabricatorFileQuery()) - ->setViewer($viewer) - ->withPHIDs(array($file_phid)) - ->executeOne(); - if ($file) { - id(new PhabricatorDestructionEngine()) - ->destroyObject($file); - } - $log - ->setFilePHID(null) - ->save(); - } + $log->destroyFile(); } if (!$log->getFilePHID()) { From d152bd58360884f41105dc642225492730b536ac Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 05:32:51 -0800 Subject: [PATCH 07/39] Manage log locks on the Log object to prepare for multiple writers Summary: Depends on D19134. Ref T13088. Future changes will support API writers, so push the log lock into the Log object. Allow open/close ("this process is writing to this log") to be separate from live/final ("this log is still generating more data"). Test Plan: Wrote logs with `bin/harbormater write-log` and updated logs with `bin/harbormaster rebuild-log`. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19135 --- .../storage/build/HarbormasterBuildLog.php | 261 +++++++++++------- .../worker/HarbormasterLogWorker.php | 26 +- 2 files changed, 174 insertions(+), 113 deletions(-) diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 8b8fac48f2..0ae4446837 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -16,6 +16,7 @@ final class HarbormasterBuildLog private $buildTarget = self::ATTACHABLE; private $rope; private $isOpen; + private $lock; const CHUNK_BYTE_LIMIT = 1048576; @@ -27,6 +28,12 @@ final class HarbormasterBuildLog if ($this->isOpen) { $this->closeBuildLog(); } + + if ($this->lock) { + if ($this->lock->isLocked()) { + $this->lock->unlock(); + } + } } public static function initializeNewBuildLog( @@ -35,37 +42,7 @@ final class HarbormasterBuildLog return id(new HarbormasterBuildLog()) ->setBuildTargetPHID($build_target->getPHID()) ->setDuration(null) - ->setLive(0); - } - - public function openBuildLog() { - if ($this->isOpen) { - throw new Exception(pht('This build log is already open!')); - } - - $this->isOpen = true; - - return $this - ->setLive(1) - ->save(); - } - - public function closeBuildLog() { - if (!$this->isOpen) { - throw new Exception(pht('This build log is not open!')); - } - - $start = $this->getDateCreated(); - $now = PhabricatorTime::getNow(); - - $this - ->setDuration($now - $start) - ->setLive(0) - ->save(); - - $this->scheduleRebuild(false); - - return $this; + ->setLive(1); } public function scheduleRebuild($force) { @@ -120,72 +97,6 @@ final class HarbormasterBuildLog return pht('Build Log'); } - public function append($content) { - if (!$this->getLive()) { - throw new PhutilInvalidStateException('openBuildLog'); - } - - $content = (string)$content; - - $this->rope->append($content); - $this->flush(); - - return $this; - } - - private function flush() { - - // TODO: Maybe don't flush more than a couple of times per second. If a - // caller writes a single character over and over again, we'll currently - // spend a lot of time flushing that. - - $chunk_table = id(new HarbormasterBuildLogChunk())->getTableName(); - $chunk_limit = self::CHUNK_BYTE_LIMIT; - $encoding_text = HarbormasterBuildLogChunk::CHUNK_ENCODING_TEXT; - - $rope = $this->rope; - - while (true) { - $length = $rope->getByteLength(); - if (!$length) { - break; - } - - $conn_w = $this->establishConnection('w'); - $last = $this->loadLastChunkInfo(); - - $can_append = - ($last) && - ($last['encoding'] == $encoding_text) && - ($last['size'] < $chunk_limit); - if ($can_append) { - $append_id = $last['id']; - $prefix_size = $last['size']; - } else { - $append_id = null; - $prefix_size = 0; - } - - $data_limit = ($chunk_limit - $prefix_size); - $append_data = $rope->getPrefixBytes($data_limit); - $data_size = strlen($append_data); - - if ($append_id) { - queryfx( - $conn_w, - 'UPDATE %T SET chunk = CONCAT(chunk, %B), size = %d WHERE id = %d', - $chunk_table, - $append_data, - $prefix_size + $data_size, - $append_id); - } else { - $this->writeChunk($encoding_text, $data_size, $append_data); - } - - $rope->removeBytesFromHead($data_size); - } - } - public function newChunkIterator() { return id(new HarbormasterBuildLogChunkIterator($this)) ->setPageSize(8); @@ -216,6 +127,16 @@ final class HarbormasterBuildLog return implode('', $full_text); } + + public function getURI() { + $id = $this->getID(); + return "/harbormaster/log/view/{$id}/"; + } + + +/* -( Chunks )------------------------------------------------------------- */ + + public function canCompressLog() { return function_exists('gzdeflate'); } @@ -229,6 +150,13 @@ final class HarbormasterBuildLog } private function processLog($mode) { + if (!$this->getLock()->isLocked()) { + throw new Exception( + pht( + 'You can not process build log chunks unless the log lock is '. + 'held.')); + } + $chunks = $this->newChunkIterator(); // NOTE: Because we're going to insert new chunks, we need to stop the @@ -292,12 +220,145 @@ final class HarbormasterBuildLog ->save(); } - public function getURI() { - $id = $this->getID(); - return "/harbormaster/log/view/{$id}/"; + +/* -( Writing )------------------------------------------------------------ */ + + + public function getLock() { + if (!$this->lock) { + $phid = $this->getPHID(); + $phid_key = PhabricatorHash::digestToLength($phid, 14); + $lock_key = "build.log({$phid_key})"; + $lock = PhabricatorGlobalLock::newLock($lock_key); + $this->lock = $lock; + } + + return $this->lock; } + public function openBuildLog() { + if ($this->isOpen) { + throw new Exception(pht('This build log is already open!')); + } + + $is_new = !$this->getID(); + if ($is_new) { + $this->save(); + } + + $this->getLock()->lock(); + $this->isOpen = true; + + $this->reload(); + + if (!$this->getLive()) { + $this->setLive(1)->save(); + } + + return $this; + } + + public function closeBuildLog($forever = true) { + if (!$this->isOpen) { + throw new Exception( + pht( + 'You must openBuildLog() before you can closeBuildLog().')); + } + + $this->flush(); + + if ($forever) { + $start = $this->getDateCreated(); + $now = PhabricatorTime::getNow(); + + $this + ->setDuration($now - $start) + ->setLive(0) + ->save(); + } + + $this->getLock()->unlock(); + $this->isOpen = false; + + if ($forever) { + $this->scheduleRebuild(false); + } + + return $this; + } + + public function append($content) { + if (!$this->isOpen) { + throw new Exception( + pht( + 'You must openBuildLog() before you can append() content to '. + 'the log.')); + } + + $content = (string)$content; + + $this->rope->append($content); + $this->flush(); + + return $this; + } + + private function flush() { + + // TODO: Maybe don't flush more than a couple of times per second. If a + // caller writes a single character over and over again, we'll currently + // spend a lot of time flushing that. + + $chunk_table = id(new HarbormasterBuildLogChunk())->getTableName(); + $chunk_limit = self::CHUNK_BYTE_LIMIT; + $encoding_text = HarbormasterBuildLogChunk::CHUNK_ENCODING_TEXT; + + $rope = $this->rope; + + while (true) { + $length = $rope->getByteLength(); + if (!$length) { + break; + } + + $conn_w = $this->establishConnection('w'); + $last = $this->loadLastChunkInfo(); + + $can_append = + ($last) && + ($last['encoding'] == $encoding_text) && + ($last['size'] < $chunk_limit); + if ($can_append) { + $append_id = $last['id']; + $prefix_size = $last['size']; + } else { + $append_id = null; + $prefix_size = 0; + } + + $data_limit = ($chunk_limit - $prefix_size); + $append_data = $rope->getPrefixBytes($data_limit); + $data_size = strlen($append_data); + + if ($append_id) { + queryfx( + $conn_w, + 'UPDATE %T SET chunk = CONCAT(chunk, %B), size = %d WHERE id = %d', + $chunk_table, + $append_data, + $prefix_size + $data_size, + $append_id); + } else { + $this->writeChunk($encoding_text, $data_size, $append_data); + } + + $rope->removeBytesFromHead($data_size); + } + } + + + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index 615781da66..b0c84b52f3 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -8,9 +8,18 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $data = $this->getTaskData(); $log_phid = idx($data, 'logPHID'); - $phid_key = PhabricatorHash::digestToLength($log_phid, 14); - $lock_key = "build.log({$phid_key})"; - $lock = PhabricatorGlobalLock::newLock($lock_key); + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withPHIDs(array($log_phid)) + ->executeOne(); + if (!$log) { + throw new PhabricatorWorkerPermanentFailureException( + pht( + 'Invalid build log PHID "%s".', + $log_phid)); + } + + $lock = $log->getLock(); try { $lock->lock(); @@ -20,16 +29,7 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $caught = null; try { - $log = id(new HarbormasterBuildLogQuery()) - ->setViewer($viewer) - ->withPHIDs(array($log_phid)) - ->executeOne(); - if (!$log) { - throw new PhabricatorWorkerPermanentFailureException( - pht( - 'Invalid build log PHID "%s".', - $log_phid)); - } + $log->reload(); if ($log->getLive()) { throw new PhabricatorWorkerPermanentFailureException( From 57e3d607f566ba1cddbca5b149738632359ae80e Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 05:48:07 -0800 Subject: [PATCH 08/39] In Harbormaster, record byte length on the build logs Summary: Depends on D19135. Ref T13088. Denormalize the total log size onto the log itself. This makes reasoning about the log at display time easier, and we don't need to fish around in the database as much to figure out what we're dealing with. Test Plan: Ran `bin/harbormaster rebuild-log`, saw an existing log populate. Ran `bin/harbormaster write-log`, saw new log write with proper length information. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19136 --- .../20180223.log.01.bytelength.sql | 2 ++ .../storage/build/HarbormasterBuildLog.php | 33 +++++++++++-------- .../worker/HarbormasterLogWorker.php | 13 ++++++++ 3 files changed, 35 insertions(+), 13 deletions(-) create mode 100644 resources/sql/autopatches/20180223.log.01.bytelength.sql diff --git a/resources/sql/autopatches/20180223.log.01.bytelength.sql b/resources/sql/autopatches/20180223.log.01.bytelength.sql new file mode 100644 index 0000000000..a4c3505628 --- /dev/null +++ b/resources/sql/autopatches/20180223.log.01.bytelength.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildlog + ADD byteLength BIGINT UNSIGNED NOT NULL; diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 0ae4446837..f43f028d5d 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -12,6 +12,7 @@ final class HarbormasterBuildLog protected $duration; protected $live; protected $filePHID; + protected $byteLength; private $buildTarget = self::ATTACHABLE; private $rope; @@ -42,7 +43,8 @@ final class HarbormasterBuildLog return id(new HarbormasterBuildLog()) ->setBuildTargetPHID($build_target->getPHID()) ->setDuration(null) - ->setLive(1); + ->setLive(1) + ->setByteLength(0); } public function scheduleRebuild($force) { @@ -70,6 +72,7 @@ final class HarbormasterBuildLog 'live' => 'bool', 'filePHID' => 'phid?', + 'byteLength' => 'uint64', ), self::CONFIG_KEY_SCHEMA => array( 'key_buildtarget' => array( @@ -341,24 +344,28 @@ final class HarbormasterBuildLog $append_data = $rope->getPrefixBytes($data_limit); $data_size = strlen($append_data); - if ($append_id) { - queryfx( - $conn_w, - 'UPDATE %T SET chunk = CONCAT(chunk, %B), size = %d WHERE id = %d', - $chunk_table, - $append_data, - $prefix_size + $data_size, - $append_id); - } else { - $this->writeChunk($encoding_text, $data_size, $append_data); - } + $this->openTransaction(); + if ($append_id) { + queryfx( + $conn_w, + 'UPDATE %T SET chunk = CONCAT(chunk, %B), size = %d WHERE id = %d', + $chunk_table, + $append_data, + $prefix_size + $data_size, + $append_id); + } else { + $this->writeChunk($encoding_text, $data_size, $append_data); + } + + $this->byteLength += $data_size; + $this->save(); + $this->saveTransaction(); $rope->removeBytesFromHead($data_size); } } - /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index b0c84b52f3..d90c2391c0 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -57,6 +57,19 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $data = $this->getTaskData(); $is_force = idx($data, 'force'); + if (!$log->getByteLength() || $is_force) { + $iterator = $log->newChunkIterator() + ->setAsString(true); + + $byte_length = 0; + foreach ($iterator as $block) { + $byte_length += strlen($block); + } + $log + ->setByteLength($byte_length) + ->save(); + } + if ($log->canCompressLog()) { $log->compressLog(); } From 46d735d312ca8e54f9b2604783b62c360bf7a1a9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 05:55:52 -0800 Subject: [PATCH 09/39] Add "--all" and an explicit "--force" flag to `bin/harbormaster rebuild-log` Summary: Depends on D19136. Ref T13088. Since it's probably impractical to do all the migrations these changes imply during `bin/storage upgrade`, provide some support for performing them online. Test Plan: Ran `bin/harbormaster rebuild-log` with `--all`, `--id`, and with and without `--force`. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19137 --- ...rbormasterManagementRebuildLogWorkflow.php | 76 +++++++++++++++---- 1 file changed, 62 insertions(+), 14 deletions(-) diff --git a/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php b/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php index 8644bb4515..1dc91bcf0f 100644 --- a/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php +++ b/src/applications/harbormaster/management/HarbormasterManagementRebuildLogWorkflow.php @@ -6,7 +6,10 @@ final class HarbormasterManagementRebuildLogWorkflow protected function didConstruct() { $this ->setName('rebuild-log') - ->setExamples('**rebuild-log** --id __id__ [__options__]') + ->setExamples( + pht( + "**rebuild-log** --id __id__ [__options__]\n". + "**rebuild-log** --all")) ->setSynopsis( pht( 'Rebuild the file and summary for a log. This is primarily '. @@ -18,31 +21,76 @@ final class HarbormasterManagementRebuildLogWorkflow 'param' => 'id', 'help' => pht('Log to rebuild.'), ), + array( + 'name' => 'all', + 'help' => pht('Rebuild all logs.'), + ), + array( + 'name' => 'force', + 'help' => pht( + 'Force logs to rebuild even if they appear to be in good '. + 'shape already.'), + ), )); } public function execute(PhutilArgumentParser $args) { $viewer = $this->getViewer(); - $log_id = $args->getArg('id'); - if (!$log_id) { - throw new PhutilArgumentUsageException( - pht('Choose a build log to rebuild with "--id".')); - } + $is_force = $args->getArg('force'); - $log = id(new HarbormasterBuildLogQuery()) - ->setViewer($viewer) - ->withIDs(array($log_id)) - ->executeOne(); - if (!$log) { + $log_id = $args->getArg('id'); + $is_all = $args->getArg('all'); + + if (!$is_all && !$log_id) { throw new PhutilArgumentUsageException( pht( - 'Unable to load build log "%s".', - $log_id)); + 'Choose a build log to rebuild with "--id", or rebuild all '. + 'logs with "--all".')); + } + + if ($is_all && $log_id) { + throw new PhutilArgumentUsageException( + pht( + 'You can not specify both "--id" and "--all". Choose one or '. + 'the other.')); + } + + if ($log_id) { + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withIDs(array($log_id)) + ->executeOne(); + if (!$log) { + throw new PhutilArgumentUsageException( + pht( + 'Unable to load build log "%s".', + $log_id)); + } + $logs = array($log); + } else { + $logs = new LiskMigrationIterator(new HarbormasterBuildLog()); } PhabricatorWorker::setRunAllTasksInProcess(true); - $log->scheduleRebuild(true); + + foreach ($logs as $log) { + echo tsprintf( + "%s\n", + pht( + 'Rebuilding log "%s"...', + pht('Build Log %d', $log->getID()))); + + try { + $log->scheduleRebuild($is_force); + } catch (Exception $ex) { + if ($is_all) { + phlog($ex); + } else { + throw $ex; + } + } + } echo tsprintf( "%s\n", From d6311044bba66828f8e5d8898662d9dfe40e3088 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 06:07:14 -0800 Subject: [PATCH 10/39] Store the Harbormaster log chunk format on the log record Summary: Depends on D19137. Ref T13088. This allows `rebuild-log` to skip work if the chunks are already compressed. It also prepares for a future GC which is looking for "text" or "gzip" chunks to throw away in favor of archival into Files; such a GC can use this column to find collectable logs and then write "file" to it, meaning "chunks are gone, this data is only available in Files". Test Plan: Ran migration, saw logs populate as "text". Ran `rebuild-log`, saw logs rebuild as "gzip". Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19138 --- .../autopatches/20180223.log.02.chunkformat.sql | 2 ++ .../autopatches/20180223.log.03.chunkdefault.sql | 2 ++ .../storage/build/HarbormasterBuildLog.php | 14 +++++++++++++- .../harbormaster/worker/HarbormasterLogWorker.php | 14 ++++++++------ 4 files changed, 25 insertions(+), 7 deletions(-) create mode 100644 resources/sql/autopatches/20180223.log.02.chunkformat.sql create mode 100644 resources/sql/autopatches/20180223.log.03.chunkdefault.sql diff --git a/resources/sql/autopatches/20180223.log.02.chunkformat.sql b/resources/sql/autopatches/20180223.log.02.chunkformat.sql new file mode 100644 index 0000000000..a15676a952 --- /dev/null +++ b/resources/sql/autopatches/20180223.log.02.chunkformat.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildlog + ADD chunkFormat VARCHAR(32) NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180223.log.03.chunkdefault.sql b/resources/sql/autopatches/20180223.log.03.chunkdefault.sql new file mode 100644 index 0000000000..2a1f2c812b --- /dev/null +++ b/resources/sql/autopatches/20180223.log.03.chunkdefault.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_harbormaster.harbormaster_buildlog + SET chunkFormat = 'text' WHERE chunkFormat = ''; diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index f43f028d5d..6b0b15d8b1 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -13,6 +13,7 @@ final class HarbormasterBuildLog protected $live; protected $filePHID; protected $byteLength; + protected $chunkFormat; private $buildTarget = self::ATTACHABLE; private $rope; @@ -44,7 +45,8 @@ final class HarbormasterBuildLog ->setBuildTargetPHID($build_target->getPHID()) ->setDuration(null) ->setLive(1) - ->setByteLength(0); + ->setByteLength(0) + ->setChunkFormat(HarbormasterBuildLogChunk::CHUNK_ENCODING_TEXT); } public function scheduleRebuild($force) { @@ -73,6 +75,7 @@ final class HarbormasterBuildLog 'live' => 'bool', 'filePHID' => 'phid?', 'byteLength' => 'uint64', + 'chunkFormat' => 'text32', ), self::CONFIG_KEY_SCHEMA => array( 'key_buildtarget' => array( @@ -105,6 +108,11 @@ final class HarbormasterBuildLog ->setPageSize(8); } + public function newDataIterator() { + return $this->newChunkIterator() + ->setAsString(true); + } + private function loadLastChunkInfo() { $chunk_table = new HarbormasterBuildLogChunk(); $conn_w = $chunk_table->establishConnection('w'); @@ -188,6 +196,10 @@ final class HarbormasterBuildLog $this->writeEncodedChunk($rope, $byte_limit, $mode); } + $this + ->setChunkFormat($mode) + ->save(); + $this->saveTransaction(); } diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index d90c2391c0..fc8762de0a 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -58,20 +58,23 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $is_force = idx($data, 'force'); if (!$log->getByteLength() || $is_force) { - $iterator = $log->newChunkIterator() - ->setAsString(true); + $iterator = $log->newDataIterator(); $byte_length = 0; foreach ($iterator as $block) { $byte_length += strlen($block); } + $log ->setByteLength($byte_length) ->save(); } - if ($log->canCompressLog()) { - $log->compressLog(); + $format_text = HarbormasterBuildLogChunk::CHUNK_ENCODING_TEXT; + if (($log->getChunkFormat() === $format_text) || $is_force) { + if ($log->canCompressLog()) { + $log->compressLog(); + } } if ($is_force) { @@ -79,8 +82,7 @@ final class HarbormasterLogWorker extends HarbormasterWorker { } if (!$log->getFilePHID()) { - $iterator = $log->newChunkIterator() - ->setAsString(true); + $iterator = $log->newDataIterator(); $source = id(new PhabricatorIteratorFileUploadSource()) ->setName('harbormaster-log-'.$log->getID().'.log') From 6dc341be87088984beb6ff4a267c4deff57a0bd5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 23 Feb 2018 07:10:18 -0800 Subject: [PATCH 11/39] As Harbormaster logs are processed, build a sparse map of byte offsets to line numbers Summary: Depends on D19138. Ref T13088. When we want to read the last part of a logfile //and show accurate line numbers//, we need to be able to get from byte offsets to line numbers somehow. Our fundamental unit must remain byte offsets, because a test can emit an arbitrarily long line, and we should accommodate it cleanly if a test emits 2GB of the letter "A". To support going from byte offsets to line numbers, compute a map with periodic line markers throughout the offsets of the file. From here, we can figure out the line numbers for arbitrary positions in the file with only a constant amount of work. Test Plan: Added unit tests; ran unit tests. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19139 --- .../autopatches/20180223.log.04.linemap.sql | 2 + .../20180223.log.05.linemapdefault.sql | 2 + src/__phutil_library_map__.php | 2 + .../HarbormasterBuildLogTestCase.php | 117 ++++++++++++++++ .../storage/build/HarbormasterBuildLog.php | 131 +++++++++++++++++- .../worker/HarbormasterLogWorker.php | 13 +- 6 files changed, 260 insertions(+), 7 deletions(-) create mode 100644 resources/sql/autopatches/20180223.log.04.linemap.sql create mode 100644 resources/sql/autopatches/20180223.log.05.linemapdefault.sql create mode 100644 src/applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php diff --git a/resources/sql/autopatches/20180223.log.04.linemap.sql b/resources/sql/autopatches/20180223.log.04.linemap.sql new file mode 100644 index 0000000000..75ed27cf7c --- /dev/null +++ b/resources/sql/autopatches/20180223.log.04.linemap.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_buildlog + ADD lineMap LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20180223.log.05.linemapdefault.sql b/resources/sql/autopatches/20180223.log.05.linemapdefault.sql new file mode 100644 index 0000000000..59b4dc9a62 --- /dev/null +++ b/resources/sql/autopatches/20180223.log.05.linemapdefault.sql @@ -0,0 +1,2 @@ +UPDATE {$NAMESPACE}_harbormaster.harbormaster_buildlog + SET lineMap = '[]' WHERE lineMap = ''; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index be9cb258cc..01407ed67b 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1230,6 +1230,7 @@ phutil_register_library_map(array( 'HarbormasterBuildLogDownloadController' => 'applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php', 'HarbormasterBuildLogPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php', 'HarbormasterBuildLogQuery' => 'applications/harbormaster/query/HarbormasterBuildLogQuery.php', + 'HarbormasterBuildLogTestCase' => 'applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php', 'HarbormasterBuildLogView' => 'applications/harbormaster/view/HarbormasterBuildLogView.php', 'HarbormasterBuildLogViewController' => 'applications/harbormaster/controller/HarbormasterBuildLogViewController.php', 'HarbormasterBuildMessage' => 'applications/harbormaster/storage/HarbormasterBuildMessage.php', @@ -6518,6 +6519,7 @@ phutil_register_library_map(array( 'HarbormasterBuildLogDownloadController' => 'HarbormasterController', 'HarbormasterBuildLogPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildLogTestCase' => 'PhabricatorTestCase', 'HarbormasterBuildLogView' => 'AphrontView', 'HarbormasterBuildLogViewController' => 'HarbormasterController', 'HarbormasterBuildMessage' => array( diff --git a/src/applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php b/src/applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php new file mode 100644 index 0000000000..6cbdd6b0db --- /dev/null +++ b/src/applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php @@ -0,0 +1,117 @@ + array( + 64, + array( + str_repeat('AAAAAAAA', 32), + ), + array( + array(64, 0), + array(128, 0), + array(192, 0), + array(255, 0), + ), + ), + 'no_newlines_updated.log' => array( + 64, + array_fill(0, 32, 'AAAAAAAA'), + array( + array(64, 0), + array(128, 0), + array(192, 0), + ), + ), + 'one_newline.log' => array( + 64, + array( + str_repeat('AAAAAAAA', 16), + "\n", + str_repeat('AAAAAAAA', 16), + ), + array( + array(64, 0), + array(127, 0), + array(191, 1), + array(255, 1), + ), + ), + 'several_newlines.log' => array( + 64, + array_fill(0, 12, "AAAAAAAAAAAAAAAAAA\n"), + array( + array(56, 2), + array(113, 5), + array(170, 8), + array(227, 11), + ), + ), + 'mixed_newlines.log' => array( + 64, + array( + str_repeat('A', 63)."\r", + str_repeat('A', 63)."\r\n", + str_repeat('A', 63)."\n", + str_repeat('A', 63), + ), + array( + array(63, 0), + array(127, 1), + array(191, 2), + array(255, 3), + ), + ), + 'more_mixed_newlines.log' => array( + 64, + array( + str_repeat('A', 63)."\r", + str_repeat('A', 62)."\r\n", + str_repeat('A', 63)."\n", + str_repeat('A', 63), + ), + array( + array(63, 0), + array(128, 2), + array(191, 2), + array(254, 3), + ), + ), + 'emoji.log' => array( + 64, + array( + str_repeat($snowman, 64), + ), + array( + array(63, 0), + array(126, 0), + array(189, 0), + ), + ), + ); + + foreach ($inputs as $label => $input) { + list($distance, $parts, $expect) = $input; + + $log = id(new HarbormasterBuildLog()) + ->setByteLength(0); + + foreach ($parts as $part) { + $log->updateLineMap($part, $distance); + } + + list($actual) = $log->getLineMap(); + + $this->assertEqual( + $expect, + $actual, + pht('Line Map for "%s"', $label)); + } + } + +} diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 6b0b15d8b1..9b983f0733 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -14,6 +14,7 @@ final class HarbormasterBuildLog protected $filePHID; protected $byteLength; protected $chunkFormat; + protected $lineMap = array(); private $buildTarget = self::ATTACHABLE; private $rope; @@ -64,6 +65,9 @@ final class HarbormasterBuildLog protected function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, + self::CONFIG_SERIALIZATION => array( + 'lineMap' => self::SERIALIZATION_JSON, + ), self::CONFIG_COLUMN_SCHEMA => array( // T6203/NULLABILITY // It seems like these should be non-nullable? All logs should have a @@ -369,7 +373,8 @@ final class HarbormasterBuildLog $this->writeChunk($encoding_text, $data_size, $append_data); } - $this->byteLength += $data_size; + $this->updateLineMap($append_data); + $this->save(); $this->saveTransaction(); @@ -377,6 +382,130 @@ final class HarbormasterBuildLog } } + public function updateLineMap($append_data, $marker_distance = null) { + $this->byteLength += strlen($append_data); + + if (!$marker_distance) { + $marker_distance = (self::CHUNK_BYTE_LIMIT / 2); + } + + if (!$this->lineMap) { + $this->lineMap = array( + array(), + 0, + 0, + null, + ); + } + + list($map, $map_bytes, $line_count, $prefix) = $this->lineMap; + + $buffer = $append_data; + + if ($prefix) { + $prefix = base64_decode($prefix); + $buffer = $prefix.$buffer; + } + + if ($map) { + list($last_marker, $last_count) = last($map); + } else { + $last_marker = 0; + $last_count = 0; + } + + $max_utf8_width = 8; + $next_marker = $last_marker + $marker_distance; + + $pos = 0; + $len = strlen($buffer); + while (true) { + // If we only have a few bytes left in the buffer, leave it as a prefix + // for next time. + if (($len - $pos) <= ($max_utf8_width * 2)) { + $prefix = substr($buffer, $pos); + break; + } + + // The next slice we're going to look at is the smaller of: + // + // - the number of bytes we need to make it to the next marker; or + // - all the bytes we have left, minus one. + + $slice_length = min( + ($marker_distance - $map_bytes), + ($len - $pos) - 1); + + // We don't slice all the way to the end for two reasons. + + // First, we want to avoid slicing immediately after a "\r" if we don't + // know what the next character is, because we want to make sure to + // count "\r\n" as a single newline, rather than counting the "\r" as + // a newline and then later counting the "\n" as another newline. + + // Second, we don't want to slice in the middle of a UTF8 character if + // we can help it. We may not be able to avoid this, since the whole + // buffer may just be binary data, but in most cases we can backtrack + // a little bit and try to make it out of emoji or other legitimate + // multibyte UTF8 characters which appear in the log. + + $min_width = max(1, $slice_length - $max_utf8_width); + while ($slice_length >= $min_width) { + $here = $buffer[$pos + ($slice_length - 1)]; + $next = $buffer[$pos + ($slice_length - 1) + 1]; + + // If this is "\r" and the next character is "\n", extend the slice + // to include the "\n". Otherwise, we're fine to slice here since we + // know we're not in the middle of a UTF8 character. + if ($here === "\r") { + if ($next === "\n") { + $slice_length++; + } + break; + } + + // If the next character is 0x7F or lower, or between 0xC2 and 0xF4, + // we're not slicing in the middle of a UTF8 character. + $ord = ord($next); + if ($ord <= 0x7F || ($ord >= 0xC2 && $ord <= 0xF4)) { + break; + } + + $slice_length--; + } + + $slice = substr($buffer, $pos, $slice_length); + $pos += $slice_length; + + $map_bytes += $slice_length; + $line_count += count(preg_split("/\r\n|\r|\n/", $slice)) - 1; + + if ($map_bytes >= ($marker_distance - $max_utf8_width)) { + $map[] = array( + $last_marker + $map_bytes, + $last_count + $line_count, + ); + + $last_count = $last_count + $line_count; + $line_count = 0; + + $last_marker = $last_marker + $map_bytes; + $map_bytes = 0; + + $next_marker = $last_marker + $marker_distance; + } + } + + $this->lineMap = array( + $map, + $map_bytes, + $line_count, + base64_encode($prefix), + ); + + return $this; + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/harbormaster/worker/HarbormasterLogWorker.php b/src/applications/harbormaster/worker/HarbormasterLogWorker.php index fc8762de0a..d9d7511c27 100644 --- a/src/applications/harbormaster/worker/HarbormasterLogWorker.php +++ b/src/applications/harbormaster/worker/HarbormasterLogWorker.php @@ -57,17 +57,18 @@ final class HarbormasterLogWorker extends HarbormasterWorker { $data = $this->getTaskData(); $is_force = idx($data, 'force'); - if (!$log->getByteLength() || $is_force) { + if (!$log->getByteLength() || !$log->getLineMap() || $is_force) { $iterator = $log->newDataIterator(); - $byte_length = 0; + $log + ->setByteLength(0) + ->setLineMap(array()); + foreach ($iterator as $block) { - $byte_length += strlen($block); + $log->updateLineMap($block); } - $log - ->setByteLength($byte_length) - ->save(); + $log->save(); } $format_text = HarbormasterBuildLogChunk::CHUNK_ENCODING_TEXT; From 11d1dc484b867d287a258813510c395407e9151c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 22 Feb 2018 19:35:13 -0800 Subject: [PATCH 12/39] Sort of make Harbormaster build logs page properly Summary: Depends on D19139. Ref T13088. This doesn't actually work, but is close enough that a skilled attacker might be able to briefly deceive a small child. Test Plan: - Viewed some very small logs under very controlled conditions, saw content. - Larger logs vaguely do something resembling working correctly. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19141 --- resources/celerity/map.php | 9 +- src/__phutil_library_map__.php | 2 + .../PhabricatorHarbormasterApplication.php | 5 +- .../HarbormasterBuildLogRenderController.php | 562 ++++++++++++++++++ .../HarbormasterBuildLogViewController.php | 6 +- .../storage/build/HarbormasterBuildLog.php | 33 + .../view/HarbormasterBuildLogView.php | 30 +- .../application/harbormaster/harbormaster.css | 38 ++ .../harbormaster/behavior-harbormaster-log.js | 43 ++ 9 files changed, 721 insertions(+), 7 deletions(-) create mode 100644 src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php create mode 100644 webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 01dd75c901..1361bbd12c 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -78,7 +78,7 @@ return array( 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', 'rsrc/css/application/flag/flag.css' => 'bba8f811', - 'rsrc/css/application/harbormaster/harbormaster.css' => 'f491c9f4', + 'rsrc/css/application/harbormaster/harbormaster.css' => 'fecac64f', 'rsrc/css/application/herald/herald-test.css' => 'a52e323e', 'rsrc/css/application/herald/herald.css' => 'cd8d0134', 'rsrc/css/application/maniphest/report.css' => '9b9580b7', @@ -416,6 +416,7 @@ return array( 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', + 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '0844f3c1', 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', @@ -578,7 +579,7 @@ return array( 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', 'global-drag-and-drop-css' => 'b556a948', - 'harbormaster-css' => 'f491c9f4', + 'harbormaster-css' => 'fecac64f', 'herald-css' => 'cd8d0134', 'herald-rule-editor' => 'dca75c0e', 'herald-test-css' => 'a52e323e', @@ -635,6 +636,7 @@ return array( 'javelin-behavior-event-all-day' => 'b41537c9', 'javelin-behavior-fancy-datepicker' => 'ecf4e799', 'javelin-behavior-global-drag-and-drop' => '960f6a39', + 'javelin-behavior-harbormaster-log' => '0844f3c1', 'javelin-behavior-herald-rule-editor' => '7ebaeed3', 'javelin-behavior-high-security-warning' => 'a464fe03', 'javelin-behavior-history-install' => '7ee2b591', @@ -960,6 +962,9 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), + '0844f3c1' => array( + 'javelin-behavior', + ), '08f4ccc3' => array( 'phui-oi-list-view-css', ), diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 01407ed67b..3a7ba1dbbc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1230,6 +1230,7 @@ phutil_register_library_map(array( 'HarbormasterBuildLogDownloadController' => 'applications/harbormaster/controller/HarbormasterBuildLogDownloadController.php', 'HarbormasterBuildLogPHIDType' => 'applications/harbormaster/phid/HarbormasterBuildLogPHIDType.php', 'HarbormasterBuildLogQuery' => 'applications/harbormaster/query/HarbormasterBuildLogQuery.php', + 'HarbormasterBuildLogRenderController' => 'applications/harbormaster/controller/HarbormasterBuildLogRenderController.php', 'HarbormasterBuildLogTestCase' => 'applications/harbormaster/__tests__/HarbormasterBuildLogTestCase.php', 'HarbormasterBuildLogView' => 'applications/harbormaster/view/HarbormasterBuildLogView.php', 'HarbormasterBuildLogViewController' => 'applications/harbormaster/controller/HarbormasterBuildLogViewController.php', @@ -6519,6 +6520,7 @@ phutil_register_library_map(array( 'HarbormasterBuildLogDownloadController' => 'HarbormasterController', 'HarbormasterBuildLogPHIDType' => 'PhabricatorPHIDType', 'HarbormasterBuildLogQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'HarbormasterBuildLogRenderController' => 'HarbormasterController', 'HarbormasterBuildLogTestCase' => 'PhabricatorTestCase', 'HarbormasterBuildLogView' => 'AphrontView', 'HarbormasterBuildLogViewController' => 'HarbormasterController', diff --git a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php index b4f62d8e22..103e6bce69 100644 --- a/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php +++ b/src/applications/harbormaster/application/PhabricatorHarbormasterApplication.php @@ -97,7 +97,10 @@ final class PhabricatorHarbormasterApplication extends PhabricatorApplication { 'buildkite/' => 'HarbormasterBuildkiteHookController', ), 'log/' => array( - 'view/(?P\d+)/' => 'HarbormasterBuildLogViewController', + 'view/(?P\d+)/(?:\$(?P\d+(?:-\d+)?))?' + => 'HarbormasterBuildLogViewController', + 'render/(?P\d+)/(?:\$(?P\d+(?:-\d+)?))?' + => 'HarbormasterBuildLogRenderController', 'download/(?P\d+)/' => 'HarbormasterBuildLogDownloadController', ), ), diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php new file mode 100644 index 0000000000..a2322dc51d --- /dev/null +++ b/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php @@ -0,0 +1,562 @@ +getViewer(); + + $id = $request->getURIData('id'); + + $log = id(new HarbormasterBuildLogQuery()) + ->setViewer($viewer) + ->withIDs(array($id)) + ->executeOne(); + if (!$log) { + return new Aphront404Response(); + } + + $log_size = $this->getTotalByteLength($log); + + $head_lines = $request->getInt('head'); + if ($head_lines === null) { + $head_lines = 8; + } + $head_lines = min($head_lines, 100); + $head_lines = max($head_lines, 0); + + $tail_lines = $request->getInt('tail'); + if ($tail_lines === null) { + $tail_lines = 16; + } + $tail_lines = min($tail_lines, 100); + $tail_lines = max($tail_lines, 0); + + $head_offset = $request->getInt('headOffset'); + if ($head_offset === null) { + $head_offset = 0; + } + + $tail_offset = $request->getInt('tailOffset'); + if ($tail_offset === null) { + $tail_offset = $log_size; + } + + // Figure out which ranges we're actually going to read. We'll read either + // one range (either just at the head, or just at the tail) or two ranges + // (one at the head and one at the tail). + + // This gets a little bit tricky because: the ranges may overlap; we just + // want to do one big read if there is only a little bit of text left + // between the ranges; we may not know where the tail range ends; and we + // can only read forward from line map markers, not from any arbitrary + // position in the file. + + $bytes_per_line = 140; + $body_lines = 8; + + $views = array(); + if ($head_lines > 0) { + $views[] = array( + 'offset' => $head_offset, + 'lines' => $head_lines, + 'direction' => 1, + ); + } + + if ($tail_lines > 0) { + $views[] = array( + 'offset' => $tail_offset, + 'lines' => $tail_lines, + 'direction' => -1, + ); + } + + $reads = $views; + foreach ($reads as $key => $read) { + $offset = $read['offset']; + + $lines = $read['lines']; + + $read_length = 0; + $read_length += ($lines * $bytes_per_line); + $read_length += ($body_lines * $bytes_per_line); + + $direction = $read['direction']; + if ($direction < 0) { + $offset -= $read_length; + if ($offset < 0) { + $offset = 0; + $read_length = $log_size; + } + } + + $position = $log->getReadPosition($offset); + list($position_offset, $position_line) = $position; + $read_length += ($offset - $position_offset); + + $reads[$key]['fetchOffset'] = $position_offset; + $reads[$key]['fetchLength'] = $read_length; + $reads[$key]['fetchLine'] = $position_line; + } + + $reads = $this->mergeOverlappingReads($reads); + + foreach ($reads as $key => $read) { + $data = $log->loadData($read['fetchOffset'], $read['fetchLength']); + + $offset = $read['fetchOffset']; + $line = $read['fetchLine']; + $lines = $this->getLines($data); + $line_data = array(); + foreach ($lines as $line_text) { + $length = strlen($line_text); + $line_data[] = array( + 'offset' => $offset, + 'length' => $length, + 'line' => $line, + 'data' => $line_text, + ); + $line += 1; + $offset += $length; + } + + $reads[$key]['data'] = $data; + $reads[$key]['lines'] = $line_data; + } + + foreach ($views as $view_key => $view) { + $anchor_byte = $view['offset']; + + $data_key = null; + foreach ($reads as $read_key => $read) { + $s = $read['fetchOffset']; + $e = $s + $read['fetchLength']; + + if (($s <= $anchor_byte) && ($e >= $anchor_byte)) { + $data_key = $read_key; + break; + } + } + + if ($data_key === null) { + throw new Exception( + pht('Unable to find fetch!')); + } + + $anchor_key = null; + foreach ($reads[$data_key]['lines'] as $line_key => $line) { + $s = $line['offset']; + $e = $s + $line['length']; + if (($s <= $anchor_byte) && ($e >= $anchor_byte)) { + $anchor_key = $line_key; + break; + } + } + + if ($anchor_key === null) { + throw new Exception( + pht( + 'Unable to find lines.')); + } + + if ($direction > 0) { + $slice_offset = $line_key; + } else { + $slice_offset = max(0, $line_key - ($view['lines'] - 1)); + } + $slice_length = $view['lines']; + + $views[$view_key] += array( + 'sliceKey' => $data_key, + 'sliceOffset' => $slice_offset, + 'sliceLength' => $slice_length, + ); + } + + foreach ($views as $view_key => $view) { + $slice_key = $view['sliceKey']; + $lines = array_slice( + $reads[$slice_key]['lines'], + $view['sliceOffset'], + $view['sliceLength']); + + $data_offset = null; + $data_length = null; + foreach ($lines as $line) { + if ($data_offset === null) { + $data_offset = $line['offset']; + } + $data_length += $line['length']; + } + + // If the view cursor starts in the middle of a line, we're going to + // strip part of the line. + $direction = $view['direction']; + if ($direction > 0) { + $view_offset = $view['offset']; + $view_length = $data_length; + if ($data_offset < $view_offset) { + $trim = ($view_offset - $data_offset); + $view_length -= $trim; + } + } else { + $view_offset = $data_offset; + $view_length = $data_length; + if ($data_offset + $data_length > $view['offset']) { + $view_length -= (($data_offset + $data_length) - $view['offset']); + } + } + + $views[$view_key] += array( + 'viewOffset' => $view_offset, + 'viewLength' => $view_length, + ); + } + + $views = $this->mergeOverlappingViews($views); + + foreach ($views as $view_key => $view) { + $slice_key = $view['sliceKey']; + $lines = array_slice( + $reads[$slice_key]['lines'], + $view['sliceOffset'], + $view['sliceLength']); + + $view_offset = $view['viewOffset']; + foreach ($lines as $line_key => $line) { + $line_offset = $line['offset']; + + if ($line_offset >= $view_offset) { + break; + } + + $trim = ($view_offset - $line_offset); + $line_data = substr($line['data'], $trim); + if (!strlen($line_data)) { + unset($lines[$line_key]); + continue; + } + + $lines[$line_key]['data'] = $line_data; + $lines[$line_key]['length'] = strlen($line_data); + $lines[$line_key]['offset'] += $trim; + break; + } + + $view_end = $view['viewOffset'] + $view['viewLength']; + foreach ($lines as $line_key => $line) { + $line_end = $line['offset'] + $line['length']; + if ($line_end <= $view_end) { + break; + } + + $trim = ($line_end - $view_end); + $line_data = substr($line['data'], -$trim); + if (!strlen($line_data)) { + unset($lines[$line_key]); + continue; + } + + $lines[$line_key]['data'] = $line_data; + $lines[$line_key]['length'] = strlen($line_data); + } + + $views[$view_key]['viewData'] = $lines; + } + + $spacer = null; + $render = array(); + foreach ($views as $view) { + if ($spacer) { + $spacer['tail'] = $view['viewOffset']; + $render[] = $spacer; + } + + $render[] = $view; + + $spacer = array( + 'spacer' => true, + 'head' => ($view['viewOffset'] + $view['viewLength']), + ); + } + + $uri = $log->getURI(); + $highlight_range = $request->getURIData('lines'); + + $rows = array(); + foreach ($render as $range) { + if (isset($range['spacer'])) { + $rows[] = phutil_tag( + 'tr', + array(), + array( + phutil_tag( + 'th', + array(), + null), + phutil_tag( + 'td', + array(), + array( + javelin_tag( + 'a', + array( + 'sigil' => 'harbormaster-log-expand', + 'meta' => array( + 'headOffset' => $range['head'], + 'tailOffset' => $range['tail'], + 'head' => 4, + ), + ), + 'Show Up ^^^^'), + '... '.($range['tail'] - $range['head']).' bytes ...', + javelin_tag( + 'a', + array( + 'sigil' => 'harbormaster-log-expand', + 'meta' => array( + 'headOffset' => $range['head'], + 'tailOffset' => $range['tail'], + 'tail' => 4, + ), + ), + 'Show Down VVVV'), + )), + )); + continue; + } + + $lines = $range['viewData']; + foreach ($lines as $line) { + $display_line = ($line['line'] + 1); + $display_text = ($line['data']); + + $display_line = phutil_tag( + 'a', + array( + 'href' => $uri.'$'.$display_line, + ), + $display_line); + + $line_cell = phutil_tag('th', array(), $display_line); + $text_cell = phutil_tag('td', array(), $display_text); + + $rows[] = phutil_tag( + 'tr', + array(), + array( + $line_cell, + $text_cell, + )); + } + } + + $table = phutil_tag( + 'table', + array( + 'class' => 'harbormaster-log-table PhabricatorMonospaced', + ), + $rows); + + // When this is a normal AJAX request, return the rendered log fragment + // in an AJAX payload. + if ($request->isAjax()) { + return id(new AphrontAjaxResponse()) + ->setContent( + array( + 'markup' => hsprintf('%s', $table), + )); + } + + // If the page is being accessed as a standalone page, present a + // readable version of the fragment for debugging. + + require_celerity_resource('harbormaster-css'); + + $header = pht('Standalone Log Fragment'); + + $render_view = id(new PHUIObjectBoxView()) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setHeaderText($header) + ->appendChild($table); + + $page_view = id(new PHUITwoColumnView()) + ->setFooter($render_view); + + $crumbs = $this->buildApplicationCrumbs() + ->addTextCrumb(pht('Build Log %d', $log->getID()), $log->getURI()) + ->addTextCrumb(pht('Fragment')) + ->setBorder(true); + + return $this->newPage() + ->setTitle( + array( + pht('Build Log %d', $log->getID()), + pht('Standalone Fragment'), + )) + ->setCrumbs($crumbs) + ->appendChild($page_view); + } + + private function getTotalByteLength(HarbormasterBuildLog $log) { + $total_bytes = $log->getByteLength(); + if ($total_bytes) { + return (int)$total_bytes; + } + + // TODO: Remove this after enough time has passed for installs to run + // log rebuilds or decide they don't care about older logs. + + // Older logs don't have this data denormalized onto the log record unless + // an administrator has run `bin/harbormaster rebuild-log --all` or + // similar. Try to figure it out by summing up the size of each chunk. + + // Note that the log may also be legitimately empty and have actual size + // zero. + $chunk = new HarbormasterBuildLogChunk(); + $conn = $chunk->establishConnection('r'); + + $row = queryfx_one( + $conn, + 'SELECT SUM(size) total FROM %T WHERE logID = %d', + $chunk->getTableName(), + $log->getID()); + + return (int)$row['total']; + } + + private function getLines($data) { + $parts = preg_split("/(\r\n|\r|\n)/", $data, 0, PREG_SPLIT_DELIM_CAPTURE); + + if (last($parts) === '') { + array_pop($parts); + } + + $lines = array(); + for ($ii = 0; $ii < count($parts); $ii += 2) { + $line = $parts[$ii]; + if (isset($parts[$ii + 1])) { + $line .= $parts[$ii + 1]; + } + $lines[] = $line; + } + + return $lines; + } + + + private function mergeOverlappingReads(array $reads) { + // Find planned reads which will overlap and merge them into a single + // larger read. + + $uk = array_keys($reads); + $vk = array_keys($reads); + + foreach ($uk as $ukey) { + foreach ($vk as $vkey) { + // Don't merge a range into itself, even though they do technically + // overlap. + if ($ukey === $vkey) { + continue; + } + + $uread = idx($reads, $ukey); + if ($uread === null) { + continue; + } + + $vread = idx($reads, $vkey); + if ($vread === null) { + continue; + } + + $us = $uread['fetchOffset']; + $ue = $us + $uread['fetchLength']; + + $vs = $vread['fetchOffset']; + $ve = $vs + $vread['fetchLength']; + + if (($vs > $ue) || ($ve < $us)) { + continue; + } + + $min = min($us, $vs); + $max = max($ue, $ve); + + $reads[$ukey]['fetchOffset'] = $min; + $reads[$ukey]['fetchLength'] = ($max - $min); + $reads[$ukey]['fetchLine'] = min( + $uread['fetchLine'], + $vread['fetchLine']); + + unset($reads[$vkey]); + } + } + + return $reads; + } + + private function mergeOverlappingViews(array $views) { + $uk = array_keys($views); + $vk = array_keys($views); + + $body_lines = 8; + $body_bytes = ($body_lines * 140); + + foreach ($uk as $ukey) { + foreach ($vk as $vkey) { + if ($ukey === $vkey) { + continue; + } + + $uview = idx($views, $ukey); + if ($uview === null) { + continue; + } + + $vview = idx($views, $vkey); + if ($vview === null) { + continue; + } + + // If these views don't use the same line data, don't try to + // merge them. + if ($uview['sliceKey'] != $vview['sliceKey']) { + continue; + } + + // If these views are overlapping or separated by only a few bytes, + // merge them into a single view. + $us = $uview['viewOffset']; + $ue = $us + $uview['viewLength']; + + $vs = $vview['viewOffset']; + $ve = $vs + $vview['viewLength']; + + $uss = $uview['sliceOffset']; + $use = $uss + $uview['sliceLength']; + + $vss = $vview['sliceOffset']; + $vse = $vss + $vview['sliceLength']; + + if ($ue <= $vs) { + if (($ue + $body_bytes) >= $vs) { + if (($use + $body_lines) >= $vss) { + $views[$ukey] = array( + 'sliceLength' => ($vse - $uss), + 'viewLength' => ($ve - $us), + ) + $views[$ukey]; + + unset($views[$vkey]); + continue; + } + } + } + } + } + + return $views; + } + +} diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php index 084c48d1f1..41f2aef8e5 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildLogViewController.php @@ -4,8 +4,7 @@ final class HarbormasterBuildLogViewController extends HarbormasterController { public function handleRequest(AphrontRequest $request) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); $id = $request->getURIData('id'); @@ -21,7 +20,8 @@ final class HarbormasterBuildLogViewController $log_view = id(new HarbormasterBuildLogView()) ->setViewer($viewer) - ->setBuildLog($log); + ->setBuildLog($log) + ->setHighlightedLineRange($request->getURIData('lines')); $crumbs = $this->buildApplicationCrumbs() ->addTextCrumb(pht('Build Logs')) diff --git a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php index 9b983f0733..64934bbea8 100644 --- a/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php +++ b/src/applications/harbormaster/storage/build/HarbormasterBuildLog.php @@ -129,6 +129,30 @@ final class HarbormasterBuildLog $this->getID()); } + public function loadData($offset, $length) { + return substr($this->getLogText(), $offset, $length); + } + + public function getReadPosition($read_offset) { + $position = array(0, 0); + + $map = $this->getLineMap(); + if (!$map) { + throw new Exception(pht('No line map.')); + } + + list($map) = $map; + foreach ($map as $marker) { + list($offset, $count) = $marker; + if ($offset > $read_offset) { + break; + } + $position = $marker; + } + + return $position; + } + public function getLogText() { // TODO: Remove this method since it won't scale for big logs. @@ -148,6 +172,15 @@ final class HarbormasterBuildLog return "/harbormaster/log/view/{$id}/"; } + public function getRenderURI($lines) { + if (strlen($lines)) { + $lines = '$'.$lines; + } + + $id = $this->getID(); + return "/harbormaster/log/render/{$id}/{$lines}"; + } + /* -( Chunks )------------------------------------------------------------- */ diff --git a/src/applications/harbormaster/view/HarbormasterBuildLogView.php b/src/applications/harbormaster/view/HarbormasterBuildLogView.php index 7543524285..81705f3f49 100644 --- a/src/applications/harbormaster/view/HarbormasterBuildLogView.php +++ b/src/applications/harbormaster/view/HarbormasterBuildLogView.php @@ -3,6 +3,7 @@ final class HarbormasterBuildLogView extends AphrontView { private $log; + private $highlightedLineRange; public function setBuildLog(HarbormasterBuildLog $log) { $this->log = $log; @@ -13,6 +14,15 @@ final class HarbormasterBuildLogView extends AphrontView { return $this->log; } + public function setHighlightedLineRange($range) { + $this->highlightedLineRange = $range; + return $this; + } + + public function getHighlightedLineRange() { + return $this->highlightedLineRange; + } + public function render() { $viewer = $this->getViewer(); $log = $this->getBuildLog(); @@ -34,10 +44,28 @@ final class HarbormasterBuildLogView extends AphrontView { $header->addActionLink($download_button); + $content_id = celerity_generate_unique_node_id(); + $content_div = javelin_tag( + 'div', + array( + 'id' => $content_id, + 'class' => 'harbormaster-log-view-loading', + ), + pht('Loading...')); + + require_celerity_resource('harbormaster-css'); + + Javelin::initBehavior( + 'harbormaster-log', + array( + 'contentNodeID' => $content_id, + 'renderURI' => $log->getRenderURI($this->getHighlightedLineRange()), + )); + $box_view = id(new PHUIObjectBoxView()) ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) ->setHeader($header) - ->appendChild('...'); + ->appendChild($content_div); return $box_view; } diff --git a/webroot/rsrc/css/application/harbormaster/harbormaster.css b/webroot/rsrc/css/application/harbormaster/harbormaster.css index 4bfd2d6cbe..f55139ade0 100644 --- a/webroot/rsrc/css/application/harbormaster/harbormaster.css +++ b/webroot/rsrc/css/application/harbormaster/harbormaster.css @@ -30,3 +30,41 @@ text-overflow: ellipsis; color: {$lightgreytext}; } + +.harbormaster-log-view-loading { + padding: 8px; + text-align: center; + color: {$lightgreytext}; +} + +.harbormaster-log-table th { + background-color: {$paste.highlight}; + border-right: 1px solid {$paste.border}; + + -moz-user-select: -moz-none; + -khtml-user-select: none; + -webkit-user-select: none; + -ms-user-select: none; + user-select: none; +} + +.harbormaster-log-table th a { + display: block; + color: {$darkbluetext}; + text-align: right; + padding: 2px 6px 1px 12px; +} + +.harbormaster-log-table th a:hover { + background: {$paste.border}; +} + +.harbormaster-log-table td { + white-space: pre-wrap; + padding: 2px 8px 1px; + width: 100%; +} + +.harbormaster-log-table tr.harbormaster-log-highlighted td { + background: {$paste.highlight}; +} diff --git a/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js new file mode 100644 index 0000000000..d011233afd --- /dev/null +++ b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js @@ -0,0 +1,43 @@ +/** + * @provides javelin-behavior-harbormaster-log + * @requires javelin-behavior + */ + +JX.behavior('harbormaster-log', function(config) { + var contentNode = JX.$(config.contentNodeID); + + JX.DOM.listen(contentNode, 'click', 'harbormaster-log-expand', function(e) { + if (!e.isNormalClick()) { + return; + } + + e.kill(); + + var row = e.getNode('tag:tr'); + var data = e.getNodeData('harbormaster-log-expand'); + + var uri = new JX.URI(config.renderURI) + .addQueryParams(data); + + var request = new JX.Request(uri, function(r) { + var result = JX.$H(r.markup).getNode(); + var rows = JX.DOM.scry(result, 'tr'); + + JX.DOM.replace(row, rows); + }); + + request.send(); + }); + + function onresponse(r) { + JX.DOM.alterClass(contentNode, 'harbormaster-log-view-loading', false); + + JX.DOM.setContent(contentNode, JX.$H(r.markup)); + } + + var uri = new JX.URI(config.renderURI); + + new JX.Request(uri, onresponse) + .send(); + +}); From f450c6c55bfdc3680a1895842ac17765a60e3d15 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Feb 2018 17:46:49 -0800 Subject: [PATCH 13/39] Fix some of the most egregious errors in Harbormaster log paging Summary: Depends on D19141. Ref T13088. Some of the fundamental log behaviors like "loading the correct rows" are now a bit better behaved. The UI is a little less garbage, too. Test Plan: Viewed some logs and loaded more context by clicking the buttons. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13088 Differential Revision: https://secure.phabricator.com/D19142 --- resources/celerity/map.php | 14 +- .../HarbormasterBuildLogRenderController.php | 196 ++++++++++++++---- .../application/harbormaster/harbormaster.css | 64 +++++- .../harbormaster/behavior-harbormaster-log.js | 4 +- 4 files changed, 220 insertions(+), 58 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 1361bbd12c..7907d3e18f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -78,7 +78,7 @@ return array( 'rsrc/css/application/feed/feed.css' => 'ecd4ec57', 'rsrc/css/application/files/global-drag-and-drop.css' => 'b556a948', 'rsrc/css/application/flag/flag.css' => 'bba8f811', - 'rsrc/css/application/harbormaster/harbormaster.css' => 'fecac64f', + 'rsrc/css/application/harbormaster/harbormaster.css' => 'c7e29d9e', 'rsrc/css/application/herald/herald-test.css' => 'a52e323e', 'rsrc/css/application/herald/herald.css' => 'cd8d0134', 'rsrc/css/application/maniphest/report.css' => '9b9580b7', @@ -416,7 +416,7 @@ return array( 'rsrc/js/application/drydock/drydock-live-operation-status.js' => '901935ef', 'rsrc/js/application/files/behavior-icon-composer.js' => '8499b6ab', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', - 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => '0844f3c1', + 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => 'be6974cc', 'rsrc/js/application/herald/HeraldRuleEditor.js' => 'dca75c0e', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', @@ -579,7 +579,7 @@ return array( 'font-fontawesome' => 'e838e088', 'font-lato' => 'c7ccd872', 'global-drag-and-drop-css' => 'b556a948', - 'harbormaster-css' => 'fecac64f', + 'harbormaster-css' => 'c7e29d9e', 'herald-css' => 'cd8d0134', 'herald-rule-editor' => 'dca75c0e', 'herald-test-css' => 'a52e323e', @@ -636,7 +636,7 @@ return array( 'javelin-behavior-event-all-day' => 'b41537c9', 'javelin-behavior-fancy-datepicker' => 'ecf4e799', 'javelin-behavior-global-drag-and-drop' => '960f6a39', - 'javelin-behavior-harbormaster-log' => '0844f3c1', + 'javelin-behavior-harbormaster-log' => 'be6974cc', 'javelin-behavior-herald-rule-editor' => '7ebaeed3', 'javelin-behavior-high-security-warning' => 'a464fe03', 'javelin-behavior-history-install' => '7ee2b591', @@ -962,9 +962,6 @@ return array( 'javelin-stratcom', 'javelin-workflow', ), - '0844f3c1' => array( - 'javelin-behavior', - ), '08f4ccc3' => array( 'phui-oi-list-view-css', ), @@ -1892,6 +1889,9 @@ return array( 'javelin-util', 'javelin-request', ), + 'be6974cc' => array( + 'javelin-behavior', + ), 'bea6e7f4' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php b/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php index a2322dc51d..c1c4275982 100644 --- a/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php +++ b/src/applications/harbormaster/controller/HarbormasterBuildLogRenderController.php @@ -61,6 +61,7 @@ final class HarbormasterBuildLogRenderController 'offset' => $head_offset, 'lines' => $head_lines, 'direction' => 1, + 'limit' => $tail_offset, ); } @@ -69,6 +70,7 @@ final class HarbormasterBuildLogRenderController 'offset' => $tail_offset, 'lines' => $tail_lines, 'direction' => -1, + 'limit' => $head_offset, ); } @@ -128,6 +130,10 @@ final class HarbormasterBuildLogRenderController foreach ($views as $view_key => $view) { $anchor_byte = $view['offset']; + if ($view['direction'] < 0) { + $anchor_byte = $anchor_byte - 1; + } + $data_key = null; foreach ($reads as $read_key => $read) { $s = $read['fetchOffset']; @@ -148,7 +154,8 @@ final class HarbormasterBuildLogRenderController foreach ($reads[$data_key]['lines'] as $line_key => $line) { $s = $line['offset']; $e = $s + $line['length']; - if (($s <= $anchor_byte) && ($e >= $anchor_byte)) { + + if (($s <= $anchor_byte) && ($e > $anchor_byte)) { $anchor_key = $line_key; break; } @@ -161,9 +168,9 @@ final class HarbormasterBuildLogRenderController } if ($direction > 0) { - $slice_offset = $line_key; + $slice_offset = $anchor_key; } else { - $slice_offset = max(0, $line_key - ($view['lines'] - 1)); + $slice_offset = max(0, $anchor_key - ($view['lines'] - 1)); } $slice_length = $view['lines']; @@ -200,12 +207,23 @@ final class HarbormasterBuildLogRenderController $trim = ($view_offset - $data_offset); $view_length -= $trim; } + + $limit = $view['limit']; + if ($limit < ($view_offset + $view_length)) { + $view_length = ($limit - $view_offset); + } } else { $view_offset = $data_offset; $view_length = $data_length; if ($data_offset + $data_length > $view['offset']) { $view_length -= (($data_offset + $data_length) - $view['offset']); } + + $limit = $view['limit']; + if ($limit > $view_offset) { + $view_length -= ($limit - $view_offset); + $view_offset = $limit; + } } $views[$view_key] += array( @@ -232,12 +250,12 @@ final class HarbormasterBuildLogRenderController } $trim = ($view_offset - $line_offset); - $line_data = substr($line['data'], $trim); - if (!strlen($line_data)) { + if ($trim && ($trim >= strlen($line['data']))) { unset($lines[$line_key]); continue; } + $line_data = substr($line['data'], $trim); $lines[$line_key]['data'] = $line_data; $lines[$line_key]['length'] = strlen($line_data); $lines[$line_key]['offset'] += $trim; @@ -248,16 +266,16 @@ final class HarbormasterBuildLogRenderController foreach ($lines as $line_key => $line) { $line_end = $line['offset'] + $line['length']; if ($line_end <= $view_end) { - break; + continue; } $trim = ($line_end - $view_end); - $line_data = substr($line['data'], -$trim); - if (!strlen($line_data)) { + if ($trim && ($trim >= strlen($line['data']))) { unset($lines[$line_key]); continue; } + $line_data = substr($line['data'], -$trim); $lines[$line_key]['data'] = $line_data; $lines[$line_key]['length'] = strlen($line_data); } @@ -267,6 +285,17 @@ final class HarbormasterBuildLogRenderController $spacer = null; $render = array(); + + $head_view = head($views); + if ($head_view['viewOffset'] > $head_offset) { + $render[] = array( + 'spacer' => true, + 'head' => $head_offset, + 'tail' => $head_view['viewOffset'], + ); + } + + foreach ($views as $view) { if ($spacer) { $spacer['tail'] = $view['viewOffset']; @@ -281,49 +310,22 @@ final class HarbormasterBuildLogRenderController ); } + $tail_view = last($views); + if ($tail_view['viewOffset'] + $tail_view['viewLength'] < $tail_offset) { + $render[] = array( + 'spacer' => true, + 'head' => $tail_view['viewOffset'] + $tail_view['viewLength'], + 'tail' => $tail_offset, + ); + } + $uri = $log->getURI(); $highlight_range = $request->getURIData('lines'); $rows = array(); foreach ($render as $range) { if (isset($range['spacer'])) { - $rows[] = phutil_tag( - 'tr', - array(), - array( - phutil_tag( - 'th', - array(), - null), - phutil_tag( - 'td', - array(), - array( - javelin_tag( - 'a', - array( - 'sigil' => 'harbormaster-log-expand', - 'meta' => array( - 'headOffset' => $range['head'], - 'tailOffset' => $range['tail'], - 'head' => 4, - ), - ), - 'Show Up ^^^^'), - '... '.($range['tail'] - $range['head']).' bytes ...', - javelin_tag( - 'a', - array( - 'sigil' => 'harbormaster-log-expand', - 'meta' => array( - 'headOffset' => $range['head'], - 'tailOffset' => $range['tail'], - 'tail' => 4, - ), - ), - 'Show Down VVVV'), - )), - )); + $rows[] = $this->renderExpandRow($range); continue; } @@ -559,4 +561,108 @@ final class HarbormasterBuildLogRenderController return $views; } + private function renderExpandRow($range) { + + $icon_up = id(new PHUIIconView()) + ->setIcon('fa-chevron-up'); + + $icon_down = id(new PHUIIconView()) + ->setIcon('fa-chevron-down'); + + $up_text = array( + pht('Show More Above'), + ' ', + $icon_up, + ); + + $expand_up = javelin_tag( + 'a', + array( + 'sigil' => 'harbormaster-log-expand', + 'meta' => array( + 'headOffset' => $range['head'], + 'tailOffset' => $range['tail'], + 'head' => 4, + 'tail' => 0, + ), + ), + $up_text); + + $mid_text = pht( + 'Show More (%s bytes Hidden)', + new PhutilNumber($range['tail'] - $range['head'])); + + $expand_mid = javelin_tag( + 'a', + array( + 'sigil' => 'harbormaster-log-expand', + 'meta' => array( + 'headOffset' => $range['head'], + 'tailOffset' => $range['tail'], + 'head' => 2, + 'tail' => 2, + ), + ), + $mid_text); + + $down_text = array( + $icon_down, + ' ', + pht('Show More Below'), + ); + + $expand_down = javelin_tag( + 'a', + array( + 'sigil' => 'harbormaster-log-expand', + 'meta' => array( + 'headOffset' => $range['head'], + 'tailOffset' => $range['tail'], + 'head' => 0, + 'tail' => 4, + ), + ), + $down_text); + + $expand_cells = array( + phutil_tag( + 'td', + array( + 'class' => 'harbormaster-log-expand-up', + ), + $expand_up), + phutil_tag( + 'td', + array( + 'class' => 'harbormaster-log-expand-mid', + ), + $expand_mid), + phutil_tag( + 'td', + array( + 'class' => 'harbormaster-log-expand-down', + ), + $expand_down), + ); + $expand_row = phutil_tag('tr', array(), $expand_cells); + $expand_table = phutil_tag( + 'table', + array( + 'class' => 'harbormaster-log-expand-table', + ), + $expand_row); + + $cells = array( + phutil_tag('th', array()), + phutil_tag( + 'td', + array( + 'class' => 'harbormaster-log-expand-cell', + ), + $expand_table), + ); + + return phutil_tag('tr', array(), $cells); + } + } diff --git a/webroot/rsrc/css/application/harbormaster/harbormaster.css b/webroot/rsrc/css/application/harbormaster/harbormaster.css index f55139ade0..1c389a4383 100644 --- a/webroot/rsrc/css/application/harbormaster/harbormaster.css +++ b/webroot/rsrc/css/application/harbormaster/harbormaster.css @@ -37,7 +37,7 @@ color: {$lightgreytext}; } -.harbormaster-log-table th { +.harbormaster-log-table > tbody > tr > th { background-color: {$paste.highlight}; border-right: 1px solid {$paste.border}; @@ -48,23 +48,77 @@ user-select: none; } -.harbormaster-log-table th a { +.harbormaster-log-table > tbody > tr > th a { display: block; color: {$darkbluetext}; text-align: right; padding: 2px 6px 1px 12px; } -.harbormaster-log-table th a:hover { +.harbormaster-log-table > tbody > tr > th a:hover { background: {$paste.border}; } -.harbormaster-log-table td { +.harbormaster-log-table > tbody > tr > td { white-space: pre-wrap; padding: 2px 8px 1px; width: 100%; } -.harbormaster-log-table tr.harbormaster-log-highlighted td { +.harbormaster-log-table > tbody > tr > td.harbormaster-log-expand-cell { + padding: 4px 0; +} + +.harbormaster-log-table tr.harbormaster-log-highlighted > td { background: {$paste.highlight}; } + +.harbormaster-log-expand-table { + width: 100%; + background: {$paste.highlight}; + border-top: 1px solid {$paste.border}; + border-bottom: 1px solid {$paste.border}; +} + +.harbormaster-log-expand-table a { + display: block; + padding: 6px 16px; + color: {$darkbluetext}; +} + +.device-desktop .harbormaster-log-expand-table a:hover { + background: {$paste.border}; + text-decoration: none; +} + +.harbormaster-log-expand-table td { + vertical-align: middle; + font: 13px 'Segoe UI', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Lato', + 'Helvetica Neue', Helvetica, Arial, sans-serif; +} + + +.harbormaster-log-expand-up { + text-align: right; + width: 50%; +} + +.harbormaster-log-expand-up .phui-icon-view { + margin: 0 0 0px 4px; +} + +.harbormaster-log-expand-mid { + text-align: center; + white-space: nowrap; + border-left: 1px solid {$paste.border}; + border-right: 1px solid {$paste.border}; +} + +.harbormaster-log-expand-down { + text-align: left; + width: 50%; +} + +.harbormaster-log-expand-down .phui-icon-view { + margin: 0 4px 0 0; +} diff --git a/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js index d011233afd..38baca6199 100644 --- a/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js +++ b/webroot/rsrc/js/application/harbormaster/behavior-harbormaster-log.js @@ -14,6 +14,8 @@ JX.behavior('harbormaster-log', function(config) { e.kill(); var row = e.getNode('tag:tr'); + row = JX.DOM.findAbove(row, 'tr'); + var data = e.getNodeData('harbormaster-log-expand'); var uri = new JX.URI(config.renderURI) @@ -21,7 +23,7 @@ JX.behavior('harbormaster-log', function(config) { var request = new JX.Request(uri, function(r) { var result = JX.$H(r.markup).getNode(); - var rows = JX.DOM.scry(result, 'tr'); + var rows = [].slice.apply(result.firstChild.childNodes); JX.DOM.replace(row, rows); }); From dba4c4bdf6a850ab9f693ecebedbd0d90aad08d4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 27 Feb 2018 06:56:15 -0800 Subject: [PATCH 14/39] Emit a "Content-Security-Policy" HTTP header Summary: See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `