From a27c83757d9fd73b1d9f79b0ae4268a636f35b6f Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 23 Jul 2020 12:17:56 -0700 Subject: [PATCH] Remove ancient "phd.trace" and "phd.verbose" configuration options Summary: Ref T13556. These options are very old and effectively obsoleted by "bin/phd debug [--trace]". I haven't used either option diagnostically in many years, and they aren't mentioned in the documentation. Remove them to simplify configuration, and because "phd.trace" doesn't work anyway and likely hasn't for a long time -- it has specific issues with TTY detection (see T13556). Test Plan: Grepped for "phd.trace" and "phd.verbose". Ran "bin/phd debug [--trace]" and saw verbose/trace output. Maniphest Tasks: T13556 Differential Revision: https://secure.phabricator.com/D21426 --- .../check/PhabricatorDaemonsSetupCheck.php | 21 ++++++++---- .../PhabricatorExtraConfigSetupCheck.php | 6 ++++ .../option/PhabricatorPHDConfigOptions.php | 32 ------------------- .../PhabricatorDaemonManagementWorkflow.php | 4 +-- .../workers/PhabricatorTaskmasterDaemon.php | 2 +- 5 files changed, 23 insertions(+), 42 deletions(-) diff --git a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php index 331a4f0908..7ef44dc384 100644 --- a/src/applications/config/check/PhabricatorDaemonsSetupCheck.php +++ b/src/applications/config/check/PhabricatorDaemonsSetupCheck.php @@ -8,14 +8,21 @@ final class PhabricatorDaemonsSetupCheck extends PhabricatorSetupCheck { protected function executeChecks() { - $task_daemon = id(new PhabricatorDaemonLogQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) - ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) - ->setLimit(1) - ->execute(); + try { + $task_daemons = id(new PhabricatorDaemonLogQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withStatus(PhabricatorDaemonLogQuery::STATUS_ALIVE) + ->withDaemonClasses(array('PhabricatorTaskmasterDaemon')) + ->setLimit(1) + ->execute(); - if (!$task_daemon) { + $no_daemons = !$task_daemons; + } catch (Exception $ex) { + // Just skip this warning if the query fails for some reason. + $no_daemons = false; + } + + if ($no_daemons) { $doc_href = PhabricatorEnv::getDoclink('Managing Daemons with phd'); $summary = pht( diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index d863c928b7..1de39f3468 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -322,6 +322,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'directly supported. Prefixes and other strings may be customized with '. '"translation.override".'); + $phd_reason = pht( + 'Use "bin/phd debug ..." to get a detailed daemon execution log.'); + $ancient_config += array( 'phid.external-loaders' => pht( @@ -539,6 +542,9 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'phd.pid-directory' => pht( 'Phabricator daemons no longer use PID files.'), + + 'phd.trace' => $phd_reason, + 'phd.verbose' => $phd_reason, ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorPHDConfigOptions.php b/src/applications/config/option/PhabricatorPHDConfigOptions.php index 7a1d39e617..259660d562 100644 --- a/src/applications/config/option/PhabricatorPHDConfigOptions.php +++ b/src/applications/config/option/PhabricatorPHDConfigOptions.php @@ -43,22 +43,6 @@ final class PhabricatorPHDConfigOptions "configuration changes are picked up by the daemons ". "automatically, but pool sizes can not be changed without a ". "restart.")), - $this->newOption('phd.verbose', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Verbose mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'verbose' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'verbose' mode by default. This creates a lot ". - "of output, but can help debug issues. Daemons launched in debug ". - "mode with '%s' are always launched in verbose mode. ". - "See also '%s'.", - 'phd debug', - 'phd.trace')), $this->newOption('phd.user', 'string', null) ->setLocked(true) ->setSummary(pht('System user to run daemons as.')) @@ -68,22 +52,6 @@ final class PhabricatorPHDConfigOptions 'user will own the working copies of any repositories that '. 'Phabricator imports or manages. This option is new and '. 'experimental.')), - $this->newOption('phd.trace', 'bool', false) - ->setLocked(true) - ->setBoolOptions( - array( - pht('Trace mode'), - pht('Normal mode'), - )) - ->setSummary(pht("Launch daemons in 'trace' mode by default.")) - ->setDescription( - pht( - "Launch daemons in 'trace' mode by default. This creates an ". - "ENORMOUS amount of output, but can help debug issues. Daemons ". - "launched in debug mode with '%s' are always launched in ". - "trace mode. See also '%s'.", - 'phd debug', - 'phd.verbose')), $this->newOption('phd.garbage-collection', 'wild', array()) ->setLocked(true) ->setLockedMessage( diff --git a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php index 05d94e218d..b9645323c2 100644 --- a/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php +++ b/src/applications/daemon/management/PhabricatorDaemonManagementWorkflow.php @@ -116,11 +116,11 @@ abstract class PhabricatorDaemonManagementWorkflow $trace = PhutilArgumentParser::isTraceModeEnabled(); $flags = array(); - if ($trace || PhabricatorEnv::getEnvConfig('phd.trace')) { + if ($trace) { $flags[] = '--trace'; } - if ($debug || PhabricatorEnv::getEnvConfig('phd.verbose')) { + if ($debug) { $flags[] = '--verbose'; } diff --git a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php index c2276843db..12b06131d8 100644 --- a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php +++ b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php @@ -24,7 +24,7 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon { if ($ex) { if ($ex instanceof PhabricatorWorkerPermanentFailureException) { // NOTE: Make sure these reach the daemon log, even when not - // running in "phd.verbose" mode. See T12803 for discussion. + // running in verbose mode. See T12803 for discussion. $log_exception = new PhutilProxyException( pht( 'Task "%s" encountered a permanent failure and was '.