Skip to content

Commit

Permalink
extension/src/goInstallTools: strengthing minimum go version requirement
Browse files Browse the repository at this point in the history 
This change addresses problems in tools installation surfaced
by the recent changes in go and gopls.

* Automated toolchain switch for Go forwards compatibility

The automated Go toolchain switch change added in go1.21
changed the meaning of 'go version' output different. Previously,
it was meant exactly the version of the toolchain on the system.
After go1.21, this is a go version for chosen for the workspace,
computed based on the local toolchain version and the required
go version dictated by go.mod or go.work.

The extension has a facility to detect the go version used to
compile tools, and ask users to rebuild them if they were built
with older go standard libraries. When a local toolchain is
older (for example, `go1.21.0`) than the workspace's required go
version (`go1.22.0`), the 'go version' reports `go1.22.0`.
The extension will detect tools that need recompiling to support
`go1.22.0` project. That works as expected.

However, when recompiling those tools with `go install`,
the workspace's go version requirement doesn't come into play
at all, but only the local toolchain's go version and the target
tool's minimum required go version will be used in the toolchain
switch decision making. As of today, none of the tools set their
minimum required go version to go1.22.x, so if the local toolchain
is still go1.21.0, `go install` will use go1.21.0 for build.

We need to explicitly specify the minimum required version using
the `GOTOOLCHAIN` environment variable. In this example, go
install with `GOTOOLCHAIN=go1.22.0+auto` will address the issue.

In this CL,
  - extend `getGoVersion` to allow the local toolchain version
    by specifying the optional GOTOOLCHAIN parameter.
  - change `getGoForInstall` to produce the correct GoVersion
    object that carries the true version of the local toolchain.
    (computed with `getGoVersion(..., 'local')`).
  - change `installTools` to append the `GOTOOLCHAIN` env var
    if the go version to be used for install is older than
    the project's minimum required go version.

* More frequent crashes/malfunctions upon version mismatch

Gopls is supposed to prompt when it detects it needs to be
recompiled to process the modules that require newer go versions.
The extension has relied this as the second, and more reliable
defense mechanism. Unfortunately, bugs in the recent gopls
stopped gopls from reliably detecting this case or, even made
the gopls crash before showing the notification.

