[vscode-go] tools/installtools: install staticcheck@v0.2.2 for go < 1.17
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim has uploaded this change for review.
tools/installtools: install stati...@v0.2.2 for go < 1.17
Updates golang/vscode-go#2162
Change-Id: I5adc4ca6d7b0f8a8e3df7e1f403917fc94ef7b23
---
M tools/installtools/main.go
1 file changed, 26 insertions(+), 0 deletions(-)
diff --git a/tools/installtools/main.go b/tools/installtools/main.go
index d892bd2..efd9e9a 100644
--- a/tools/installtools/main.go
+++ b/tools/installtools/main.go
@@ -34,11 +34,26 @@
{"github.com/go-delve/delve/cmd/dlv", "", ""},
}
+// pickCompatibleVersions updates tools' version fields
+// so tools can be built with the given go version (minor version)
+func usePinnedVersions(goVersion int) {
+ for i, tool := range tools {
+ if tool.path == "honnef.co/go/tools/cmd/staticcheck" {
+ if goVersion < 17 {
+ tool.version = "v0.2.2"
+ }
+ }
+ tools[i] = tool
+ }
+}
+
func main() {
ver, err := goVersion()
if err != nil {
exitf("failed to find go version: %v", err)
}
+ usePinnedVersions(ver)
+
bin, err := goBin()
if err != nil {
exitf("failed to determine go tool installation directory: %v", err)
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
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/853f0d94-b245-488a-a645-4c943b77c4ef
Patch set 1:TryBot-Result -1
kokoro (Gerrit)
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/fce07d5c-626a-4f39-a41a-249861980c78
Patch set 2:TryBot-Result -1
kokoro (Gerrit)
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/bf5feeaf-306e-48ee-9292-f601eb327b0a
Patch set 3:TryBot-Result +1
Robert Findley (Gerrit)
Attention is currently required from: Hyang-Ah Hana Kim.
2 comments:
File tools/installtools/main.go:
Patch Set #3, Line 37:
pickCompatibleVersionsThis docstring looks inaccurate
Patch Set #3, Line 55:
usePinnedVersions(ver)This is minor, but I'd prefer if we weren't mutating the tool variable as we go. Eventually we'll have a more robust compatibility matrix, and it makes sense for it to be data-driven, rather than hard-coded in this way. We can set ourselves up for that (and make the code
Can we do the following?
```
// supportedVersion defines the last Version supported at GoVersion
type supportedVersion struct {
GoVersion, Version string
}
```And then
```
var tools = []struct {
path string
dest string
supportedVersions []supportedVersion
} {
{"honnef.co/go/tools/cmd/staticcheck", "", []supportedVersion{
{ "go1.16", "v0.2.2"},
}}
}
```Theoretically, we can also name the row:
```
type supportedTool struct {
path string
dest string
supportedVersions []supportedVersion
}func (t supportedTool) VersionForGoVersion(goVersion int) string { ... }
```Obviously, better names are welcome :)
This way, the tools table never needs to be mutated, and in the future can potentially be read from a common data source.
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Hyang-Ah Hana Kim.
Hyang-Ah Hana Kim uploaded patch set #4 to this change.
tools/installtools: install stati...@v0.2.2 for go < 1.17
And let the ci run `go test ./...`
Updates golang/vscode-go#2162
Change-Id: I5adc4ca6d7b0f8a8e3df7e1f403917fc94ef7b23
---
M build/all.bash
M tools/installtools/main.go
A tools/installtools/main_test.go
3 files changed, 111 insertions(+), 30 deletions(-)
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Robert Findley.
Patch set 3:Run-TryBot +1
2 comments:
File tools/installtools/main.go:
Patch Set #3, Line 37:
pickCompatibleVersionsThis docstring looks inaccurate
Ack
Patch Set #3, Line 55:
usePinnedVersions(ver)
This is minor, but I'd prefer if we weren't mutating the tool variable as we go. […]
Done.
Re: compatibility matrix -
I am not sure if we want to take the route.
That adds complexity for diminishing returns and false promise IMO.Let's say gopls stops supporting build with go version X and the last gopls version that works for X is Y. At some point in the future, a user tries to work with go version X, and the extension happily picks the version Y. Can we safely assume that gopls@Y is compatible with the current extension & vscode - what if we encounter another LSP version incompatibility between gopls and vscode? Or, what if later a vulnerability or crashing bug is discovered in gopls Y, but it's fixed in the latest version of gopls. Do we want gopls to release Y' with the vulnerability patch?
I think a better path is to ask users to build the tool with the later version of go, or we fully manage the choice of go for installation, or distribute with tool binaries.
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: NOT_BUILT
Logs at: https://source.cloud.google.com/results/invocations/f0276e07-ee30-4fdd-ae0d-8c7f81a011b0
Patch set 4:TryBot-Result -1
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Hyang-Ah Hana Kim removed a vote from this change.
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Robert Findley.
1 comment:
Patchset:
kokoro rerun
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: NOT_BUILT
Logs at: https://source.cloud.google.com/results/invocations/cc1e1f61-998a-469e-a907-b62389a9f64c
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Hyang-Ah Hana Kim uploaded patch set #5 to this change.
tools/installtools: install stati...@v0.2.2 for go < 1.17
Updates golang/vscode-go#2162
Change-Id: I5adc4ca6d7b0f8a8e3df7e1f403917fc94ef7b23
---
M tools/installtools/main.go
A tools/installtools/main_test.go
2 files changed, 106 insertions(+), 30 deletions(-)
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Attention is currently required from: Robert Findley, Hyang-Ah Hana Kim.
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/f65f2372-8f54-445b-a76e-471294df9691
Patch set 5:TryBot-Result +1
Robert Findley (Gerrit)
Attention is currently required from: Hyang-Ah Hana Kim.
Patch set 5:Code-Review +2
3 comments:
File tools/installtools/main.go:
Patch Set #3, Line 55:
usePinnedVersions(ver)Done. […]
That's a very good point, but I think we still need to encode this information: it is useful to know that the Go version is not supported anymore, so that we can provide a helpful dialog to the user, rather than just trying to build and failing (or worse, building successfully and encountering a runtime failure due to an old bug in Go that gopls no longer works around).
I think the new logic is cleaner, thanks.
File tools/installtools/main.go:
Patch Set #5, Line 15:
minorversiongoMinorVersion
Patch Set #5, Line 16:
supportedVersionI know I suggested this name, but maybe finalVersion is a better name?
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
Attention is currently required from: Hyang-Ah Hana Kim.
Hyang-Ah Hana Kim uploaded patch set #6 to this change.
tools/installtools: install stati...@v0.2.2 for go < 1.17
Updates golang/vscode-go#2162
Change-Id: I5adc4ca6d7b0f8a8e3df7e1f403917fc94ef7b23
---
M tools/installtools/main.go
A tools/installtools/main_test.go
2 files changed, 106 insertions(+), 30 deletions(-)
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
Hyang-Ah Hana Kim (Gerrit)
2 comments:
File tools/installtools/main.go:
Patch Set #5, Line 15:
minorversiongoMinorVersion
Done
Patch Set #5, Line 16:
supportedVersionI know I suggested this name, but maybe finalVersion is a better name?
Done
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.
kokoro (Gerrit)
Kokoro presubmit build finished with status: SUCCESS
Logs at: https://source.cloud.google.com/results/invocations/0201f330-1069-47eb-a11b-21be607b1408
Patch set 6:TryBot-Result +1
Hyang-Ah Hana Kim (Gerrit)
Patch set 6:Code-Review +1
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim submitted this change.
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: tools/installtools/main.go
Insertions: 6, Deletions: 6.
@@ -11,9 +11,9 @@
"strings"
)
-// supportedVersion encodes the fact that the specified tool version
-// is the last version that can be buildable with the minorversion.
-type supportedVersion struct {
+// finalVersion encodes the fact that the specified tool version
+// is the known last version that can be buildable with goMinorVersion.
+type finalVersion struct {
goMinorVersion int
version string
}
@@ -26,7 +26,7 @@
// add a fake entry with a large goMinorVersion
// value and the pinned tool version as the last entry.
// Nil of empty list indicates we can use the `latest` version.
- versions []supportedVersion
+ versions []finalVersion
}{
// TODO: auto-generate based on allTools.ts.in.
{"golang.org/x/tools/gopls", "", nil},
@@ -41,14 +41,14 @@
{"github.com/sqs/goreturns", "", nil},
{"github.com/uudashr/gopkgs/v2/cmd/gopkgs", "", nil},
{"github.com/zmb3/gogetdoc", "", nil},
- {"honnef.co/go/tools/cmd/staticcheck", "", []supportedVersion{{16, "v0.2.2"}}},
+ {"honnef.co/go/tools/cmd/staticcheck", "", []finalVersion{{16, "v0.2.2"}}},
{"golang.org/x/tools/cmd/gorename", "", nil},
{"github.com/go-delve/delve/cmd/dlv", "", nil},
}
// pickVersion returns the version to install based on the supported
// version list.
-func pickVersion(goMinorVersion int, versions []supportedVersion) string {
+func pickVersion(goMinorVersion int, versions []finalVersion) string {
for _, v := range versions {
if goMinorVersion <= v.goMinorVersion {
return v.version
```
```
The name of the file: tools/installtools/main_test.go
Insertions: 3, Deletions: 3.
@@ -6,7 +6,7 @@
func Test_pickVersion(t *testing.T) {
tests := []struct {
name string
- versions []supportedVersion
+ versions []finalVersion
want map[int]string
}{
{
@@ -16,14 +16,14 @@
},
{
name: "one_entry",
- versions: []supportedVersion{
+ versions: []finalVersion{
{16, "v0.2.2"},
},
want: map[int]string{15: "v0.2.2", 16: "v0.2.2", 17: "latest", 18: "latest"},
},
{
name: "two_entries",
- versions: []supportedVersion{
+ versions: []finalVersion{
{16, "v0.2.2"},
{17, "v0.3.0"},
},
```
tools/installtools: install stati...@v0.2.2 for go < 1.17
Updates golang/vscode-go#2162
Change-Id: I5adc4ca6d7b0f8a8e3df7e1f403917fc94ef7b23
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/400362
Run-TryBot: Hyang-Ah Hana Kim <hya...@gmail.com>
Reviewed-by: Robert Findley <rfin...@google.com>
TryBot-Result: kokoro <noreply...@google.com>
Reviewed-by: Hyang-Ah Hana Kim <hya...@gmail.com>
---
M tools/installtools/main.go
A tools/installtools/main_test.go
2 files changed, 111 insertions(+), 30 deletions(-)
diff --git a/tools/installtools/main.go b/tools/installtools/main.go
index d892bd2..f33f19e 100644
--- a/tools/installtools/main.go
+++ b/tools/installtools/main.go
@@ -11,27 +11,50 @@
"strings"
)
+// finalVersion encodes the fact that the specified tool version
+// is the known last version that can be buildable with goMinorVersion.
+type finalVersion struct {
+ goMinorVersion int
+ version string
+}
+
var tools = []struct {
- path string
- version string
- dest string
+ path string
+ dest string
+ // versions is a list of supportedVersions sorted by
+ // goMinorVersion. If we want to pin a tool's version
+ // add a fake entry with a large goMinorVersion
+ // value and the pinned tool version as the last entry.
+ // Nil of empty list indicates we can use the `latest` version.
+ versions []finalVersion
}{
// TODO: auto-generate based on allTools.ts.in.
- {"golang.org/x/tools/gopls", "", ""},
- {"github.com/acroca/go-symbols", "", ""},
- {"github.com/cweill/gotests/gotests", "", ""},
- {"github.com/davidrjenni/reftools/cmd/fillstruct", "", ""},
- {"github.com/haya14busa/goplay/cmd/goplay", "", ""},
- {"github.com/stamblerre/gocode", "", "gocode-gomod"},
- {"github.com/mdempsky/gocode", "", ""},
- {"github.com/ramya-rao-a/go-outline", "", ""},
- {"github.com/rogpeppe/godef", "", ""},
- {"github.com/sqs/goreturns", "", ""},
- {"github.com/uudashr/gopkgs/v2/cmd/gopkgs", "", ""},
- {"github.com/zmb3/gogetdoc", "", ""},
- {"honnef.co/go/tools/cmd/staticcheck", "", ""},
- {"golang.org/x/tools/cmd/gorename", "", ""},
- {"github.com/go-delve/delve/cmd/dlv", "", ""},
+ {"golang.org/x/tools/gopls", "", nil},
+ {"github.com/acroca/go-symbols", "", nil},
+ {"github.com/cweill/gotests/gotests", "", nil},
+ {"github.com/davidrjenni/reftools/cmd/fillstruct", "", nil},
+ {"github.com/haya14busa/goplay/cmd/goplay", "", nil},
+ {"github.com/stamblerre/gocode", "gocode-gomod", nil},
+ {"github.com/mdempsky/gocode", "", nil},
+ {"github.com/ramya-rao-a/go-outline", "", nil},
+ {"github.com/rogpeppe/godef", "", nil},
+ {"github.com/sqs/goreturns", "", nil},
+ {"github.com/uudashr/gopkgs/v2/cmd/gopkgs", "", nil},
+ {"github.com/zmb3/gogetdoc", "", nil},
+ {"honnef.co/go/tools/cmd/staticcheck", "", []finalVersion{{16, "v0.2.2"}}},
+ {"golang.org/x/tools/cmd/gorename", "", nil},
+ {"github.com/go-delve/delve/cmd/dlv", "", nil},
+}
+
+// pickVersion returns the version to install based on the supported
+// version list.
+func pickVersion(goMinorVersion int, versions []finalVersion) string {
+ for _, v := range versions {
+ if goMinorVersion <= v.goMinorVersion {
+ return v.version
+ }
+ }
+ return "latest"
}
func main() {
@@ -39,17 +62,15 @@
if err != nil {
exitf("failed to find go version: %v", err)
}
+ if ver < 1 {
+ exitf("unsupported go version: 1.%v", ver)
+ }
+
bin, err := goBin()
if err != nil {
exitf("failed to determine go tool installation directory: %v", err)
}
- if ver < 1 {
- exitf("unsupported go version: 1.%v", ver)
- } else if ver < 16 {
- err = installTools(bin, "get")
- } else {
- err = installTools(bin, "install")
- }
+ err = installTools(bin, ver)
if err != nil {
exitf("failed to install tools: %v", err)
}
@@ -100,17 +121,19 @@
return filepath.Join(gopaths[0], "bin"), nil
}
-func installTools(binDir, installCmd string) error {
+func installTools(binDir string, goMinorVersion int) error {
+ installCmd := "install"
+ if goMinorVersion < 16 {
+ installCmd = "get"
+ }
+
dir := ""
if installCmd == "get" { // run `go get` command from an empty directory.
dir = os.TempDir()
}
env := append(os.Environ(), "GO111MODULE=on")
for _, tool := range tools {
- ver := tool.version
- if ver == "" {
- ver = "latest"
- }
+ ver := pickVersion(goMinorVersion, tool.versions)
path := tool.path + "@" + ver
cmd := exec.Command("go", installCmd, path)
cmd.Env = env
diff --git a/tools/installtools/main_test.go b/tools/installtools/main_test.go
new file mode 100644
index 0000000..99e0961
--- /dev/null
+++ b/tools/installtools/main_test.go
@@ -0,0 +1,42 @@
+// Binary installtools is a helper that installs Go tools extension tests depend on.
+package main
+
+import "testing"
+
+func Test_pickVersion(t *testing.T) {
+ tests := []struct {
+ name string
+ versions []finalVersion
+ want map[int]string
+ }{
+ {
+ name: "nil",
+ versions: nil,
+ want: map[int]string{15: "latest", 16: "latest", 17: "latest", 18: "latest"},
+ },
+ {
+ name: "one_entry",
+ versions: []finalVersion{
+ {16, "v0.2.2"},
+ },
+ want: map[int]string{15: "v0.2.2", 16: "v0.2.2", 17: "latest", 18: "latest"},
+ },
+ {
+ name: "two_entries",
+ versions: []finalVersion{
+ {16, "v0.2.2"},
+ {17, "v0.3.0"},
+ },
+ want: map[int]string{15: "v0.2.2", 16: "v0.2.2", 17: "v0.3.0", 18: "latest"},
+ },
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ for goMinorVersion, want := range tt.want {
+ if got := pickVersion(goMinorVersion, tt.versions); got != want {
+ t.Errorf("pickVersion(go 1.%v) = %v, want %v", goMinorVersion, got, want)
+ }
+ }
+ })
+ }
+}
To view, visit change 400362. To unsubscribe, or for help writing mail filters, visit settings.