changes to how python utilities are located (setting path envionment variable, giving preference to python executable path) #178

DonJayamanne · DonJayamanne · commit cd7f593a16a8 · 2016-06-22T10:56:32.000+10:00
diff --git a/src/client/common/utils.ts b/src/client/common/utils.ts
@@ -6,8 +6,11 @@ import * as child_process from "child_process";
 import * as settings from "./configSettings";
 
 let pythonInterpretterDirectory: string = null;
+let previouslyIdentifiedPythonPath: string = null;
+
 export function getPythonInterpreterDirectory(): Promise<string> {
-    if (pythonInterpretterDirectory) {
+    // If we already have it and the python path hasn't changed, yay
+    if (pythonInterpretterDirectory && previouslyIdentifiedPythonPath === settings.PythonSettings.getInstance().pythonPath) {
         return Promise.resolve(pythonInterpretterDirectory);
     }
 
@@ -22,74 +25,46 @@ export function getPythonInterpreterDirectory(): Promise<string> {
 
         // If we can execute the python, then get the path from the fullyqualitified name
         child_process.execFile(pythonFileName, ["-c", "print(1234)"], (error, stdout, stderr) => {
+            // Yes this is a valid python path
             if (stdout.startsWith("1234")) {
                 return resolve(path.dirname(pythonFileName));
             }
+            // No idea, didn't work, hence don't reject, but return empty path
             resolve("");
         });
     }).then(value => {
+        // Cache and return
+        previouslyIdentifiedPythonPath = settings.PythonSettings.getInstance().pythonPath;
         return pythonInterpretterDirectory = value;
     }).catch(() => {
         // Don't care what the error is, all we know is that this doesn't work
         return pythonInterpretterDirectory = "";
     });
 }
 
-const IN_VALID_FILE_PATHS: Map<string, boolean> = new Map<string, boolean>();
-
 export function execPythonFile(file: string, args: string[], cwd: string, includeErrorAsResponse: boolean = false): Promise<string> {
-    // Whether to try executing the command without prefixing it with the python path
-    let tryUsingCommandArg = false;
-    let fullyQualifiedFilePromise = getPythonInterpreterDirectory().then(pyPath => {
-        let pythonIntepreterPath = pyPath;
-        let fullyQualifiedFile = file;
+    return getPythonInterpreterDirectory().then(pyPath => {
+        let options: any = { cwd: cwd };
 
-        // Qualify the command with the python path
-        if (pythonIntepreterPath.length > 0 && !file.startsWith(pyPath)) {
-            fullyQualifiedFile = pythonIntepreterPath + (pythonIntepreterPath.endsWith(path.sep) ? "" : path.sep) + file;
-
-            // Check if we know whether this trow ENONE errors
-            if (IN_VALID_FILE_PATHS.has(fullyQualifiedFile)) {
-                fullyQualifiedFile = file;
-            }
-            else {
-                tryUsingCommandArg = true;
-            }
+        // If we have a fully qualitified path to the python executable directory, then we can use that
+        // as the base path for all other executables
+        if (pyPath.length > 0) {
+            options.env = mergeEnvVariables({ PATH: pyPath + path.delimiter + process.env.PATH });
         }
 
-        return execFileInternal(fullyQualifiedFile, args, cwd, includeErrorAsResponse, true);
+        return execFileInternal(file, args, options, includeErrorAsResponse);
     });
-
-    if (tryUsingCommandArg) {
-        return fullyQualifiedFilePromise.catch(error => {
-            if (error && (<any>error).code === "ENOENT") {
-                // Re-execute the file, without the python path prefix
-                // Only if we know that the previous one failed with ENOENT
-                return execFileInternal(file, args, cwd, includeErrorAsResponse, true);
-            }
-            Promise.reject(error);
-        });
-    }
-    else {
-        return fullyQualifiedFilePromise;
-    }
 }
 
-function execFileInternal(file: string, args: string[], cwd: string, includeErrorAsResponse: boolean, rejectIfENOENTErrors: boolean = false): Promise<string> {
+function execFileInternal(file: string, args: string[], options: child_process.ExecFileOptions, includeErrorAsResponse: boolean): Promise<string> {
     return new Promise<string>((resolve, reject) => {
-        child_process.execFile(file, args, { cwd: cwd }, (error, stdout, stderr) => {
+        child_process.execFile(file, args, options, (error, stdout, stderr) => {
             // pylint:
             //      In the case of pylint we have some messages (such as config file not found and using default etc...) being returned in stderr
             //      These error messages are useless when using pylint   
             if (includeErrorAsResponse && (stdout.length > 0 || stderr.length > 0)) {
                 return resolve(stdout + "\n" + stderr);
             }
-            if (error && (<any>error).code === "ENOENT" && rejectIfENOENTErrors) {
-                if (!IN_VALID_FILE_PATHS.has(file)) {
-                    IN_VALID_FILE_PATHS.set(file, true);
-                }
-                return reject(error);
-            }
 
             let hasErrors = (error && error.message.length > 0) || (stderr && stderr.length > 0);
             if (hasErrors && (typeof stdout !== "string" || stdout.length === 0)) {
@@ -101,3 +76,13 @@ function execFileInternal(file: string, args: string[], cwd: string, includeErro
         });
     });
 }
+
+export function mergeEnvVariables(newVariables: { [key: string]: string }): any {
+    for (let setting in process.env) {
+        if (!newVariables[setting]) {
+            newVariables[setting] = process.env[setting];
+        }
+    }
+
+    return newVariables;
+}
