Fix python configuration processing.

Summary:
Previously buck would try to use any executable file or
directory in its working directory as a python interpreter. It would
also silently fall back to finding a python on the PATH if a python
path was specified in the config, but invalid. This change fixes both
of those problems and adds tests.

Test Plan:
0) buck test --all
1) mkdir python
2) touch BUCK
3) check that "buck build :lolwut" returns the error "BUILD FAILED: No rule
found when resolving target //:lolwut in build file //BUCK"
diff --git a/src/com/facebook/buck/cli/BuckConfig.java b/src/com/facebook/buck/cli/BuckConfig.java
index e3b9869..5666d6c 100644
--- a/src/com/facebook/buck/cli/BuckConfig.java
+++ b/src/com/facebook/buck/cli/BuckConfig.java
@@ -638,7 +638,8 @@
     return Objects.hashCode(sectionsToEntries);
   }
 
-  private String[] getEnv(String propertyName, String separator) {
+  @VisibleForTesting
+  String[] getEnv(String propertyName, String separator) {
     String value = System.getenv(propertyName);
     if (value == null) {
       value = "";
@@ -647,36 +648,48 @@
   }
 
   /**
-   * Returns the path to python interpreter. Firstly, it queries "python" under "tools" section
-   * defined in .buckconfig. If not found or invalid, it will try to find python under PATH.
+   * @return true if file is executable and not a directory.
+   */
+  private boolean isExecutableFile(File file) {
+    return file.canExecute() && !file.isDirectory();
+  }
+
+  /**
+   * Returns the path to python interpreter. If python is specified in the tools section
+   * that is used and an error reported if invalid. If no python is specified, the PATH
+   * is searched and if no python is found, jython is used as a fallback.
    * @return The found python interpreter.
    */
   public String getPythonInterpreter() {
-    String interpreter = getValue("tools", "python").or("python");
-    // Try finding interpreter with file name directly.
-    File executable = new File(interpreter);
-    if (executable.canExecute()) {
-      return executable.getAbsolutePath();
-    }
-
-    // Try to prepend path
-    // For windows, executables have certain extension names, i.e. .exe, .bat, .cmd, etc, which are
-    // defined in %PATHEXT%
-    String[] paths = getEnv("PATH", File.pathSeparator);
-    String[] pathExts = getEnv("PATHEXT", File.pathSeparator);
-    for (String path : paths) {
-      for (String pathExt : pathExts) {
-        executable = new File(path, interpreter + pathExt);
-        if (executable.canExecute()) {
-          return executable.getAbsolutePath();
+    Optional<String> configPath = getValue("tools", "python");
+    if (configPath.isPresent()) {
+      // Python path in config. Use it or report error if invalid.
+      File python = new File(configPath.get());
+      if (isExecutableFile(python)) {
+        return python.getAbsolutePath();
+      }
+      throw new HumanReadableException("Not a python executable: " + configPath.get());
+    } else {
+      // For each path in PATH, test "python" with each PATHEXT suffix to allow for file extensions.
+      for (String path : getEnv("PATH", File.pathSeparator)) {
+        for (String pathExt : getEnv("PATHEXT", File.pathSeparator)) {
+          File python = new File(path, "python" + pathExt);
+          if (isExecutableFile(python)) {
+            return python.getAbsolutePath();
+          }
         }
       }
+      // Fall back to Jython if no python found.
+      File jython = new File(getProperty("buck.path_to_python_interp", "bin/jython"));
+      if (isExecutableFile(jython)) {
+        return jython.getAbsolutePath();
+      }
+      throw new HumanReadableException("No python or jython found.");
     }
+  }
 
-    // Use Jython as a fallback
-    interpreter = System.getProperty("buck.path_to_python_interp", "bin/jython");
-    executable = new File(interpreter);
-    assert(executable.canExecute());
-    return interpreter;
+  @VisibleForTesting
+  String getProperty(String key, String def) {
+    return System.getProperty(key, def);
   }
 }
diff --git a/test/com/facebook/buck/cli/BUCK b/test/com/facebook/buck/cli/BUCK
index 9995d00..cc2c361 100644
--- a/test/com/facebook/buck/cli/BUCK
+++ b/test/com/facebook/buck/cli/BUCK
@@ -1,5 +1,6 @@
 FAKE_BUCK_CONFIG_SRCS = [
   'FakeBuckConfig.java',
+  'FakeBuckEnvironment.java',
 ]
 
 STANDARD_TEST_SRCS = [
diff --git a/test/com/facebook/buck/cli/BuckConfigTest.java b/test/com/facebook/buck/cli/BuckConfigTest.java
index 92b43f2..0385fbb 100644
--- a/test/com/facebook/buck/cli/BuckConfigTest.java
+++ b/test/com/facebook/buck/cli/BuckConfigTest.java
@@ -461,6 +461,93 @@
     assertFalse(linuxConfig.createAnsi(Optional.of("never")).isAnsiTerminal());
   }
 
+  @Test
+  public void whenToolsPythonIsExecutableFileThenItIsUsed() throws IOException {
+    File configPythonFile = temporaryFolder.newFile("python");
+    assertTrue("Should be able to set file executable", configPythonFile.setExecutable(true));
+    FakeBuckConfig config = new FakeBuckConfig(ImmutableMap.<String, Map<String, String>>builder()
+        .put("tools", ImmutableMap.of("python", configPythonFile.getAbsolutePath()))
+        .build());
+    assertEquals("Should return path to temp file.",
+        configPythonFile.getAbsolutePath(), config.getPythonInterpreter());
+  }
+
+  @Test(expected = HumanReadableException.class)
+  public void whenToolsPythonDoesNotExistThenItIsNotUsed() throws IOException {
+    String invalidPath = temporaryFolder.getRoot().getAbsolutePath() + "DoesNotExist";
+    FakeBuckConfig config = new FakeBuckConfig(ImmutableMap.<String, Map<String, String>>builder()
+        .put("tools", ImmutableMap.of("python", invalidPath))
+        .build());
+    config.getPythonInterpreter();
+    fail("Should throw exception as python config is invalid.");
+  }
+
+  @Test(expected = HumanReadableException.class)
+  public void whenToolsPythonIsNonExecutableFileThenItIsNotUsed() throws IOException {
+    File configPythonFile = temporaryFolder.newFile("python");
+    assertTrue("Should be able to set file non-executable", configPythonFile.setExecutable(false));
+    FakeBuckConfig config = new FakeBuckConfig(ImmutableMap.<String, Map<String, String>>builder()
+        .put("tools", ImmutableMap.of("python", configPythonFile.getAbsolutePath()))
+        .build());
+    config.getPythonInterpreter();
+    fail("Should throw exception as python config is invalid.");
+  }
+
+  @Test(expected = HumanReadableException.class)
+  public void whenToolsPythonIsExecutableDirectoryThenItIsNotUsed() throws IOException {
+    File configPythonFile = temporaryFolder.newFolder("python");
+    assertTrue("Should be able to set file executable", configPythonFile.setExecutable(true));
+    FakeBuckConfig config = new FakeBuckConfig(ImmutableMap.<String, Map<String, String>>builder()
+        .put("tools", ImmutableMap.of("python", configPythonFile.getAbsolutePath()))
+        .build());
+    config.getPythonInterpreter();
+    fail("Should throw exception as python config is invalid.");
+  }
+
+  @Test
+  public void whenPythonOnPathIsExecutableFileThenItIsUsed() throws IOException {
+    File python = temporaryFolder.newFile("python");
+    assertTrue("Should be able to set file executable", python.setExecutable(true));
+    FakeBuckConfig config = new FakeBuckEnvironment(ImmutableMap.<String, Map<String, String>>of(),
+        ImmutableMap.<String, String>builder()
+            .put("PATH", temporaryFolder.getRoot().getAbsolutePath())
+            .put("PATHEXT", "")
+            .build(),
+        ImmutableMap.<String, String>of()
+    );
+    config.getPythonInterpreter();
+  }
+
+  @Test
+  public void whenPythonPlusExtensionOnPathIsExecutableFileThenItIsUsed() throws IOException {
+    File python = temporaryFolder.newFile("python.exe");
+    assertTrue("Should be able to set file executable", python.setExecutable(true));
+    FakeBuckConfig config = new FakeBuckEnvironment(ImmutableMap.<String, Map<String, String>>of(),
+        ImmutableMap.<String, String>builder()
+            .put("PATH", temporaryFolder.getRoot().getAbsolutePath())
+            .put("PATHEXT", ".exe")
+            .build(),
+        ImmutableMap.<String, String>of()
+    );
+    config.getPythonInterpreter();
+  }
+
+  @Test
+  public void whenPythonOnPathNotFoundThenJythonUsed() throws IOException {
+    File jython = temporaryFolder.newFile("jython");
+    assertTrue("Should be able to set file executable", jython.setExecutable(true));
+    FakeBuckConfig config = new FakeBuckEnvironment(ImmutableMap.<String, Map<String, String>>of(),
+        ImmutableMap.<String, String>builder()
+            .put("PATH", temporaryFolder.getRoot().getAbsolutePath())
+            .put("PATHEXT", "")
+            .build(),
+        ImmutableMap.of("buck.path_to_python_interp", jython.getAbsolutePath())
+    );
+    assertEquals("Should fallback to Jython.",
+        jython.getAbsolutePath(),
+        config.getPythonInterpreter());
+  }
+
   private BuckConfig createWithDefaultFilesystem(Reader reader, @Nullable BuildTargetParser parser)
       throws IOException {
     ProjectFilesystem projectFilesystem = new ProjectFilesystem(new File("."));
diff --git a/test/com/facebook/buck/cli/FakeBuckEnvironment.java b/test/com/facebook/buck/cli/FakeBuckEnvironment.java
new file mode 100644
index 0000000..8a83fac
--- /dev/null
+++ b/test/com/facebook/buck/cli/FakeBuckEnvironment.java
@@ -0,0 +1,47 @@
+/*
+ * Copyright 2013-present Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may
+ * not use this file except in compliance with the License. You may obtain
+ * a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+
+package com.facebook.buck.cli;
+
+import java.util.Map;
+
+public class FakeBuckEnvironment extends FakeBuckConfig {
+
+  private final Map<String, String> environment;
+  private final Map<String, String> properties;
+
+  public FakeBuckEnvironment(
+      Map<String, Map<String, String>> sections,
+      Map<String, String> environment,
+      Map<String, String> properties) {
+    super(sections);
+    this.environment = environment;
+    this.properties = properties;
+  }
+
+  @Override
+  String[] getEnv(String propertyName, String separator) {
+    return environment.get(propertyName).split(separator);
+  }
+
+  @Override
+  String getProperty(String name, String def) {
+    if (properties.containsKey(name)) {
+      return properties.get(name);
+    }
+    return def;
+  }
+}