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;
+ }
+}