[release] src/goMain: warn users if go.goroot is set

Warn users if this is an obviously invalid value (binary). Show information popup if go.goroot is configured. And, fixes a bug in v0.25.0 - missing await when checking whether the go.goroot value is a valid directory. Fixes #1501 Change-Id: I3487f4b089c9ba4fe34f36e5e033ae19d63537e2 Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/320429 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 9fa871b) Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/320549
golang · May 21, 2021 · 250bfb0 · 250bfb0
1 parent 5a495aa
commit 250bfb0
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 12 deletions.
diff --git a/src/goMain.ts b/src/goMain.ts
@@ -151,11 +151,11 @@ export async function activate(ctx: vscode.ExtensionContext) {
 
 	const configGOROOT = getGoConfig()['goroot'];
 	if (configGOROOT) {
-		const goroot = resolvePath(configGOROOT);
-		if (dirExists(goroot)) {
-			logVerbose(`setting GOROOT = ${goroot} because "go.goroot": "${configGOROOT}"`);
-			process.env['GOROOT'] = goroot;
-		}
+		// We don't support unsetting go.goroot because we don't know whether
+		// !configGOROOT case indicates the user wants to unset process.env['GOROOT']
+		// or the user wants the extension to use the current process.env['GOROOT'] value.
+		// TODO(hyangah): consider utilizing an empty value to indicate unset?
+		await setGOROOTEnvVar(configGOROOT);
 	}
 
 	// Present a warning about the deprecation of the go.documentLink setting.
@@ -440,7 +440,7 @@ If you would like additional configuration for diagnostics from gopls, please se
 	);
 
 	ctx.subscriptions.push(
-		vscode.workspace.onDidChangeConfiguration((e: vscode.ConfigurationChangeEvent) => {
+		vscode.workspace.onDidChangeConfiguration(async (e: vscode.ConfigurationChangeEvent) => {
 			if (!e.affectsConfiguration('go')) {
 				return;
 			}
@@ -449,12 +449,7 @@ If you would like additional configuration for diagnostics from gopls, please se
 			if (e.affectsConfiguration('go.goroot')) {
 				const configGOROOT = updatedGoConfig['goroot'];
 				if (configGOROOT) {
-					const goroot = resolvePath(configGOROOT);
-					const oldGoroot = process.env['GOROOT'];
-					if (oldGoroot !== goroot && dirExists(goroot)) {
-						logVerbose(`setting GOROOT = ${goroot} because "go.goroot": "${configGOROOT}"`);
-						process.env['GOROOT'] = goroot;
-					}
+					await setGOROOTEnvVar(configGOROOT);
 				}
 			}
 			if (
@@ -997,3 +992,42 @@ function lintDiagnosticCollectionName(lintToolName: string) {
 	}
 	return `go-${lintToolName}`;
 }
+
+// set GOROOT env var. If necessary, shows a warning.
+export async function setGOROOTEnvVar(configGOROOT: string) {
+	if (!configGOROOT) {
+		return;
+	}
+	const goroot = configGOROOT ? resolvePath(configGOROOT) : undefined;
+
+	const currentGOROOT = process.env['GOROOT'];
+	if (goroot === currentGOROOT) {
+		return;
+	}
+	if (!(await dirExists(goroot))) {
+		vscode.window.showWarningMessage(`go.goroot setting is ignored. ${goroot} is not a valid GOROOT directory.`);
+		return;
+	}
+	const neverAgain = { title: "Don't Show Again" };
+	const ignoreGOROOTSettingWarningKey = 'ignoreGOROOTSettingWarning';
+	const ignoreGOROOTSettingWarning = getFromGlobalState(ignoreGOROOTSettingWarningKey);
+	if (!ignoreGOROOTSettingWarning) {
+		vscode.window
+			.showInformationMessage(
+				`"go.goroot" setting (${goroot}) will be applied and set the GOROOT environment variable.`,
+				neverAgain
+			)
+			.then((result) => {
+				if (result === neverAgain) {
+					updateGlobalState(ignoreGOROOTSettingWarningKey, true);
+				}
+			});
+	}
+
+	logVerbose(`setting GOROOT = ${goroot} (old value: ${currentGOROOT}) because "go.goroot": "${configGOROOT}"`);
+	if (goroot) {
+		process.env['GOROOT'] = goroot;
+	} else {
+		delete process.env.GOROOT;
+	}
+}
diff --git a/test/integration/statusbar.test.ts b/test/integration/statusbar.test.ts
@@ -26,6 +26,7 @@ import { getWorkspaceState, setWorkspaceState } from '../../src/stateUtils';
 import { MockMemento } from '../mocks/MockMemento';
 
 import ourutil = require('../../src/util');
+import { setGOROOTEnvVar } from '../../src/goMain';
 
 describe('#initGoStatusBar()', function () {
 	this.beforeAll(async () => {
@@ -52,6 +53,38 @@ describe('#initGoStatusBar()', function () {
 	});
 });
 
+describe('#setGOROOTEnvVar', function () {
+	let origGOROOT = process.env['GOROOT'];
+
+	this.beforeEach(() => {
+		origGOROOT = process.env['GOROOT'];
+	});
+
+	this.afterEach(() => {
+		if (origGOROOT) {
+			process.env['GOROOT'] = origGOROOT;
+		} else {
+			delete process.env.GOROOT;
+		}
+	});
+
+	it('empty goroot does not change GOROOT', async () => {
+		await setGOROOTEnvVar('');
+		assert.strictEqual(process.env['GOROOT'], origGOROOT);
+	});
+
+	it('non-directory value is rejected', async () => {
+		await setGOROOTEnvVar(ourutil.getBinPath('go'));
+		assert.strictEqual(process.env['GOROOT'], origGOROOT);
+	});
+
+	it('directory value is accepted', async () => {
+		const goroot = path.join(path.dirname(ourutil.getBinPath('go')), '..');
+		await setGOROOTEnvVar(goroot);
+		assert.strictEqual(process.env['GOROOT'], goroot);
+	});
+});
+
 describe('#setSelectedGo()', function () {
 	this.timeout(40000);
 	let sandbox: sinon.SinonSandbox | undefined;