[release] src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn't found from PATH

The fix for #679 changed to prepend GOROOT/bin to PATH or Path
only if users explicitly configured to use go different from what's found from PATH.
I forgot the case where go was not found from PATH and the extension picked up
a common default go installation directory. (C:\Go\bin or /usr/local/go/bin).

This change rewrote the fix - rewrote getBinPath to return why it chose the
path as well. Then, we mutate the PATH env var if getBinPath picked
go with a reason other than it's in PATH (why === 'path').

Since getBinPath and getBinPathWithPreferredGopathGoroot are
used in many places, this CL introduces getBinPathWithExplanation and
getBinPath wraps it.

Updates #679
Fixes #713

Change-Id: Ie00612fcef2cf4c2a187a263da04b342182c030b
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258316
Trust: Hyang-Ah Hana Kim <[email protected]>
Run-TryBot: Hyang-Ah Hana Kim <[email protected]>
TryBot-Result: kokoro <[email protected]>
Reviewed-by: Suzy Mueller <[email protected]>
(cherry picked from commit 9bf9d64)
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258519

Author: hyangah

src/goInstallTools.ts

@@ -30,14 +30,15 @@ import {
 import { getFromWorkspaceState } from './stateUtils';
 import {
 	getBinPath,
+	getBinPathWithExplanation,
 	getGoConfig,
 	getGoVersion,
 	getTempFilePath,
 	getWorkspaceFolderPath,
 	GoVersion,
 	rmdirRecursive,
 } from './util';
-import { correctBinname, envPath, getCurrentGoRoot, getToolFromToolPath, setCurrentGoRoot } from './utils/pathUtils';
+import { envPath, getCurrentGoRoot, getToolFromToolPath, setCurrentGoRoot } from './utils/pathUtils';
 
 // declinedUpdates tracks the tools that the user has declined to update.
 const declinedUpdates: Tool[] = [];
@@ -343,7 +344,8 @@ export async function promptForUpdatingTool(toolName: string, newVersion?: SemVe
 }
 
 export function updateGoVarsFromConfig(): Promise<void> {
-	const goRuntimePath = getBinPath('go', false);
+	const {binPath, why} = getBinPathWithExplanation('go', false);
+	const goRuntimePath = binPath;
 
 	if (!goRuntimePath || !path.isAbsolute(goRuntimePath)) {
 		// getBinPath returns the absolute path to the tool if it exists.
@@ -384,8 +386,8 @@ export function updateGoVarsFromConfig(): Promise<void> {
 				// cgo, gopls, and other underlying tools will inherit the environment and attempt
 				// to locate 'go' from the PATH env var.
 				// Update the PATH only if users configured to use a different
-				// version of go than the system default.
-				if (!!goPickedByExtension()) {
+				// version of go than the system default found from PATH (or Path).
+				if (why !== 'path') {
 					addGoRuntimeBaseToPATH(path.join(getCurrentGoRoot(), 'bin'));
 				}
 				initGoStatusBar();
@@ -396,41 +398,6 @@ export function updateGoVarsFromConfig(): Promise<void> {
 	});
 }
 
-// The go command is picked up by searching directories in PATH by default.
-// But users can override it and force the extension to pick a different
-// one by configuring
-//
-//   1) with the go.environment.choose command, which stores the selection
-//      in the workspace memento with the key 'selectedGo',
-//   2) with 'go.alternateTools': { 'go': ... } setting, or
-//   3) with 'go.goroot' setting
-//
-// goPickedByExtension returns the chosen path if the default path should
-// be overridden by above methods.
-// TODO: This logic is duplicated in getBinPath. Centralize this logic.
-function goPickedByExtension(): string | undefined {
-	// getFromWorkspaceState('selectedGo')
-	const selectedGoPath: string = getFromWorkspaceState('selectedGo')?.binpath;
-	if (selectedGoPath) {
-		return selectedGoPath;
-	}
-
-	const cfg = getGoConfig();
-
-	// 'go.alternateTools.go'
-	const alternateTools: { [key: string]: string } = cfg.get('alternateTools');
-	const alternateToolPath: string = alternateTools['go'];
-	if (alternateToolPath) {
-		return alternateToolPath;
-	}
-	// 'go.goroot'
-	const goRoot: string = cfg.get('goroot');
-	if (goRoot) {
-		return path.join(goRoot, 'bin', correctBinname('go'));
-	}
-	return undefined;
-}
-
 let alreadyOfferedToInstallTools = false;
 
 export async function offerToInstallTools() {

src/util.ts

@@ -20,7 +20,7 @@ import { getFromWorkspaceState } from './stateUtils';
 import {
 	envPath,
 	fixDriveCasingInWindows,
-	getBinPathWithPreferredGopathGoroot,
+	getBinPathWithPreferredGopathGorootWithExplanation,
 	getCurrentGoRoot,
 	getInferredGopath,
 	resolveHomeDir,
@@ -477,7 +477,15 @@ function resolveToolsGopath(): string {
 	}
 }
 
+// getBinPath returns the path to the tool.
 export function getBinPath(tool: string, useCache = true): string {
+	const r = getBinPathWithExplanation(tool, useCache);
+	return r.binPath;
+}
+
+// getBinPathWithExplanation returns the path to the tool, and the explanation on why
+// the path was chosen. See getBinPathWithPreferredGopathGorootWithExplanation for details.
+export function getBinPathWithExplanation(tool: string, useCache = true): {binPath: string, why?: string} {
 	const cfg = getGoConfig();
 	const alternateTools: { [key: string]: string } = cfg.get('alternateTools');
 	const alternateToolPath: string = alternateTools[tool];
@@ -487,7 +495,7 @@ export function getBinPath(tool: string, useCache = true): string {
 		selectedGoPath = getFromWorkspaceState('selectedGo')?.binpath;
 	}
 
-	return getBinPathWithPreferredGopathGoroot(
+	return getBinPathWithPreferredGopathGorootWithExplanation(
 		tool,
 		tool === 'go' ? [] : [getToolsGopath(), getCurrentGoPath()],
 		tool === 'go' && cfg.get('goroot') ? resolvePath(cfg.get('goroot')) : undefined,

src/utils/pathUtils.ts

@@ -37,23 +37,37 @@ export function getBinPathWithPreferredGopathGoroot(
 	preferredGopaths: string[],
 	preferredGoroot?: string,
 	alternateTool?: string,
-	useCache = true,
+	useCache = true
 ): string {
+	const r = getBinPathWithPreferredGopathGorootWithExplanation(
+		toolName, preferredGopaths, preferredGoroot, alternateTool, useCache);
+	return r.binPath;
+}
+
+// Is same as getBinPAthWithPreferredGopathGoroot, but returns why the
+// returned path was chosen.
+export function getBinPathWithPreferredGopathGorootWithExplanation(
+	toolName: string,
+	preferredGopaths: string[],
+	preferredGoroot?: string,
+	alternateTool?: string,
+	useCache = true,
+): {binPath: string, why?: string} {
 	if (alternateTool && path.isAbsolute(alternateTool) && executableFileExists(alternateTool)) {
 		binPathCache[toolName] = alternateTool;
-		return alternateTool;
+		return {binPath: alternateTool, why: 'alternateTool'};
 	}
 
 	// FIXIT: this cache needs to be invalidated when go.goroot or go.alternateTool is changed.
 	if (useCache && binPathCache[toolName]) {
-		return binPathCache[toolName];
+		return {binPath: binPathCache[toolName], why: 'cached'};
 	}
 
 	const binname = alternateTool && !path.isAbsolute(alternateTool) ? alternateTool : toolName;
 	const pathFromGoBin = getBinPathFromEnvVar(binname, process.env['GOBIN'], false);
 	if (pathFromGoBin) {
 		binPathCache[toolName] = pathFromGoBin;
-		return pathFromGoBin;
+		return {binPath: pathFromGoBin, why: 'gobin'};
 	}
 
 	for (const preferred of preferredGopaths) {
@@ -62,7 +76,7 @@ export function getBinPathWithPreferredGopathGoroot(
 			const pathFrompreferredGoPath = getBinPathFromEnvVar(binname, preferred, true);
 			if (pathFrompreferredGoPath) {
 				binPathCache[toolName] = pathFrompreferredGoPath;
-				return pathFrompreferredGoPath;
+				return {binPath: pathFrompreferredGoPath, why: 'gopath'};
 			}
 		}
 	}
@@ -71,28 +85,28 @@ export function getBinPathWithPreferredGopathGoroot(
 	const pathFromGoRoot = getBinPathFromEnvVar(binname, preferredGoroot || getCurrentGoRoot(), true);
 	if (pathFromGoRoot) {
 		binPathCache[toolName] = pathFromGoRoot;
-		return pathFromGoRoot;
+		return {binPath: pathFromGoRoot, why: 'goroot'};
 	}
 
 	// Finally search PATH parts
 	const pathFromPath = getBinPathFromEnvVar(binname, envPath, false);
 	if (pathFromPath) {
 		binPathCache[toolName] = pathFromPath;
-		return pathFromPath;
+		return {binPath: pathFromPath, why: 'path'};
 	}
 
 	// Check default path for go
 	if (toolName === 'go') {
 		const defaultPathForGo = process.platform === 'win32' ? 'C:\\Go\\bin\\go.exe' : '/usr/local/go/bin/go';
 		if (executableFileExists(defaultPathForGo)) {
 			binPathCache[toolName] = defaultPathForGo;
-			return defaultPathForGo;
+			return {binPath: defaultPathForGo, why: 'default'};
 		}
-		return;
+		return {binPath: ''};
 	}
 
 	// Else return the binary name directly (this will likely always fail downstream)
-	return toolName;
+	return {binPath: toolName};
 }
 
 /**