Introduce a rough abstract base class for "linters which run programs and read the results"

Summary:
Ref T3186. We have about 50 linters which run programs and read the results, all of which have ad-hoc one-off custom config that isn't formalized anywhere.

Consolidate all this stuff into `ArcanistExternalLinter`, which is configurable through `.arclint` (although nothing supports this quite yet).

Extend CSSLint and Pep8Lint from `ArcanistExternalLinter`.

Add unit tests for both.

There are still some rough edges here, but it mostly seems to work pretty well.

Test Plan: Ran unit tests, hit some (most?) of the error cases.

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran, Firehed

Maniphest Tasks: T3186

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

src/__phutil_library_map__.php

@@ -29,6 +29,7 @@ phutil_register_library_map(array(
     'ArcanistBundle' => 'parser/ArcanistBundle.php',
     'ArcanistBundleTestCase' => 'parser/__tests__/ArcanistBundleTestCase.php',
     'ArcanistCSSLintLinter' => 'lint/linter/ArcanistCSSLintLinter.php',
+    'ArcanistCSSLintLinterTestCase' => 'lint/linter/__tests__/ArcanistCSSLintLinterTestCase.php',
     'ArcanistCallConduitWorkflow' => 'workflow/ArcanistCallConduitWorkflow.php',
     'ArcanistCapabilityNotSupportedException' => 'workflow/exception/ArcanistCapabilityNotSupportedException.php',
     'ArcanistChooseInvalidRevisionException' => 'exception/ArcanistChooseInvalidRevisionException.php',
@@ -60,6 +61,7 @@ phutil_register_library_map(array(
     'ArcanistDownloadWorkflow' => 'workflow/ArcanistDownloadWorkflow.php',
     'ArcanistEventType' => 'events/constant/ArcanistEventType.php',
     'ArcanistExportWorkflow' => 'workflow/ArcanistExportWorkflow.php',
+    'ArcanistExternalLinter' => 'lint/linter/ArcanistExternalLinter.php',
     'ArcanistFeatureWorkflow' => 'workflow/ArcanistFeatureWorkflow.php',
     'ArcanistFilenameLinter' => 'lint/linter/ArcanistFilenameLinter.php',
     'ArcanistFlagWorkflow' => 'workflow/ArcanistFlagWorkflow.php',
@@ -107,6 +109,7 @@ phutil_register_library_map(array(
     'ArcanistNoLintLinter' => 'lint/linter/ArcanistNoLintLinter.php',
     'ArcanistNoLintTestCaseMisnamed' => 'lint/linter/__tests__/ArcanistNoLintTestCase.php',
     'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php',
+    'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php',
     'ArcanistPasteWorkflow' => 'workflow/ArcanistPasteWorkflow.php',
     'ArcanistPatchWorkflow' => 'workflow/ArcanistPatchWorkflow.php',
     'ArcanistPhpcsLinter' => 'lint/linter/ArcanistPhpcsLinter.php',
@@ -191,7 +194,8 @@ phutil_register_library_map(array(
     'ArcanistBritishTestCase' => 'ArcanistTestCase',
     'ArcanistBrowseWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistBundleTestCase' => 'ArcanistTestCase',
-    'ArcanistCSSLintLinter' => 'ArcanistLinter',
+    'ArcanistCSSLintLinter' => 'ArcanistExternalLinter',
+    'ArcanistCSSLintLinterTestCase' => 'ArcanistArcanistLinterTestCase',
     'ArcanistCallConduitWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistCapabilityNotSupportedException' => 'Exception',
     'ArcanistChooseInvalidRevisionException' => 'Exception',
@@ -213,6 +217,7 @@ phutil_register_library_map(array(
     'ArcanistDownloadWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistEventType' => 'PhutilEventType',
     'ArcanistExportWorkflow' => 'ArcanistBaseWorkflow',
+    'ArcanistExternalLinter' => 'ArcanistFutureLinter',
     'ArcanistFeatureWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistFilenameLinter' => 'ArcanistLinter',
     'ArcanistFlagWorkflow' => 'ArcanistBaseWorkflow',
@@ -248,7 +253,8 @@ phutil_register_library_map(array(
     'ArcanistNoEngineException' => 'ArcanistUsageException',
     'ArcanistNoLintLinter' => 'ArcanistLinter',
     'ArcanistNoLintTestCaseMisnamed' => 'ArcanistLinterTestCase',
-    'ArcanistPEP8Linter' => 'ArcanistFutureLinter',
+    'ArcanistPEP8Linter' => 'ArcanistExternalLinter',
+    'ArcanistPEP8LinterTestCase' => 'ArcanistArcanistLinterTestCase',
     'ArcanistPasteWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistPatchWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistPhpcsLinter' => 'ArcanistLinter',

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php

@@ -36,24 +36,47 @@ final class ArcanistConfigurationDrivenLintEngine extends ArcanistLintEngine {
     $built_linters = array();
     $all_paths = $this->getPaths();
     foreach ($config['linters'] as $name => $spec) {
+      $type = idx($spec, 'type');
+      if ($type !== null) {
+        if (empty($linters[$type])) {
+          $list = implode(', ', array_keys($linters));
+          throw new Exception(
+            "Linter '{$name}' specifies invalid type '{$type}'. Available ".
+            "linters are: {$list}.");
+        }
+
+        $linter = clone $linters[$type];
+        $linter->setEngine($this);
+        $more = $linter->getLinterConfigurationOptions();
+      } else {
+        // We'll raise an error below about the invalid "type" key.
+        $linter = null;
+        $more = array();
+      }
+
       PhutilTypeSpec::checkMap(
         $spec,
         array(
           'type' => 'string',
           'include' => 'optional string | list<string>',
           'exclude' => 'optional string | list<string>',
-        ));
+        ) + $more);
 
-      $type = $spec['type'];
-      if (empty($linters[$type])) {
-        $list = implode(', ', array_keys($linters));
-        throw new Exception(
-          "Linter '{$name}' specifies invalid type '{$type}'. Available ".
-          "linters are: {$list}.");
+      foreach ($more as $key => $value) {
+        if (array_key_exists($key, $spec)) {
+          try {
+            $linter->setLinterConfigurationValue($key, $spec);
+          } catch (Exception $ex) {
+            $message = pht(
+              'Error in parsing ".arclint" file, in key "%s" for '.
+              'linter "%s".',
+              $key,
+              $name);
+            throw new PhutilProxyException($message, $ex);
+          }
+        }
       }
 
-      $linter = clone $linters[$type];
-
       $include = (array)idx($spec, 'include', array());
       $exclude = (array)idx($spec, 'exclude', array());
 
@@ -69,6 +92,7 @@ final class ArcanistConfigurationDrivenLintEngine extends ArcanistLintEngine {
 
       $linter->setPaths($paths);
 
+
       $built_linters[] = $linter;
     }
 
@@ -180,4 +204,5 @@ final class ArcanistConfigurationDrivenLintEngine extends ArcanistLintEngine {
     call_user_func_array(array($console, 'writeErr'), $argv);
   }
 
+
 }

src/lint/linter/ArcanistCSSLintLinter.php

@@ -13,81 +13,50 @@
  *
  * @group linter
  */
-final class ArcanistCSSLintLinter extends ArcanistLinter {
-
-  private $reports;
+final class ArcanistCSSLintLinter extends ArcanistExternalLinter {
 
   public function getLinterName() {
     return 'CSSLint';
   }
 
-  public function getLintSeverityMap() {
-    return array();
+  public function getMandatoryFlags() {
+    return '--format=lint-xml';
   }
 
-  public function getLintNameMap() {
-    return array();
-  }
-
-  public function getCSSLintOptions() {
+  public function getDefaultFlags() {
     $working_copy = $this->getEngine()->getWorkingCopy();
 
     $options = $working_copy->getConfig('lint.csslint.options');
+    // TODO: Deprecation warning.
 
     return $options;
   }
 
-  private function getCSSLintPath() {
+  public function getDefaultBinary() {
+    // TODO: Deprecation warning.
     $working_copy = $this->getEngine()->getWorkingCopy();
     $bin = $working_copy->getConfig('lint.csslint.bin');
-
-    if ($bin === null) {
-      $bin = 'csslint';
+    if ($bin) {
+      return $bin;
     }
 
-    return $bin;
+    return 'csslint';
   }
 
-  public function willLintPaths(array $paths) {
-    $csslint_bin = $this->getCSSLintPath();
-    $csslint_options = $this->getCSSLintOptions();
-    $futures = array();
-
-    foreach ($paths as $path) {
-      $filepath = $this->getEngine()->getFilePathOnDisk($path);
-      $this->reports[$path] = new TempFile();
-      $futures[$path] = new ExecFuture('%C %C --format=lint-xml >%s %s',
-        $csslint_bin,
-        $csslint_options,
-        $this->reports[$path],
-        $filepath);
-    }
-
-    foreach (Futures($futures)->limit(8) as $path => $future) {
-      $this->results[$path] = $future->resolve();
-    }
-
-    libxml_use_internal_errors(true);
+  public function getInstallInstructions() {
+    return pht('Install CSSLint using `npm install -g csslint`.');
   }
 
-  public function lintPath($path) {
-    list($rc, $stdout) = $this->results[$path];
+  protected function parseLinterOutput($path, $err, $stdout, $stderr) {
+    $report_dom = new DOMDocument();
+    $ok = @$report_dom->loadXML($stdout);
 
-    $report = Filesystem::readFile($this->reports[$path]);
-
-    if ($report) {
-      $report_dom = new DOMDocument();
-      libxml_clear_errors();
-      $report_dom->loadXML($report);
-    }
-    if (!$report || libxml_get_errors()) {
-      throw new ArcanistUsageException('CSS Linter failed to load ' .
-        'reporting file. Something happened when running csslint. ' .
-        "Output:\n$stdout" .
-        "\nTry running lint with --trace flag to get more details.");
+    if (!$ok) {
+      return false;
     }
 
     $files = $report_dom->getElementsByTagName('file');
+    $messages = array();
     foreach ($files as $file) {
       foreach ($file->childNodes as $child) {
         if (!($child instanceof DOMElement)) {
@@ -118,8 +87,10 @@ final class ArcanistCSSLintLinter extends ArcanistLinter {
           $message->setOriginalText($text);
         }
 
-        $this->addLintMessage($message);
+        $messages[] = $message;
       }
     }
+
+    return $messages;
   }
 }

src/lint/linter/ArcanistExternalLinter.php

@@ -0,0 +1,473 @@
+<?php
+
+/**
+ * Base class for linters which operate by invoking an external program and
+ * parsing results.
+ *
+ * @task bin      Interpreters, Binaries and Flags
+ * @task parse    Parsing Linter Output
+ * @task exec     Executing the Linter
+ */
+abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
+
+  private $bin;
+  private $interpreter;
+  private $flags;
+
+
+/* -(  Interpreters, Binaries and Flags  )----------------------------------- */
+
+
+  /**
+   * Return the default binary name or binary path where the external linter
+   * lives. This can either be a binary name which is expected to be installed
+   * in PATH (like "jshint"), or a relative path from the project root
+   * (like "resources/support/bin/linter") or an absolute path.
+   *
+   * If the binary needs an interpreter (like "python" or "node"), you should
+   * also override @{method:shouldUseInterpreter} and provide the interpreter
+   * in @{method:getDefaultInterpreter}.
+   *
+   * @return string Default binary to execute.
+   * @task bin
+   */
+  abstract public function getDefaultBinary();
+
+
+  /**
+   * Return a human-readable string describing how to install the linter. This
+   * is normally something like "Install such-and-such by running `npm install
+   * -g such-and-such`.", but will differ from linter to linter.
+   *
+   * @return string Human readable install instructions
+   * @task bin
+   */
+  abstract public function getInstallInstructions();
+
+
+  /**
+   * Return true to continue when the external linter exits with an error code.
+   * By default, linters which exit with an error code are assumed to have
+   * failed. However, some linters exit with a specific code to indicate that
+   * lint messages were detected.
+   *
+   * If the linter sometimes raises errors during normal operation, override
+   * this method and return true so execution continues when it exits with
+   * a nonzero status.
+   *
+   * @param bool  Return true to continue on nonzero error code.
+   * @task bin
+   */
+  public function shouldExpectCommandErrors() {
+    return false;
+  }
+
+
+  /**
+   * Return true to indicate that the external linter can read input from
+   * stdin, rather than requiring a file. If this mode is supported, it is
+   * slightly more flexible and may perform better, and is thus preferable.
+   *
+   * To send data over stdin instead of via a command line parameter, override
+   * this method and return true. If the linter also needs a command line
+   * flag (like `--stdin` or `-`), override
+   * @{method:getReadDataFromStdinFilename} to provide it.
+   *
+   * For example, linters are normally invoked something like this:
+   *
+   *   $ linter file.js
+   *
+   * If you override this method, invocation will be more similar to this:
+   *
+   *   $ linter < file.js
+   *
+   * If you additionally override @{method:getReadDataFromStdinFilename} to
+   * return `"-"`, invocation will be similar to this:
+   *
+   *   $ linter - < file.js
+   *
+   * @return bool True to send data over stdin.
+   * @task bin
+   */
+  public function supportsReadDataFromStdin() {
+    return false;
+  }
+
+
+  /**
+   * If the linter can read data over stdin, override
+   * @{method:supportsReadDataFromStdin} and then optionally override this
+   * method to provide any required arguments (like `-` or `--stdin`). See
+   * that method for discussion.
+   *
+   * @return string|null  Additional arguments required by the linter when
+   *                      operating in stdin mode.
+   * @task bin
+   */
+  public function getReadDataFromStdinFilename() {
+    return null;
+  }
+
+
+  /**
+   * Provide mandatory, non-overridable flags to the linter. Generally these
+   * are format flags, like `--format=xml`, which must always be given for
+   * the output to be usable.
+   *
+   * Flags which are not mandatory should be provided in
+   * @{method:getDefaultFlags} instead.
+   *
+   * @return string|null  Mandatory flags, like `"--format=xml"`.
+   * @task bin
+   */
+  protected function getMandatoryFlags() {
+    return null;
+  }
+
+
+  /**
+   * Provide default, overridable flags to the linter. Generally these are
+   * configuration flags which affect behavior but aren't critical. Flags
+   * which are required should be provided in @{method:getMandatoryFlags}
+   * instead.
+   *
+   * Default flags can be overridden with @{method:setFlags}.
+   *
+   * @return string|null  Overridable default flags.
+   * @task bin
+   */
+  protected function getDefaultFlags() {
+    return null;
+  }
+
+
+  /**
+   * Override default flags with custom flags. If not overridden, flags provided
+   * by @{method:getDefaultFlags} are used.
+   *
+   * @param string New flags.
+   * @return this
+   * @task bin
+   */
+  final public function setFlags($flags) {
+    $this->flags = $flags;
+    return $this;
+  }
+
+
+  /**
+   * Return the binary or script to execute. This method synthesizes defaults
+   * and configuration. You can override the binary with @{method:setBinary}.
+   *
+   * @return string Binary to execute.
+   * @task bin
+   */
+  final public function getBinary() {
+    return coalesce($this->bin, $this->getDefaultBinary());
+  }
+
+
+  /**
+   * Override the default binary with a new one.
+   *
+   * @param string  New binary.
+   * @return this
+   * @task bin
+   */
+  final public function setBinary($bin) {
+    $this->bin = $bin;
+    return $this;
+  }
+
+
+  /**
+   * Return true if this linter should use an interpreter (like "python" or
+   * "node") in addition to the script.
+   *
+   * After overriding this method to return `true`, override
+   * @{method:getDefaultInterpreter} to set a default.
+   *
+   * @return bool True to use an interpreter.
+   * @task bin
+   */
+  public function shouldUseInterpreter() {
+    return false;
+  }
+
+
+  /**
+   * Return the default interpreter, like "python" or "node". This method is
+   * only invoked if @{method:shouldUseInterpreter} has been overridden to
+   * return `true`.
+   *
+   * @return string Default interpreter.
+   * @task bin
+   */
+  public function getDefaultInterpreter() {
+    throw new Exception("Incomplete implementation!");
+  }
+
+
+  /**
+   * Get the effective interpreter. This method synthesizes configuration and
+   * defaults.
+   *
+   * @return string Effective interpreter.
+   * @task bin
+   */
+  final public function getInterpreter() {
+    return coalesce($this->interpreter, $this->getDefaultInterpreter());
+  }
+
+
+  /**
+   * Set the interpreter, overriding any default.
+   *
+   * @param string New interpreter.
+   * @return this
+   * @task bin
+   */
+  final public function setInterpreter($interpreter) {
+    $this->interpreter = $interpreter;
+    return $this;
+  }
+
+
+/* -(  Parsing Linter Output  )---------------------------------------------- */
+
+
+  /**
+   * Parse the output of the external lint program into objects of class
+   * @{class:ArcanistLintMessage} which `arc` can consume. Generally, this
+   * means examining the output and converting each warning or error into a
+   * message.
+   *
+   * If parsing fails, returning `false` will cause the caller to throw an
+   * appropriate exception. (You can also throw a more specific exception if
+   * you're able to detect a more specific condition.) Otherwise, return a list
+   * of messages.
+   *
+   * @param  string   Path to the file being linted.
+   * @param  int      Exit code of the linter.
+   * @param  string   Stdout of the linter.
+   * @param  string   Stderr of the linter.
+   * @return list<ArcanistLintMessage>|false  List of lint messages, or false
+   *                                          to indicate parser failure.
+   * @task parse
+   */
+  abstract protected function parseLinterOutput($path, $err, $stdout, $stderr);
+
+
+/* -(  Executing the Linter  )----------------------------------------------- */
+
+
+  /**
+   * Check that the binary and interpreter (if applicable) exist, and throw
+   * an exception with a message about how to install them if they do not.
+   *
+   * @return void
+   */
+  final public function checkBinaryConfiguration() {
+    $interpreter = null;
+    if ($this->shouldUseInterpreter()) {
+      $interpreter = $this->getInterpreter();
+    }
+
+    $binary = $this->getBinary();
+
+    // NOTE: If we have an interpreter, we don't require the script to be
+    // executable (so we just check that the path exists). Otherwise, the
+    // binary must be executable.
+
+    if ($interpreter) {
+      if (!Filesystem::binaryExists($interpreter)) {
+        throw new ArcanistUsageException(
+          pht(
+            'Unable to locate interpreter "%s" to run linter %s. You may '.
+            'need to install the intepreter, or adjust your linter '.
+            'configuration.',
+            "\nTO INSTALL: %s",
+            $interpreter,
+            get_class($this),
+            $this->getInstallInstructions()));
+      }
+      if (!Filesystem::pathExists($binary)) {
+        throw new ArcanistUsageException(
+          pht(
+            'Unable to locate script "%s" to run linter %s. You may need '.
+            'to install the script, or adjust your linter configuration. '.
+            "\nTO INSTALL: %s",
+            $binary,
+            get_class($this),
+            $this->getInstallInstructions()));
+      }
+    } else {
+      if (!Filesystem::binaryExists($binary)) {
+        throw new ArcanistUsageException(
+          pht(
+            'Unable to locate binary "%s" to run linter %s. You may need '.
+            'to install the binary, or adjust your linter configuration. '.
+            "\nTO INSTALL: %s",
+            $binary,
+            get_class($this),
+            $this->getInstallInstructions()));
+      }
+    }
+  }
+
+
+  /**
+   * Get the composed executable command, including the interpreter and binary
+   * but without flags or paths. This can be used to execute `--version`
+   * commands.
+   *
+   * @return string Command to execute the raw linter.
+   * @task exec
+   */
+  protected function getExecutableCommand() {
+    $this->checkBinaryConfiguration();
+
+    $interpreter = null;
+    if ($this->shouldUseInterpreter()) {
+      $interpreter = $this->getInterpreter();
+    }
+
+    $binary = $this->getBinary();
+
+    if ($interpreter) {
+      $bin = csprintf('%s %s', $interpreter, $binary);
+    } else {
+      $bin = csprintf('%s', $binary);
+    }
+
+    return $bin;
+  }
+
+
+  /**
+   * Get the composed flags for the executable, including both mandatory and
+   * configured flags.
+   *
+   * @return string Composed flags.
+   * @task exec
+   */
+  protected function getCommandFlags() {
+    return csprintf(
+      '%C %C',
+      $this->getMandatoryFlags(),
+      coalesce($this->flags, $this->getDefaultFlags()));
+  }
+
+
+  protected function buildFutures(array $paths) {
+    $executable = $this->getExecutableCommand();
+
+    $bin = csprintf('%C %C', $executable, $this->getCommandFlags());
+
+    $futures = array();
+    foreach ($paths as $path) {
+      if ($this->supportsReadDataFromStdin()) {
+        $future = new ExecFuture(
+          '%C %C',
+          $bin,
+          $this->getReadDataFromStdinFilename());
+        $future->write($this->getFileData($path));
+      } else {
+        // TODO: In commit hook mode, we need to do more handling here.
+        $disk_path = $this->getEngine()->getFilePathOnDisk($path);
+        $future = new ExecFuture('%C %s', $bin, $disk_path);
+      }
+
+      $futures[$path] = $future;
+    }
+
+    return $futures;
+  }
+
+  protected function resolveFuture($path, Future $future) {
+    list($err, $stdout, $stderr) = $future->resolve();
+    if ($err && !$this->shouldExpectCommandErrors()) {
+      $future->resolvex();
+    }
+
+    $messages = $this->parseLinterOutput($path, $err, $stdout, $stderr);
+
+    if ($messages === false) {
+      $future->resolvex();
+      return;
+    }
+
+    foreach ($messages as $message) {
+      $this->addLintMessage($message);
+    }
+  }
+
+
+  public function getLinterConfigurationOptions() {
+    $options = array(
+      'bin' => 'optional string | list<string>',
+      'flags' => 'optional string',
+    );
+
+    if ($this->shouldUseInterpreter()) {
+      $options['interpreter'] = 'optional string | list<string>';
+    }
+
+    return $options;
+  }
+
+  public function setLinterConfigurationValue($key, $value) {
+    switch ($key) {
+      case 'interpreter':
+        $working_copy = $this->getEngine()->getWorkingCopy();
+        $root = $working_copy->getProjectRoot();
+
+        foreach ((array)$value as $path) {
+          if (Filesystem::binaryExists($path)) {
+            $this->setInterpreter($path);
+            return;
+          }
+
+          $path = Filesystem::resolvePath($path, $root);
+
+          if (Filesystem::binaryExists($path)) {
+            $this->setInterpreter($path);
+            return;
+          }
+        }
+
+        throw new Exception(
+          pht('None of the configured interpreters can be located.'));
+      case 'bin':
+        $is_script = $this->shouldUseInterpreter();
+
+        $working_copy = $this->getEngine()->getWorkingCopy();
+        $root = $working_copy->getProjectRoot();
+
+        foreach ((array)$value as $path) {
+          if (!$is_script && Filesystem::binaryExists($path)) {
+            $this->setBinary($path);
+            return;
+          }
+
+          $path = Filesystem::resolvePath($path, $root);
+          if ((!$is_script && Filesystem::binaryExists($path)) ||
+              ($is_script && Filesystem::pathExists($path))) {
+            $this->setBinary($path);
+            return;
+          }
+        }
+
+        throw new Exception(
+          pht('None of the configured binaries can be located.'));
+      case 'flags':
+        if (strlen($value)) {
+          $this->setFlags($value);
+        }
+        break;
+    }
+
+    return parent::setLinterConfigurationValue($key, $value);
+  }
+
+}

src/lint/linter/ArcanistLinter.php

@@ -256,4 +256,12 @@ abstract class ArcanistLinter {
     return null;
   }
 
+  public function getLinterConfigurationOptions() {
+    return array();
+  }
+
+  public function setLinterConfigurationValue($key, $value) {
+    throw new Exception("Incomplete implementation: {$key}!");
+  }
+
 }

src/lint/linter/ArcanistPEP8Linter.php

@@ -5,26 +5,20 @@
  *
  * @group linter
  */
-final class ArcanistPEP8Linter extends ArcanistFutureLinter {
+final class ArcanistPEP8Linter extends ArcanistExternalLinter {
 
   public function getLinterName() {
     return 'PEP8';
   }
 
-  public function getLintSeverityMap() {
-    return array();
-  }
-
-  public function getLintNameMap() {
-    return array();
-  }
-
   public function getCacheVersion() {
-    list($stdout) = execx('%C --version', $this->getPEP8Path());
-    return $stdout.$this->getPEP8Options();
+    list($stdout) = execx('%C --version', $this->getExecutableCommand());
+    return $stdout.$this->getCommandFlags();
   }
 
-  public function getPEP8Options() {
+  public function getDefaultFlags() {
+    // TODO: Warn that all of this is deprecated.
+
     $working_copy = $this->getEngine()->getWorkingCopy();
     $options = $working_copy->getConfig('lint.pep8.options');
 
@@ -35,73 +29,44 @@ final class ArcanistPEP8Linter extends ArcanistFutureLinter {
     return $options;
   }
 
-  public function getPEP8Path() {
+  public function shouldUseInterpreter() {
+    return ($this->getDefaultBinary() !== 'pep8');
+  }
+
+  public function getDefaultInterpreter() {
+    return 'python2.6';
+  }
+
+  public function getDefaultBinary() {
+    if (Filesystem::binaryExists('pep8')) {
+      return 'pep8';
+    }
+
     $working_copy = $this->getEngine()->getWorkingCopy();
-    $prefix = $working_copy->getConfig('lint.pep8.prefix');
-    $bin = $working_copy->getConfig('lint.pep8.bin');
+    $old_prefix = $working_copy->getConfig('lint.pep8.prefix');
+    $old_bin = $working_copy->getConfig('lint.pep8.bin');
 
-    if ($bin === null && $prefix === null) {
-      $bin = csprintf('/usr/bin/env python2.6 %s',
-               phutil_get_library_root('arcanist').
-               '/../externals/pep8/pep8.py');
-    } else {
-      if ($bin === null) {
-        $bin = 'pep8';
-      }
-
-      if ($prefix !== null) {
-        if (!Filesystem::pathExists($prefix.'/'.$bin)) {
-          throw new ArcanistUsageException(
-            "Unable to find PEP8 binary in a specified directory. Make sure ".
-            "that 'lint.pep8.prefix' and 'lint.pep8.bin' keys are set ".
-            "correctly. If you'd rather use a copy of PEP8 installed ".
-            "globally, you can just remove these keys from your .arcconfig.");
-        }
-
-        $bin = csprintf("%s/%s", $prefix, $bin);
-
-        return $bin;
-      }
-
-      // Look for globally installed PEP8
-      list($err) = exec_manual('which %s', $bin);
-      if ($err) {
-        throw new ArcanistUsageException(
-          "PEP8 does not appear to be installed on this system. Install it ".
-          "(e.g., with 'easy_install pep8') or configure ".
-          "'lint.pep8.prefix' in your .arcconfig to point to the directory ".
-          "where it resides.");
-      }
+    if ($old_prefix || $old_bin) {
+      // TODO: Deprecation warning.
+      $old_bin = nonempty($old_bin, 'pep8');
+      return $old_prefix.'/'.$old_bin;
     }
 
-    return $bin;
+    $arc_root = dirname(phutil_get_library_root('arcanist'));
+    return $arc_root.'/externals/pep8/pep8.py';
   }
 
-  protected function buildFutures(array $paths) {
-    $severity = ArcanistLintSeverity::SEVERITY_WARNING;
-    if (!$this->getEngine()->isSeverityEnabled($severity)) {
-      return;
-    }
-
-    $pep8_bin = $this->getPEP8Path();
-    $options = $this->getPEP8Options();
-
-    $futures = array();
-
-    foreach ($paths as $path) {
-      $futures[$path] = new ExecFuture(
-        "%C %C %s",
-        $pep8_bin,
-        $options,
-        $this->getEngine()->getFilePathOnDisk($path));
-    }
-
-    return $futures;
+  public function getInstallInstructions() {
+    return pht('Install PEP8 using `easy_install pep8`.');
   }
 
-  protected function resolveFuture($path, Future $future) {
-    list($rc, $stdout) = $future->resolve();
-    $lines = explode("\n", $stdout);
+  public function shouldExpectCommandErrors() {
+    return true;
+  }
+
+  protected function parseLinterOutput($path, $err, $stdout, $stderr) {
+    $lines = phutil_split_lines($stdout, $retain_endings = false);
+
     $messages = array();
     foreach ($lines as $line) {
       $matches = null;
@@ -122,8 +87,15 @@ final class ArcanistPEP8Linter extends ArcanistFutureLinter {
       $message->setName('PEP8 '.$matches[4]);
       $message->setDescription($matches[5]);
       $message->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING);
-      $this->addLintMessage($message);
+
+      $messages[] = $message;
     }
+
+    if ($err && !$messages) {
+      return false;
+    }
+
+    return $messages;
   }
 
 }

src/lint/linter/__tests__/ArcanistCSSLintLinterTestCase.php

@@ -0,0 +1,12 @@
+<?php
+
+final class ArcanistCSSLintLinterTestCase
+  extends ArcanistArcanistLinterTestCase {
+
+  public function testCSSLintLinter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/csslint/',
+      new ArcanistCSSLintLinter());
+  }
+
+}

src/lint/linter/__tests__/ArcanistPEP8LinterTestCase.php

@@ -0,0 +1,12 @@
+<?php
+
+final class ArcanistPEP8LinterTestCase
+  extends ArcanistArcanistLinterTestCase {
+
+  public function testPEP8Linter() {
+    $this->executeTestsInDirectory(
+      dirname(__FILE__).'/pep8/',
+      new ArcanistPEP8Linter());
+  }
+
+}

src/lint/linter/__tests__/csslint/duplicate-properties.lint-test

@@ -0,0 +1,10 @@
+.rule {
+  font-weight: bold;
+  font-weight: bold;
+  font-weight: bold;
+  font-weight: bold;
+}
+~~~~~~~~~~
+warning:3:3
+warning:4:3
+warning:5:3

src/lint/linter/__tests__/csslint/empty-rule.lint-test

@@ -0,0 +1,3 @@
+.rule { }
+~~~~~~~~~~
+warning:1:1

src/lint/linter/__tests__/pep8/imports.lint-test

@@ -0,0 +1,3 @@
+import os, sys
+~~~~~~~~~~
+warning:1:10