[dl] internal/version: set GOTOOLCHAIN=local+auto for gotip
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim has uploaded the change for review![]( "Open in Gerrit")
Commit message
internal/version: set GOTOOLCHAIN=local+auto for gotip
Address issue 64665 by setting GOTOOLCHAIN=local+auto
when invoking the real go binary from the gotip wrapper.
This ensures that user's global GOTOOLCHAIN setting
(set by `go env -w`) doesn't override gotip behavior.
Other version wrappers' behavior is left unchanged.
Fixes golang/go#64665
Change diff
diff --git a/internal/version/gotip.go b/internal/version/gotip.go
index 76ce7bb..f5c6f4e 100644
--- a/internal/version/gotip.go
+++ b/internal/version/gotip.go
@@ -46,7 +46,7 @@
log.Fatalf("gotip: not downloaded. Run 'gotip download' to install to %v", root)
}
- runGo(root)
+ runGo(root, "local+auto")
}
func installTip(root, target string) error {
diff --git a/internal/version/version.go b/internal/version/version.go
index 783a2c2..e7337f2 100644
--- a/internal/version/version.go
+++ b/internal/version/version.go
@@ -51,10 +51,10 @@
log.Fatalf("%s: not downloaded. Run '%s download' to install to %v", version, version, root)
}
- runGo(root)
+ runGo(root, "")
}
-func runGo(root string) {
+func runGo(root, gotoolchain string) {
gobin := filepath.Join(root, "bin", "go"+exe())
cmd := exec.Command(gobin, os.Args[1:]...)
cmd.Stdin = os.Stdin
@@ -64,7 +64,11 @@
if p := os.Getenv("PATH"); p != "" {
newPath += string(filepath.ListSeparator) + p
}
- cmd.Env = dedupEnv(caseInsensitiveEnv, append(os.Environ(), "GOROOT="+root, "PATH="+newPath))
+ env := append(os.Environ(), "GOROOT="+root, "PATH="+newPath)
+ if gotoolchain != "" {
+ env = append(env, "GOTOOLCHAIN="+gotoolchain)
+ }
+ cmd.Env = dedupEnv(caseInsensitiveEnv, env)
handleSignals()
Change information
- M internal/version/gotip.go
- M internal/version/version.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim uploaded new patchset![]( "Open in Gerrit")
Related details
- Dmitri Shuralyov
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Dmitri Shuralyov
- Michael Matloob
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Matloob
| Code-Review | +2 |
1 comment
os.Setenv("GOTOOLCHAIN", tt.envValue)I wonder if we can make computeEnv take the environment as an argument, so that we don't need to set the process's environment.
Related details
- Dmitri Shuralyov
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Dmitri Shuralyov
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Dmitri Shuralyov
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim uploaded new patchset![]( "Open in Gerrit")
Related details
- Dmitri Shuralyov
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim added 1 comment![]( "Open in Gerrit")
os.Setenv("GOTOOLCHAIN", tt.envValue)I wonder if we can make computeEnv take the environment as an argument, so that we don't need to set the process's environment.
PTAL
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Matloob (Gerrit)
Michael Matloob voted and added 1 comment![]( "Open in Gerrit")
Votes added by Michael Matloob
| Code-Review | +2 |
1 comment
if p := os.Getenv("PATH"); p != "" {Should we pull out the value of PATH from baseEnv?
Related details
- Dmitri Shuralyov
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmitri Shuralyov (Gerrit)
Dmitri Shuralyov added 5 comments![]( "Open in Gerrit")
Thanks. This generally seems fine to me, and Michael's already reviewed it. I left a few comments that were on my mind, but they're minor and I think you should feel free to proceed.
internal/version: set GOTOOLCHAIN=local+auto for gotipSince this changes behavior of the `golang.org/dl/gotip` command only, and not all other goA.B.C commands that also use the `internal/version` package, it might be clearer to put that in the path prefix, something like:
```diff
-internal/version: set GOTOOLCHAIN=local+auto for gotip
+gotip: set GOTOOLCHAIN=...
```
(Or "gotip, internal/version: set GOTOOLCHAIN=..." if you prefer to keep internal/version mentioned too.)
Address issue 64665 by setting GOTOOLCHAIN=local+autoA question about this choice.
https://go.dev/doc/toolchain#select documents that "GOTOOLCHAIN=auto is shorthand for GOTOOLCHAIN=local+auto". So both `GOTOOLCHAIN=local+auto` and `GOTOOLCHAIN=auto` here will behave identically.
The default value in go is to use the shorthand version:
https://cs.opensource.google/go/go/+/master:go.env;l=12;drc=35268a996052ca8716caf94467c2ed61140f3862
Is it worth doing the same here, to keep them aligned? Unless using the long-form "local+auto" version an intentional choice.
This ensures that user's global GOTOOLCHAIN setting
(set by `go env -w`) doesn't override gotip behavior.Some of the discussion on the issue was to consider explicitly erroring out on conflict, or to go with GOTOOLCHAIN=local (i.e., without +auto), but I understand this plan is different in order to strike a better balance, is that right?
I agree that it seems this is more likely to help get around problems than it is to cause problems, but I'll note that given this doesn't seem to be documented behavior, it may cause someone else to run into the (admittedly even more niche case) where the gotip command behaves unexpectedly and report that as an issue.
Another complication is that the gotip command supports downloading CL numbers or branch names, so in some cases it might be further from Go at tip.
It's probably fine, and we can still adjust it if we get new information.
Fixes golang/go#64665Just checking, the original report linked to https://go.dev/doc/manage-install, but that page doesn't seem to mention gotip at all anymore; maybe that's because the manage-install documentation changed?
Related details
- Hyang-Ah Hana Kim
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hyang-Ah Hana Kim (Gerrit)
Hyang-Ah Hana Kim added 2 comments![]( "Open in Gerrit")
Address issue 64665 by setting GOTOOLCHAIN=local+autoA question about this choice.
https://go.dev/doc/toolchain#select documents that "GOTOOLCHAIN=auto is shorthand for GOTOOLCHAIN=local+auto". So both `GOTOOLCHAIN=local+auto` and `GOTOOLCHAIN=auto` here will behave identically.
The default value in go is to use the shorthand version:
https://cs.opensource.google/go/go/+/master:go.env;l=12;drc=35268a996052ca8716caf94467c2ed61140f3862
Is it worth doing the same here, to keep them aligned? Unless using the long-form "local+auto" version an intentional choice.
Good point. I manually tested it and setting `GOTOOLCHAIN=auto` is sufficient to achieve this.
Change to `auto`.
Fixes golang/go#64665Just checking, the original report linked to https://go.dev/doc/manage-install, but that page doesn't seem to mention gotip at all anymore; maybe that's because the manage-install documentation changed?
I don't think `gotip` was mentioned in the doc. The discussion evolved to `gotip` because that's the tool I used when reporting the issue :-)
Should we leave this open so we can fix for other go1.X?
I am thinking maybe the same solution works for other go versions.
On the other hand, after introduction of `GOTOOLCHAIN`, I am not sure if this manage-install tool adds much value any more.
Related details
- Dmitri Shuralyov
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |