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