[release] src/goInstallTools.ts: add GOROOT/bin to PATH when it wasn't found from PATH The fix for golang/vscode-go#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 golang/vscode-go#679 Fixes golang/vscode-go#713 Change-Id: Ie00612fcef2cf4c2a187a263da04b342182c030b Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258316 Trust: Hyang-Ah Hana Kim <hyangah@gmail.com> Run-TryBot: Hyang-Ah Hana Kim <hyangah@gmail.com> TryBot-Result: kokoro <noreply+kokoro@google.com> Reviewed-by: Suzy Mueller <suzmue@golang.org> (cherry picked from commit 9bf9d642c76369144bc85323622240a121868454) Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/258519
diff --git a/src/goInstallTools.ts b/src/goInstallTools.ts index 9694907..759cd3b 100644 --- a/src/goInstallTools.ts +++ b/src/goInstallTools.ts
@@ -30,6 +30,7 @@ import { getFromWorkspaceState } from './stateUtils'; import { getBinPath, + getBinPathWithExplanation, getGoConfig, getGoVersion, getTempFilePath, @@ -37,7 +38,7 @@ 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 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 @@ // 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 @@ }); } -// 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() {
diff --git a/src/util.ts b/src/util.ts index fa94f7b..ee361b4 100644 --- a/src/util.ts +++ b/src/util.ts
@@ -20,7 +20,7 @@ import { envPath, fixDriveCasingInWindows, - getBinPathWithPreferredGopathGoroot, + getBinPathWithPreferredGopathGorootWithExplanation, getCurrentGoRoot, getInferredGopath, resolveHomeDir, @@ -477,7 +477,15 @@ } } +// 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 @@ selectedGoPath = getFromWorkspaceState('selectedGo')?.binpath; } - return getBinPathWithPreferredGopathGoroot( + return getBinPathWithPreferredGopathGorootWithExplanation( tool, tool === 'go' ? [] : [getToolsGopath(), getCurrentGoPath()], tool === 'go' && cfg.get('goroot') ? resolvePath(cfg.get('goroot')) : undefined,
diff --git a/src/utils/pathUtils.ts b/src/utils/pathUtils.ts index ab3be8f..3f1f59a 100644 --- a/src/utils/pathUtils.ts +++ b/src/utils/pathUtils.ts
@@ -37,23 +37,37 @@ 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 @@ const pathFrompreferredGoPath = getBinPathFromEnvVar(binname, preferred, true); if (pathFrompreferredGoPath) { binPathCache[toolName] = pathFrompreferredGoPath; - return pathFrompreferredGoPath; + return {binPath: pathFrompreferredGoPath, why: 'gopath'}; } } } @@ -71,14 +85,14 @@ 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 @@ -86,13 +100,13 @@ 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}; } /**