Similar crashes can occur in other tools (#3168)
when the version is mismatched. Previously, `installTools` warned
users only if the go version for install was very old (e.g. go1.15).

In this CL,
  - tighten `installTools`'s installation tool version check
    further. So, if the project requires a newer version of go
    and the go configured for tools installation (maybe due to
    outdated 'go.toolsManagement.go' setting) is older and
    is a version that cannot handle automated version switch
    (<go1.21), it prompts users.

* Other changes

Testing involving two versions of go is complex. Changed the
runTest helper for 'Installation Tests' test suite to accept
extra parameters and stubs, so we can make the installation
function under test believe the local go chosen for installation
is an older version.

This CL also adjusted the logging of gopls restart activities.
When gopls installed (automatically, or from the activation),
the extension may attempt to restart the gopls. That may help
investigate spurious gopls restarts obeserved during
https://github.com/golang/vscode-go/issues/3307.

This CL stops clearing of the Go outputChannel before tool
installation or goplay run. This channel is now a log output
channel, so it's better not to clear.

Fixes #3168

Change-Id: Id9a4c0fe98c85efb17eb3351cfba5665a83b094d
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/577095
Reviewed-by: Peter Weinberger <pjw@google.com>
Commit-Queue: Hyang-Ah Hana Kim <hyangah@gmail.com>
Reviewed-by: Suzy Mueller <suzmue@golang.org>
kokoro-CI: kokoro <noreply+kokoro@google.com>
  • Loading branch information
@hyangah
hyangah committed Apr 29, 2024
1 parent ea9ff87 commit ff0beea
Show file tree
Hide file tree
Showing 5 changed files with 123 additions and 33 deletions.
15 changes: 11 additions & 4 deletions extension/src/commands/startLanguageServer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ export const startLanguageServer: CommandFactory = (ctx, goCtx) => {
return async (reason: RestartReason = RestartReason.MANUAL) => {
const goConfig = getGoConfig();
const cfg = await buildLanguageServerConfig(goConfig);

if (typeof reason === 'string') {
updateRestartHistory(goCtx, reason, cfg.enabled);
}
Expand All @@ -47,9 +46,14 @@ export const startLanguageServer: CommandFactory = (ctx, goCtx) => {
errorKind.manualRestart
);
}
outputChannel.info(`Try to start language server - ${reason} (enabled: ${cfg.enabled})`);

// If the client has already been started, make sure to clear existing
// diagnostics and stop it.
if (goCtx.languageClient) {
goCtx.serverOutputChannel?.append(
`Request to stop language server - ${reason} (enabled: ${cfg.enabled})`
);
await stopLanguageClient(goCtx);
}
updateStatus(goCtx, goConfig, false);
Expand All @@ -62,6 +66,7 @@ export const startLanguageServer: CommandFactory = (ctx, goCtx) => {
}

if (!shouldActivateLanguageFeatures()) {
outputChannel.warn('Cannot activate language features - unsupported environment');
return;
}

Expand All @@ -72,6 +77,7 @@ export const startLanguageServer: CommandFactory = (ctx, goCtx) => {
}

if (!cfg.enabled) {
outputChannel.warn('Language server is disabled');
const legacyService = new LegacyLanguageService();
goCtx.legacyLanguageService = legacyService;
ctx.subscriptions.push(legacyService);
Expand All @@ -87,11 +93,12 @@ export const startLanguageServer: CommandFactory = (ctx, goCtx) => {
ctx.globalState,
goCtx.serverInfo?.Commands
);

console.log(`Server: ${JSON.stringify(goCtx.serverInfo, null, 2)}`);
outputChannel.info(
`Running language server ${goCtx.serverInfo?.Name}(${goCtx.serverInfo?.Version}/${goCtx.serverInfo?.GoVersion})`
);
} catch (e) {
const msg = `Error starting language server: ${e}`;
console.log(msg);
outputChannel.error(msg);
goCtx.serverOutputChannel?.append(msg);
} finally {
updateStatus(goCtx, goConfig, true);
Expand Down
72 changes: 53 additions & 19 deletions extension/src/goInstallTools.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export interface IToolsManager {
installTool(tool: Tool, goVersion: GoVersion, env: NodeJS.Dict<string>): Promise<string | undefined>;
}

const defaultToolsManager: IToolsManager = {
export const defaultToolsManager: IToolsManager = {
getMissingTools,
installTool: installToolWithGo
};
Expand Down Expand Up @@ -105,23 +105,22 @@ export async function installAllTools(updateExistingToolsOnly = false) {
);
}

async function getGoForInstall(goVersion: GoVersion, silent?: boolean): Promise<GoVersion> {
const configured = getGoConfig().get<string>('toolsManagement.go');
export const getGoForInstall = _getGoForInstall;
async function _getGoForInstall(goVersion: GoVersion): Promise<GoVersion | undefined> {
let configured = getGoConfig().get<string>('toolsManagement.go');
if (!configured) {
return goVersion; // use the default.
configured = goVersion.binaryPath;
}

try {
const go = await getGoVersion(configured);
// goVersion may be the version picked based on the the minimum
// toolchain version requirement specified in go.mod or go.work.
// Compute the local toolchain version. (GOTOOLCHAIN=local go version)
const go = await getGoVersion(configured, 'local');
if (go) return go;
} catch (e) {
if (!silent) {
outputChannel.error(
`failed to run "go version" with "${configured}". Provide a valid path to the Go binary`
);
}
outputChannel.error(`failed to run "go version" with "${configured}". Provide a valid path to the Go binary`);
}
return goVersion;
return;
}

interface installToolsOptions {
Expand All @@ -135,7 +134,8 @@ interface installToolsOptions {
*
* @param missing array of tool names and optionally, their versions to be installed.
* If a tool's version is not specified, it will install the latest.
* @param goVersion version of Go used in the project. If go used for tools installation
* @param goVersion version of Go used in the project. (e.g. result of 'go version' from
* workspace root). If go used for tools installation
* is not configured or misconfigured, this is used as a fallback.
* @returns a list of tools that failed to install.
*/
Expand All @@ -144,34 +144,65 @@ export async function installTools(
goVersion: GoVersion,
options?: installToolsOptions
): Promise<{ tool: ToolAtVersion; reason: string }[]> {
// TODO(hyangah): the return value (tool, reason) is not used anywhere
// other than in tests. Check if we are giving users enough information
// about failed tools installation.
if (!missing) {
return [];
}
const { silent, skipRestartGopls } = options || {};
if (!silent) {
outputChannel.show();
}
outputChannel.clear();

const goForInstall = await getGoForInstall(goVersion);
if (!goForInstall || !goForInstall.isValid()) {
vscode.window.showErrorMessage('Failed to find a go command needed to install tools.');
outputChannel.show(); // show error.
return missing.map((tool) => {
return { tool: tool, reason: 'failed to find go' };
});
}

if (goForInstall.lt('1.16')) {
const minVersion = goForInstall.lt('1.21') ? (goVersion.lt('1.18') ? '1.18' : goVersion.format()) : '1.21';
if (goForInstall.lt(minVersion)) {
vscode.window.showErrorMessage(
'Go 1.16 or newer is needed to install tools. ' +
'If your project requires a Go version older than go1.16, either manually install the tools or, use the "go.toolsManagement.go" setting ' +
`Failed to find a go command (go${minVersion} or newer) needed to install tools. ` +
`The go command (${goForInstall.binaryPath}) is too old (go${goForInstall.svString}). ` +
'If your project requires a Go version older than go1.18, either manually install the tools or, use the "go.toolsManagement.go" setting ' +
'to configure the Go version used for tools installation. See https://github.com/golang/vscode-go/issues/2898.'
);
return missing.map((tool) => {
return { tool: tool, reason: 'require go1.16 or newer' };
return { tool: tool, reason: `failed to find go (requires go${minVersion} or newer)` };
});
}

const envForTools = toolInstallationEnvironment();
if (
!goVersion.isDevel &&
goVersion.sv &&
!goForInstall.isDevel &&
goForInstall.sv &&
semver.gt(goVersion.sv, goForInstall.sv)
) {
// If goVersion.isDevel === true, for example,
// go version go1.23-20240317-RC00 cl/616607620 +0a6f05e30f X:fieldtrack,boringcrypto linux/amd64
// go version devel go1.23-cd294f55ca Mon Apr 1 20:28:41 2024 +0000 darwin/amd64
// we optimisitically assume the go command chosen for install (goForInstall)
// is new enough (possibly newer than the officially released go version),
// and don't set GOTOOLCHAIN.
const version = goVersion.format(true);
envForTools['GOTOOLCHAIN'] = `go${version}+auto`;
}

const toolsGopath = envForTools['GOPATH'];
let envMsg = `Tools environment: GOPATH=${toolsGopath}`;
if (envForTools['GOBIN']) {
envMsg += `, GOBIN=${envForTools['GOBIN']}`;
}
if (envForTools['GOTOOLCHAIN']) {
envMsg += `, GOTOOLCHAIN=${envForTools['GOTOOLCHAIN']}`;
}
outputChannel.appendLine(envMsg);

let installingMsg = `Installing ${missing.length} ${missing.length > 1 ? 'tools' : 'tool'} at `;
Expand Down Expand Up @@ -245,6 +276,9 @@ async function tmpDirForToolInstallation() {
// installTool installs the specified tool.
export async function installTool(tool: ToolAtVersion): Promise<string | undefined> {
const goVersion = await getGoForInstall(await getGoVersion());
if (!goVersion) {
return 'failed to find "go" for install';
}
const envForTools = toolInstallationEnvironment();

return await installToolWithGo(tool, goVersion, envForTools);
Expand Down Expand Up @@ -292,7 +326,7 @@ async function installToolWithGoInstall(goVersion: GoVersion, env: NodeJS.Dict<s
};

const execFile = util.promisify(cp.execFile);
outputChannel.trace(`$ ${goBinary} install -v ${importPath}} (cwd: ${opts.cwd})`);
outputChannel.trace(`${goBinary} install -v ${importPath} (cwd: ${opts.cwd})`);
await execFile(goBinary, ['install', '-v', importPath], opts);
}

Expand Down
1 change: 0 additions & 1 deletion extension/src/goPlayground.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ export const playgroundCommand: CommandFactory = () => () => {
return promptForMissingTool(TOOL_CMD_NAME);
}

outputChannel.clear();
outputChannel.show();
outputChannel.appendLine('Upload to the Go Playground in progress...\n');

Expand Down
13 changes: 10 additions & 3 deletions extension/src/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -159,8 +159,12 @@ export function parseFilePrelude(text: string): Prelude {
* Gets version of Go based on the output of the command `go version`.
* Throws if go version can't be determined because go is not available
* or `go version` fails.
* If GOTOOLCHAIN is provided, it will be used to set GOTOOLCHAIN env var.
* For example, getGoVersion(binPath, 'local') can be used to query
* the local toolchain's go version regardless of the go version specified
* in the workspace go.mod or go.work.
*/
export async function getGoVersion(goBinPath?: string): Promise<GoVersion> {
export async function getGoVersion(goBinPath?: string, GOTOOLCHAIN?: string): Promise<GoVersion> {
// TODO(hyangah): limit the number of concurrent getGoVersion call.
// When the extension starts, at least 4 concurrent calls race
// and end up calling `go version`.
Expand Down Expand Up @@ -189,6 +193,9 @@ export async function getGoVersion(goBinPath?: string): Promise<GoVersion> {
let goVersion: GoVersion | undefined;
try {
const env = toolExecutionEnvironment();
if (GOTOOLCHAIN !== undefined) {
env['GOTOOLCHAIN'] = GOTOOLCHAIN;
}
const execFile = util.promisify(cp.execFile);
const { stdout, stderr } = await execFile(goRuntimePath, ['version'], { env, cwd });
if (stderr) {
Expand All @@ -198,8 +205,8 @@ export async function getGoVersion(goBinPath?: string): Promise<GoVersion> {
} catch (err) {
throw error(`failed to run "${goRuntimePath} version": ${err} cwd: ${cwd}`);
}
if (!goBinPath) {
// if getGoVersion was called with a given goBinPath, don't cache the result.
if (!goBinPath && !GOTOOLCHAIN) {
// if getGoVersion was called with a given goBinPath or an explicit GOTOOLCHAIN env var, don't cache the result.
cachedGoBinPath = goRuntimePath;
cachedGoVersion = goVersion;
if (!cachedGoVersion.isValid()) {
Expand Down
55 changes: 49 additions & 6 deletions extension/test/integration/install.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@
import AdmZip = require('adm-zip');
import assert from 'assert';
import * as config from '../../src/config';
import { inspectGoToolVersion, installTools, maybeInstallImportantTools } from '../../src/goInstallTools';
import {
IToolsManager,
defaultToolsManager,
inspectGoToolVersion,
installTools,
maybeInstallImportantTools
} from '../../src/goInstallTools';
import { Tool, getConfiguredTools, getTool, getToolAtVersion } from '../../src/goTools';
import { getBinPath, getGoVersion, GoVersion, rmdirRecursive } from '../../src/util';
import { correctBinname } from '../../src/utils/pathUtils';
Expand Down Expand Up @@ -78,11 +84,16 @@ suite('Installation Tests', function () {
// runTest actually executes the logic of the test.
// If withLocalProxy is true, the test does not require internet.
// If withGOBIN is true, the test will set GOBIN env var.
// If withGoVersion is set, the test will run assuming the project requires the version.
// If goForInstall is set, the test will use the go version for 'go install'.
// If toolsManager is set, goInstallTools will use it instead of the default tools Manager.
async function runTest(
testCases: installationTestCase[],
withLocalProxy?: boolean,
withGOBIN?: boolean,
withGoVersion?: string
withGoVersion?: string,
goForInstall?: GoVersion,
toolsManager?: goInstallTools.IToolsManager
) {
const gobin = withLocalProxy && withGOBIN ? path.join(tmpToolsGopath, 'gobin') : undefined;

Expand Down Expand Up @@ -115,14 +126,17 @@ suite('Installation Tests', function () {
}

const missingTools = testCases.map((tc) => getToolAtVersion(tc.name));
const goBinary = getBinPath('go');
const goVersion = withGoVersion
? /* we want a fake go version, but need the real 'go' binary to run `go install` */
new GoVersion(getBinPath('go'), `go version ${withGoVersion} amd64/linux`)
new GoVersion(goBinary, `go version ${withGoVersion} linux/amd64`)
: await getGoVersion();

sandbox.stub(vscode.commands, 'executeCommand').withArgs('go.languageserver.restart');
sandbox.stub(goInstallTools, 'getGoForInstall').returns(Promise.resolve(goForInstall ?? goVersion));

const failures = await installTools(missingTools, goVersion);
const opts = toolsManager ? { toolsManager } : undefined;
const failures = await installTools(missingTools, goVersion, opts);
assert(!failures || failures.length === 0, `installTools failed: ${JSON.stringify(failures)}`);

// Confirm that each expected tool has been installed.
Expand Down Expand Up @@ -197,7 +211,8 @@ suite('Installation Tests', function () {
});

test('Try to install with old go', async () => {
const oldGo = new GoVersion(getBinPath('go'), 'go version go1.15 amd64/linux');
const oldGo = new GoVersion(getBinPath('go'), 'go version go1.17 amd64/linux');
sandbox.stub(goInstallTools, 'getGoForInstall').returns(Promise.resolve(oldGo));
const failures = await installTools([getToolAtVersion('gopls')], oldGo);
assert(failures?.length === 1 && failures[0].tool.name === 'gopls' && failures[0].reason.includes('or newer'));
});
Expand All @@ -216,9 +231,37 @@ suite('Installation Tests', function () {
[{ name: 'gofumpt', versions: ['v0.4.0', 'v0.5.0', gofumptDefault], wantVersion: gofumptDefault }],
true, // LOCAL PROXY
true, // GOBIN
'go1.22' // Go Version
'go1.22.0' // Go Version
);
});

test('Install a tool, with go1.21.0', async () => {
const systemGoVersion = await getGoVersion();
const oldGo = new GoVersion(systemGoVersion.binaryPath, 'go version go1.21.0 linux/amd64');
const tm: IToolsManager = {
getMissingTools: () => {
assert.fail('must not be called');
},
installTool: (tool, goVersion, env) => {
// Assert the go install command is what we expect.
assert.strictEqual(tool.name, 'gopls');
assert.strictEqual(goVersion, oldGo);
assert(env['GOTOOLCHAIN'], `go${systemGoVersion.format()}+auto`);
// runTest checks if the tool build succeeds. So, delegate the remaining
// build task to the default tools manager's installTool function.
return defaultToolsManager.installTool(tool, goVersion, env);
}
};
await runTest(
[{ name: 'gopls', versions: ['v0.1.0', 'v1.0.0'], wantVersion: 'v1.0.0' }],
true, // LOCAL PROXY
true, // GOBIN
'go' + systemGoVersion.format(true), // Go Version
oldGo, // Go for install
tm // stub installTool to
);
});

test('Install all tools via GOPROXY', async () => {
// Only run this test if we are in CI before a Nightly release.
if (!shouldRunSlowTests()) {
Expand Down

0 comments on commit ff0beea

Please sign in to comment.