Update Phage for toolsets and restore library loading behaviors

Summary: Ref T13490. This makes the "phage" toolset work properly and updates external library behavior to match the "classic" behavior, except that "--library" is now supported.

Test Plan: Ran "phage remote" workflows.

Maniphest Tasks: T13490

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

src/__phutil_library_map__.php

@@ -346,7 +346,6 @@ phutil_register_library_map(array(
     'ArcanistParseStrUseXHPASTLinterRuleTestCase' => 'lint/linter/xhpast/rules/__tests__/ArcanistParseStrUseXHPASTLinterRuleTestCase.php',
     'ArcanistPasteWorkflow' => 'workflow/ArcanistPasteWorkflow.php',
     'ArcanistPatchWorkflow' => 'workflow/ArcanistPatchWorkflow.php',
-    'ArcanistPhageToolset' => 'toolset/ArcanistPhageToolset.php',
     'ArcanistPhpLinter' => 'lint/linter/ArcanistPhpLinter.php',
     'ArcanistPhpLinterTestCase' => 'lint/linter/__tests__/ArcanistPhpLinterTestCase.php',
     'ArcanistPhpcsLinter' => 'lint/linter/ArcanistPhpcsLinter.php',
@@ -544,6 +543,7 @@ phutil_register_library_map(array(
     'PhagePHPAgent' => 'phage/agent/PhagePHPAgent.php',
     'PhagePHPAgentBootloader' => 'phage/bootloader/PhagePHPAgentBootloader.php',
     'PhagePlanAction' => 'phage/action/PhagePlanAction.php',
+    'PhageToolset' => 'phage/toolset/PhageToolset.php',
     'PhageWorkflow' => 'phage/workflow/PhageWorkflow.php',
     'Phobject' => 'object/Phobject.php',
     'PhobjectTestCase' => 'object/__tests__/PhobjectTestCase.php',
@@ -1288,7 +1288,6 @@ phutil_register_library_map(array(
     'ArcanistParseStrUseXHPASTLinterRuleTestCase' => 'ArcanistXHPASTLinterRuleTestCase',
     'ArcanistPasteWorkflow' => 'ArcanistWorkflow',
     'ArcanistPatchWorkflow' => 'ArcanistWorkflow',
-    'ArcanistPhageToolset' => 'ArcanistToolset',
     'ArcanistPhpLinter' => 'ArcanistExternalLinter',
     'ArcanistPhpLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistPhpcsLinter' => 'ArcanistExternalLinter',
@@ -1491,7 +1490,8 @@ phutil_register_library_map(array(
     'PhagePHPAgent' => 'Phobject',
     'PhagePHPAgentBootloader' => 'PhageAgentBootloader',
     'PhagePlanAction' => 'PhageAction',
-    'PhageWorkflow' => 'PhutilArgumentWorkflow',
+    'PhageToolset' => 'ArcanistToolset',
+    'PhageWorkflow' => 'ArcanistWorkflow',
     'Phobject' => 'Iterator',
     'PhobjectTestCase' => 'PhutilTestCase',
     'PhpunitTestEngine' => 'ArcanistUnitTestEngine',

src/phage/toolset/PhageToolset.php

@@ -0,0 +1,7 @@
+<?php
+
+final class PhageToolset extends ArcanistToolset {
+
+  const TOOLSETKEY = 'phage';
+
+}

src/phage/workflow/PhageWorkflow.php

@@ -1,10 +1,11 @@
 <?php
 
 abstract class PhageWorkflow
-  extends PhutilArgumentWorkflow {
+  extends ArcanistWorkflow {
 
-  public function isExecutable() {
+  public function supportsToolset(ArcanistToolset $toolset) {
-    return true;
+    $key = $toolset->getToolsetKey();
+    return ($key === PhageToolset::TOOLSETKEY);
   }
 
 }

src/runtime/ArcanistRuntime.php

@@ -92,7 +92,7 @@ final class ArcanistRuntime {
     $config_engine = $this->loadConfiguration($args);
     $config = $config_engine->newConfigurationSourceList();
 
-    $this->loadLibraries($args, $config);
+    $this->loadLibraries($config_engine, $config, $args);
 
     // Now that we've loaded libraries, we can validate configuration.
     // Do this before continuing since configuration can impact other
@@ -300,91 +300,70 @@ final class ArcanistRuntime {
   }
 
   private function loadLibraries(
-    PhutilArgumentParser $args,
+    ArcanistConfigurationEngine $engine,
-    ArcanistConfigurationSourceList $config) {
+    ArcanistConfigurationSourceList $config,
+    PhutilArgumentParser $args) {
 
-    // TOOLSETS: Make this work again -- or replace it entirely with package
+    $sources = array();
-    // management?
-    return;
-
-    $is_trace = $args->getArg('trace');
-
-    $load = array();
-    $working_copy = $this->getWorkingCopy();
 
     $cli_libraries = $args->getArg('library');
     if ($cli_libraries) {
-      $load[] = array(
+      $sources = array();
-        '--library',
+      foreach ($cli_libraries as $cli_library) {
-        $cli_libraries,
+        $sources[] = array(
-      );
+          'type' => 'flag',
+          'library-source' => $cli_library,
+        );
+      }
     } else {
-      $system_config = $config->readSystemArcConfig();
+      $items = $config->getStorageValueList('load');
-      $load[] = array(
+      foreach ($items as $item) {
-        $config->getSystemArcConfigLocation(),
+        foreach ($item->getValue() as $library_path) {
-        idx($system_config, 'load', array()),
+          $sources[] = array(
-      );
+            'type' => 'config',
-
+            'config-source' => $item->getConfigurationSource(),
-      $global_config = $config->readUserArcConfig();
+            'library-source' => $library_path,
-      $load[] = array(
+          );
-        $config->getUserConfigurationFileLocation(),
+        }
-        idx($global_config, 'load', array()),
+      }
-      );
-
-      $load[] = array(
-        '.arcconfig',
-        $working_copy->getProjectConfig('load'),
-      );
-
-      $load[] = array(
-        // TODO: We could explain exactly where this is coming from more
-        // clearly.
-        './.../arc/config',
-        $working_copy->getLocalConfig('load'),
-      );
-
-      $load[] = array(
-        '--config load=...',
-        $config->getRuntimeConfig('load', array()),
-      );
     }
 
-    foreach ($load as $spec) {
+    foreach ($sources as $spec) {
-      list($source, $libraries) = $spec;
+      $library_source = $spec['library-source'];
-      if ($is_trace) {
+
-        $this->logTrace(
+      switch ($spec['type']) {
-          pht('LOAD'),
+        case 'flag':
-          pht(
+          $description = pht('runtime --library flag');
-            'Loading libraries from "%s"...',
+          break;
-            $source));
+        case 'config':
+          $config_source = $spec['config-source'];
+          $description = pht(
+            'Configuration (%s)',
+            $config_source->getSourceDisplayName());
+          break;
       }
 
-      if (!$libraries) {
+      $this->loadLibrary($engine, $library_source, $description);
-        if ($is_trace) {
-          $this->logTrace(pht('NONE'), pht('Nothing to load.'));
-        }
-        continue;
-      }
-
-      if (!is_array($libraries)) {
-        throw new PhutilArgumentUsageException(
-          pht(
-            'Libraries specified by "%s" are not formatted correctly. '.
-            'Expected a list of paths. Check your configuration.',
-            $source));
-      }
-
-      foreach ($libraries as $library) {
-        $this->loadLibrary($source, $library, $working_copy, $is_trace);
-      }
     }
   }
 
   private function loadLibrary(
-    $source,
+    ArcanistConfigurationEngine $engine,
     $location,
-    ArcanistWorkingCopyIdentity $working_copy,
+    $description) {
-    $is_trace) {
+
+    // TODO: This is a legacy system that should be replaced with package
+    // management.
+
+    $log = $this->getLogEngine();
+    $working_copy = $engine->getWorkingCopy();
+    if ($working_copy) {
+      $working_copy_root = $working_copy->getPath();
+      $working_directory = $working_copy->getWorkingDirectory();
+    } else {
+      $working_copy_root = null;
+      $working_directory = getcwd();
+    }
 
     // Try to resolve the library location. We look in several places, in
     // order:
@@ -406,30 +385,44 @@ final class ArcanistRuntime {
 
     // Check inside the working copy. This also checks absolute paths, since
     // they'll resolve absolute and just ignore the project root.
-    $resolved_location = Filesystem::resolvePath(
+    if ($working_copy_root !== null) {
-      $location,
-      $working_copy->getProjectRoot());
-    if (Filesystem::pathExists($resolved_location)) {
-      $location = $resolved_location;
-      $resolved = true;
-    }
-
-    // If we didn't find anything, check alongside the working copy.
-    if (!$resolved) {
       $resolved_location = Filesystem::resolvePath(
         $location,
-        dirname($working_copy->getProjectRoot()));
+        $working_copy_root);
+      if (Filesystem::pathExists($resolved_location)) {
+        $location = $resolved_location;
+        $resolved = true;
+      }
+
+      // If we didn't find anything, check alongside the working copy.
+      if (!$resolved) {
+        $resolved_location = Filesystem::resolvePath(
+          $location,
+          dirname($working_copy_root));
+        if (Filesystem::pathExists($resolved_location)) {
+          $location = $resolved_location;
+          $resolved = true;
+        }
+      }
+    }
+
+    // Look beside "arcanist/". This is rule (3) above.
+
+    if (!$resolved) {
+      $arcanist_root = phutil_get_library_root('arcanist');
+      $arcanist_root = dirname($arcanist_root);
+      $resolved_location = Filesystem::resolvePath(
+        $location,
+        $arcanist_root);
       if (Filesystem::pathExists($resolved_location)) {
         $location = $resolved_location;
         $resolved = true;
       }
     }
 
-    if ($is_trace) {
+    $log->writeTrace(
-      $this->logTrace(
+      pht('LOAD'),
-        pht('LOAD'),
+      pht('Loading library from "%s"...', $location));
-        pht('Loading phutil library from "%s"...', $location));
-    }
 
     $error = null;
     try {
@@ -445,7 +438,7 @@ final class ArcanistRuntime {
             'is specified by "%s". Check that the setting is correct and '.
             'the library is located in the right place.',
             $location,
-            $source)));
+            $description)));
 
       $prompt = pht('Continue without loading library?');
       if (!phutil_console_confirm($prompt)) {

src/toolset/ArcanistPhageToolset.php

@@ -1,8 +0,0 @@
-<?php
-
-final class ArcanistPhageToolset extends ArcanistToolset {
-
-  const TOOLSETKEY = 'phage';
-
-
-}