[vscode-go] src/goInstallTools: use `go install` for go1.16+
25 views
Skip to first unread message
Hyang-Ah Hana Kim (Gerrit)
Oct 14, 2021, 6:02:57 PM10/14/21
to goph...@pubsubhelper.golang.org, Hyang-Ah Hana Kim, golang-co...@googlegroups.com
Hyang-Ah Hana Kim has uploaded this change for review.
src/goInstallTools: use `go install` for go1.16+
`go install` with the version specifier is THE way to build a binary
in recent versions of Go. From go1.18, `go get` will not do build & install
so shouldn't be used.
For `dlv-dap` or `gocode-gomod`, we use `go get` + `go build` because
`go install` does not allow to specify output path.
(https://github.com/golang/go/issues/44469)
Use of `go install` allows us to address another issue: we no longer
require a dummy main module in a temp directory, and we can run it from
the workspace directory. The use of temp directory broke direnv, asdf
users who configured their go to pick a different version of go based on
the directory. Thus, we pinned the go binary by selecting the go binary
from the current GOROOT's bin directory. Unfortunately, that broke
another group of users who are using GOROOT for different purpose.
(https://github.com/golang/vscode-go/discussions/1691)
Now we can remove the hack to pin the go binary and use the go binary
path as it is, and simplifies the installation logic.
In this mode, I chose to stream stdout/stderr to the Go output
channel instead of bufferring it and later passing as a part of
error details. Personally I like to see the progress and the details,
but some users may find this too overwhelming. In that case, I think
we can control it using a setting.
Updates golang/vscode-go#1825
Change-Id: Idb1604e4484cd73832c24c0077220f8cc57dfaa0
---
M src/goTools.ts
M src/goInstallTools.ts
2 files changed, 136 insertions(+), 41 deletions(-)
diff --git a/src/goInstallTools.ts b/src/goInstallTools.ts
index 6bd0dda..95655b3 100644
--- a/src/goInstallTools.ts
+++ b/src/goInstallTools.ts
@@ -169,7 +169,7 @@
const failures: { tool: ToolAtVersion; reason: string }[] = [];
for (const result of results) {
- if (result.reason === '') {
+ if (!result.reason) {
// Restart the language server if a new binary has been installed.
if (result.tool.name === 'gopls') {
restartLanguageServer('installation');
@@ -222,16 +222,81 @@
return reason;
}
}
- let toolsTmpDir = '';
- try {
- toolsTmpDir = await tmpDirForToolInstallation();
- } catch (e) {
- return `Failed to create a temp directory: ${e}`;
- }
const env = Object.assign({}, envForTools);
env['GO111MODULE'] = modulesOn ? 'on' : 'off';
+ let importPath: string;
+ if (!modulesOn) {
+ importPath = getImportPath(tool, goVersion);
+ } else {
+ let version: semver.SemVer | string | undefined = tool.version;
+ if (!version) {
+ if (tool.usePrereleaseInPreviewMode && isInPreviewMode()) {
+ version = await latestToolVersion(tool, true);
+ } else if (tool.defaultVersion) {
+ version = tool.defaultVersion;
+ }
+ }
+ importPath = getImportPathWithVersion(tool, version, goVersion);
+ }
+
+ try {
+ if (!modulesOn || goVersion.lt('1.16') || hasModSuffix(tool) || tool.name === 'dlv-dap') {
+ await installToolWithGoGet(tool, goVersion, env, modulesOn, importPath);
+ } else {
+ await installToolWithGoInstall(tool, goVersion, env, importPath, outputChannel);
+ }
+ const toolInstallPath = getBinPath(tool.name);
+ outputChannel.appendLine(`Installing ${importPath} (${toolInstallPath}) SUCCEEDED`);
+ } catch (e) {
+ outputChannel.appendLine(`Installing ${importPath} FAILED`);
+ outputChannel.appendLine(`${JSON.stringify(e, null, 1)}`);
+ return `failed to install ${tool.name}(${importPath}): ${e}`;
+ }
+}
+
+function installToolWithGoInstall(
+ tool: ToolAtVersion,
+ goVersion: GoVersion,
+ env: NodeJS.Dict<string>,
+ importPath: string,
+ channel: {
+ append: (value: string) => void;
+ appendLine: (value: string) => void;
+ }
+) {
+ // Unlike installToolWithGoGet, `go install` in module mode
+ // can run in the current directory safely. So, use the user-specified go tool path.
+ const goBinary = goVersion.binaryPath || getBinPath('go');
+ const opts = {
+ env,
+ cwd: getWorkspaceFolderPath()
+ };
+
+ return new Promise<void>((resolve, reject) => {
+ channel.appendLine(`$ ${goBinary} install -x ${importPath} (cwd: ${opts.cwd})`);
+ const p = cp.execFile(goBinary, ['install', '-x', importPath], opts);
+ p.stdout.on('data', (chunk) => channel.append(chunk));
+ p.stderr.on('data', (chunk) => channel.append(chunk));
+ p.on('close', (code, signal) => {
+ if (code) {
+ channel.appendLine(`exited with code: ${code}`);
+ reject(`exited with code: ${code} (signal: ${signal})`);
+ } else {
+ resolve();
+ }
+ });
+ });
+}
+
+async function installToolWithGoGet(
+ tool: ToolAtVersion,
+ goVersion: GoVersion,
+ env: NodeJS.Dict<string>,
+ modulesOn: boolean,
+ importPath: string
+) {
// Some users use direnv-like setup where the choice of go is affected by
// the current directory path. In order to avoid choosing a different go,
// we will explicitly use `GOROOT/bin/go` instead of goVersion.binaryPath
@@ -251,33 +316,26 @@
if (hasModSuffix(tool) || tool.name === 'dlv-dap') {
args.push('-d'); // get the version, but don't build.
}
- let importPath: string;
- if (!modulesOn) {
- importPath = getImportPath(tool, goVersion);
- } else {
- let version: semver.SemVer | string | undefined = tool.version;
- if (!version) {
- if (tool.usePrereleaseInPreviewMode && isInPreviewMode()) {
- version = await latestToolVersion(tool, true);
- } else if (tool.defaultVersion) {
- version = tool.defaultVersion;
- }
- }
- importPath = getImportPathWithVersion(tool, version, goVersion);
- }
args.push(importPath);
- let output = 'no output';
- let result = '';
+ let toolsTmpDir = '';
try {
- const opts = {
- env,
- cwd: toolsTmpDir
- };
+ toolsTmpDir = await tmpDirForToolInstallation();
+ } catch (e) {
+ throw new Error(`Failed to create a temp directory: ${e}`);
+ }
+
+ const opts = {
+ env,
+ cwd: toolsTmpDir
+ };
+ try {
const execFile = util.promisify(cp.execFile);
+ logVerbose(`$ ${goBinary} ${args.join(' ')} (cwd: ${opts.cwd})`);
+
const { stdout, stderr } = await execFile(goBinary, args, opts);
- output = `${stdout} ${stderr}`;
- logVerbose(`install: ${goBinary} ${args.join(' ')}\n${stdout}${stderr}`);
+ logVerbose(`${stdout}\n${stderr}`);
+
if (hasModSuffix(tool) || tool.name === 'dlv-dap') {
// Actual installation of the -gomod tool and dlv-dap is done by running go build.
let destDir = env['GOBIN'];
@@ -291,21 +349,22 @@
const outputFile = path.join(destDir, correctBinname(tool.name));
// go build does not take @version suffix yet.
- const importPath = getImportPath(tool, goVersion);
- await execFile(goBinary, ['build', '-o', outputFile, importPath], opts);
+ const importPathWithoutVersion = getImportPath(tool, goVersion);
+ logVerbose(`$ ${goBinary} build -o ${outputFile} ${importPathWithoutVersion} (cwd: ${opts.cwd})`);
+ const { stdout, stderr } = await execFile(
+ goBinary,
+ ['build', '-o', outputFile, importPathWithoutVersion],
+ opts
+ );
+ logVerbose(`${stdout}\n${stderr}`);
}
- const toolInstallPath = getBinPath(tool.name);
- outputChannel.appendLine(`Installing ${importPath} (${toolInstallPath}) SUCCEEDED`);
} catch (e) {
- outputChannel.appendLine(`Installing ${importPath} FAILED`);
- outputChannel.appendLine(`${JSON.stringify(e, null, 1)}`);
- result = `failed to install ${tool.name}(${importPath}): ${e} ${output}`;
+ logVerbose(`FAILED: ${JSON.stringify(e, null, 1)}`);
+ throw e;
} finally {
// Delete the temporary installation directory.
rmdirRecursive(toolsTmpDir);
}
-
- return result;
}
export function declinedToolInstall(toolName: string) {
@@ -662,11 +721,11 @@
dep github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
if the binary was built in GOPATH mode => the following code will throw an error which will be handled.
- /Users/hakim/go/bin/gopls: go1.16
+ /Users/hakim/go/bin/gopls: go1.16
if the binary was built in dev branch, in module mode => the following code will not throw an error,
and return (devel) as the moduleVersion.
- /Users/hakim/go/bin/gopls: go1.16
+ /Users/hakim/go/bin/gopls: go1.16
path golang.org/x/tools/gopls
mod golang.org/x/tools/gopls (devel)
dep github.com/BurntSushi/toml v0.3.1 h1:WXkYYl6Yr3qBf1K79EBnL4mak0OimBfB0XUf9Vl28OQ=
diff --git a/src/goTools.ts b/src/goTools.ts
index cb68bd0..d6eba5a 100644
--- a/src/goTools.ts
+++ b/src/goTools.ts
@@ -86,7 +86,7 @@
return importPath + '@' + version;
}
}
- return importPath;
+ return importPath + '@latest';
}
export function containsTool(tools: Tool[], tool: Tool): boolean {
To view, visit change 355974. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Oct 14, 2021, 6:14:05 PM10/14/21
to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: FAILURE
Logs at: https://source.cloud.google.com/results/invocations/10de6567-66f6-40a8-8055-ed1aa676b348
Patch set 1:TryBot-Result -1
Hyang-Ah Hana Kim (Gerrit)
Oct 14, 2021, 8:34:18 PM10/14/21
to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Hyang-Ah Hana Kim.
Hyang-Ah Hana Kim uploaded patch set #2 to this change.
src/goInstallTools: use `go install` for go1.16+
`go install` with the version specifier is THE way to install a binary
in recent versions of Go. From go1.18, `go get` will not do build & install
so shouldn't be used.
For `dlv-dap` or `gocode-gomod`, we use `go get` + `go build` because
`go install` does not allow to specify output path.
(https://github.com/golang/go/issues/44469)
Use of `go install` allows us to address another issue: we no longer
require a dummy main module in a temp directory, and we can run it from
the workspace directory. The use of temp directory broke direnv, asdf
users who configured their go to pick a different version of go based on
the directory. Thus, we pinned the go binary by selecting the go binary
from the current GOROOT's bin directory. Unfortunately, that broke
another group of users who are using GOROOT for different purpose.
(https://github.com/golang/vscode-go/discussions/1691)
Now we can remove the hack to pin the go binary and use the go binary
path as it is, and simplifies the installation logic.
Reduced the verbose logging printed in the console.
Instead, we log the command and the directory path so users could
reproduce. In case of errors, the exception still includes all the stdout
and stderr.
Updates golang/vscode-go#1825
Change-Id: Idb1604e4484cd73832c24c0077220f8cc57dfaa0
---
M src/goTools.ts
M src/goInstallTools.ts
2 files changed, 119 insertions(+), 61 deletions(-)
To view, visit change 355974. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Oct 14, 2021, 8:47:01 PM10/14/21
to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Attention is currently required from: Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/2cbbe2dc-2f3c-44d9-8ce0-8e1bbf2c2884
Patch set 2:TryBot-Result +1
Suzy Mueller (Gerrit)
Oct 18, 2021, 5:03:44 PM10/18/21
to Hyang-Ah Hana Kim, goph...@pubsubhelper.golang.org, kokoro, golang-co...@googlegroups.com
Attention is currently required from: Hyang-Ah Hana Kim.
Patch set 2:Code-Review +2
Reply all
Reply to author
Forward
0 new messages