Skip to content

Commit 7226986

Browse files
ChronosMasterOfAllTimeChronosMasterOfAllTime
authored
V3: Fix doctor system go detection and goenv shimming (#532)
* Fix doctor system go detection and goenv shimming * fix test? * fix windows test * Windows test? * WINDOZE? * WINBLOWS * Fix Windows CI: use runtime.GOROOT() to find Go compiler Instead of relying on PATH (which may not be set up in CI), use runtime.GOROOT() to locate the Go compiler used to run the tests. This is more reliable across different CI environments.
1 parent 6867f40 commit 7226986

4 files changed

Lines changed: 232 additions & 29 deletions

File tree

‎cmd/diagnostics/doctor.go‎

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1145,18 +1145,44 @@ func checkProfileSourcingIssues(cfg *config.Config) checkResult {
11451145
func checkSystemGoVersion(cfg *config.Config, mgr *manager.Manager) checkResult {
11461146
// Check if current version is system
11471147
currentVersion, _, err := mgr.GetCurrentVersion()
1148-
if err != nil || currentVersion != manager.SystemVersion {
1149-
// Not using system Go, skip check
1148+
usingSystemGo := err == nil && currentVersion == manager.SystemVersion
1149+
1150+
// Check if system Go exists in PATH (outside goenv directories)
1151+
hasSystemGo := mgr.HasSystemGo()
1152+
1153+
if !usingSystemGo && !hasSystemGo {
1154+
// Not using system Go and no system Go found
1155+
return checkResult{
1156+
id: "system-go-version",
1157+
name: "System Go version",
1158+
status: StatusOK,
1159+
message: "No system Go installation detected (using goenv-managed version)",
1160+
}
1161+
}
1162+
1163+
if !usingSystemGo && hasSystemGo {
1164+
// System Go exists but not using it - provide informative message
1165+
systemVersion, err := mgr.GetSystemGoVersion()
1166+
if err != nil {
1167+
return checkResult{
1168+
id: "system-go-version",
1169+
name: "System Go version",
1170+
status: StatusOK,
1171+
message: "System Go detected in PATH but using goenv-managed version",
1172+
advice: "To switch to system Go: goenv use system --global",
1173+
}
1174+
}
11501175
return checkResult{
11511176
id: "system-go-version",
11521177
name: "System Go version",
11531178
status: StatusOK,
1154-
message: "Not using system Go (using goenv-managed version)",
1179+
message: fmt.Sprintf("System Go %s detected but not active (using goenv-managed version)", systemVersion),
1180+
advice: "To switch to system Go: goenv use system --global",
11551181
}
11561182
}
11571183

11581184
// Using system Go - check if it exists and get version
1159-
if !mgr.HasSystemGo() {
1185+
if !hasSystemGo {
11601186
return checkResult{
11611187
id: "system-go-version",
11621188
name: "System Go version",

‎cmd/diagnostics/doctor_test.go‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ import (
44
"bytes"
55
"context"
66
"os"
7+
"os/exec"
78
"path/filepath"
9+
"runtime"
810
"slices"
911
"strings"
1012
"testing"
@@ -1390,3 +1392,155 @@ func TestCheckObsoleteEnvVars(t *testing.T) {
13901392
})
13911393
}
13921394
}
1395+
1396+
func TestCheckSystemGoVersion(t *testing.T) {
1397+
tests := []struct {
1398+
name string
1399+
currentVersion string
1400+
hasSystemGo bool
1401+
systemVersion string
1402+
expectedStatus Status
1403+
shouldContain string
1404+
shouldAdvice string
1405+
}{
1406+
{
1407+
name: "Using goenv version, no system Go",
1408+
currentVersion: "1.23.2",
1409+
hasSystemGo: false,
1410+
expectedStatus: StatusOK,
1411+
shouldContain: "No system Go installation detected",
1412+
},
1413+
{
1414+
name: "Using goenv version, system Go exists",
1415+
currentVersion: "1.23.2",
1416+
hasSystemGo: true,
1417+
systemVersion: "1.26.2",
1418+
expectedStatus: StatusOK,
1419+
shouldContain: "System Go 1.26.2 detected but not active",
1420+
shouldAdvice: "goenv use system --global",
1421+
},
1422+
{
1423+
name: "Using system Go, it exists",
1424+
currentVersion: "system",
1425+
hasSystemGo: true,
1426+
systemVersion: "1.26.2",
1427+
expectedStatus: StatusOK,
1428+
shouldContain: "Using system Go 1.26.2",
1429+
},
1430+
{
1431+
name: "Using system Go, but not found",
1432+
currentVersion: "system",
1433+
hasSystemGo: false,
1434+
expectedStatus: StatusError,
1435+
shouldContain: "System Go is set as current version but not found",
1436+
},
1437+
}
1438+
1439+
for _, tt := range tests {
1440+
t.Run(tt.name, func(t *testing.T) {
1441+
// On Windows, find the real Go compiler BEFORE any environment setup
1442+
// This is needed to compile mock executables later
1443+
var realGoPath string
1444+
if utils.IsWindows() && tt.hasSystemGo {
1445+
// Use runtime.GOROOT() to find the Go installation, then construct path to compiler
1446+
goroot := runtime.GOROOT()
1447+
if goroot == "" {
1448+
t.Skipf("Skipping test: GOROOT not available (needed to compile mock executable)")
1449+
}
1450+
realGoPath = filepath.Join(goroot, "bin", "go.exe")
1451+
// Verify the compiler exists
1452+
if _, err := os.Stat(realGoPath); err != nil {
1453+
t.Skipf("Skipping test: Go compiler not found at %s: %v", realGoPath, err)
1454+
}
1455+
}
1456+
1457+
// Use cmdtest for proper environment isolation
1458+
testRoot, cleanup := cmdtest.SetupTestEnv(t)
1459+
defer cleanup()
1460+
1461+
cfg := &config.Config{Root: testRoot}
1462+
1463+
// Setup current version if not "system"
1464+
if tt.currentVersion != "" && tt.currentVersion != "system" {
1465+
cmdtest.CreateTestVersion(t, testRoot, tt.currentVersion)
1466+
1467+
// Create version file
1468+
versionFile := filepath.Join(testRoot, "version")
1469+
err := os.WriteFile(versionFile, []byte(tt.currentVersion), 0o644)
1470+
require.NoError(t, err)
1471+
} else if tt.currentVersion == "system" {
1472+
// Set GOENV_VERSION to system
1473+
t.Setenv(utils.GoenvEnvVarVersion.String(), "system")
1474+
}
1475+
1476+
// Setup system Go if needed
1477+
if tt.hasSystemGo {
1478+
systemGoDir := t.TempDir()
1479+
err := utils.EnsureDir(filepath.Join(systemGoDir, "bin"))
1480+
require.NoError(t, err)
1481+
1482+
// Create mock go binary
1483+
goBinary := filepath.Join(systemGoDir, "bin", "go")
1484+
1485+
if utils.IsWindows() {
1486+
// On Windows, create a real compiled executable to avoid exec.Command issues with .bat files
1487+
goBinary += ".exe"
1488+
1489+
// Create a temporary Go source file
1490+
sourceFile := filepath.Join(systemGoDir, "go.go")
1491+
sourceCode := `package main
1492+
import "fmt"
1493+
func main() {
1494+
fmt.Println("go version go` + tt.systemVersion + ` windows/amd64")
1495+
}
1496+
`
1497+
err = os.WriteFile(sourceFile, []byte(sourceCode), 0o644)
1498+
require.NoError(t, err)
1499+
1500+
// Compile it to create the executable using the saved real Go path
1501+
cmd := exec.Command(realGoPath, "build", "-o", goBinary, sourceFile)
1502+
output, err := cmd.CombinedOutput()
1503+
require.NoError(t, err, "Failed to compile mock go.exe: %s", output)
1504+
} else {
1505+
// Use /bin/sh (not bash-specific) and explicitly exit 0
1506+
versionScript := "#!/bin/sh\n" +
1507+
"echo \"go version go" + tt.systemVersion + " darwin/arm64\"\n" +
1508+
"exit 0\n"
1509+
err = os.WriteFile(goBinary, []byte(versionScript), 0o755)
1510+
require.NoError(t, err)
1511+
}
1512+
1513+
// Prepend system Go to PATH so it wins over any runner-installed Go
1514+
// On Windows, keep existing PATH so shell tools remain available
1515+
oldPath := os.Getenv(utils.EnvVarPath)
1516+
if utils.IsWindows() {
1517+
// Prepend but keep existing PATH for Windows system tools
1518+
t.Setenv(utils.EnvVarPath, filepath.Join(systemGoDir, "bin")+string(os.PathListSeparator)+oldPath)
1519+
} else {
1520+
// On Unix, only use mock directory to ensure hermetic test
1521+
t.Setenv(utils.EnvVarPath, filepath.Join(systemGoDir, "bin"))
1522+
}
1523+
} else {
1524+
// Explicitly set PATH to empty to ensure no system Go can be found
1525+
// This makes the "not found" test case work correctly on CI runners
1526+
t.Setenv(utils.EnvVarPath, "")
1527+
}
1528+
1529+
// Create manager
1530+
mgr := manager.NewManager(cfg, nil)
1531+
1532+
// Run check
1533+
result := checkSystemGoVersion(cfg, mgr)
1534+
1535+
// Verify result
1536+
assert.Equal(t, "system-go-version", result.id)
1537+
assert.Equal(t, "System Go version", result.name)
1538+
assert.Equal(t, tt.expectedStatus, result.status, "Status mismatch: %s", result.message)
1539+
assert.Contains(t, result.message, tt.shouldContain, "Message: %s", result.message)
1540+
1541+
if tt.shouldAdvice != "" {
1542+
assert.Contains(t, result.advice, tt.shouldAdvice, "Advice: %s", result.advice)
1543+
}
1544+
})
1545+
}
1546+
}

‎cmd/shell/init.go‎

Lines changed: 44 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -235,13 +235,20 @@ func renderInitScript(shell shellutil.ShellType, cfg *config.Config, noRehash bo
235235
builder.WriteString("if test -e $GOENV_RC_FILE\n")
236236
builder.WriteString(" source $GOENV_RC_FILE\n")
237237
builder.WriteString("end\n")
238-
builder.WriteString("if not contains $GOENV_ROOT/shims $PATH\n")
239-
builder.WriteString(" if test \"$GOENV_PATH_ORDER\" = \"back\"\n")
240-
builder.WriteString(" set -gx PATH $PATH $GOENV_ROOT/shims\n")
241-
builder.WriteString(" else\n")
242-
builder.WriteString(" set -gx PATH $GOENV_ROOT/shims $PATH\n")
238+
builder.WriteString("# Remove shims from PATH if present (to re-add in correct position)\n")
239+
builder.WriteString("set -l new_path\n")
240+
builder.WriteString("for p in $PATH\n")
241+
builder.WriteString(" if test \"$p\" != \"$GOENV_ROOT/shims\"\n")
242+
builder.WriteString(" set new_path $new_path $p\n")
243243
builder.WriteString(" end\n")
244244
builder.WriteString("end\n")
245+
builder.WriteString("set -gx PATH $new_path\n")
246+
builder.WriteString("# Add shims directory to PATH in correct position\n")
247+
builder.WriteString("if test \"$GOENV_PATH_ORDER\" = \"back\"\n")
248+
builder.WriteString(" set -gx PATH $PATH $GOENV_ROOT/shims\n")
249+
builder.WriteString("else\n")
250+
builder.WriteString(" set -gx PATH $GOENV_ROOT/shims $PATH\n")
251+
builder.WriteString("end\n")
245252
case shellutil.ShellTypePowerShell:
246253
fmt.Fprintf(&builder, "$env:GOENV_SHELL = \"%s\"\n", shell)
247254
fmt.Fprintf(&builder, "$env:GOENV_ROOT = \"%s\"\n", cfg.Root)
@@ -252,12 +259,13 @@ func renderInitScript(shell shellutil.ShellType, cfg *config.Config, noRehash bo
252259
builder.WriteString(" . $env:GOENV_RC_FILE\n")
253260
builder.WriteString("}\n")
254261
shimsDir := filepath.Join(cfg.Root, "shims")
255-
builder.WriteString(fmt.Sprintf("if ($env:PATH -notlike '*%s*') {\n", shimsDir))
256-
builder.WriteString(" if ($env:GOENV_PATH_ORDER -eq 'back') {\n")
257-
builder.WriteString(fmt.Sprintf(" $env:PATH = \"$env:PATH;%s\"\n", shimsDir))
258-
builder.WriteString(" } else {\n")
259-
builder.WriteString(fmt.Sprintf(" $env:PATH = \"%s;$env:PATH\"\n", shimsDir))
260-
builder.WriteString(" }\n")
262+
builder.WriteString("# Remove shims from PATH if present (to re-add in correct position)\n")
263+
builder.WriteString(fmt.Sprintf("$env:PATH = ($env:PATH -split ';' | Where-Object { $_ -ne '%s' }) -join ';'\n", shimsDir))
264+
builder.WriteString("# Add shims directory to PATH in correct position\n")
265+
builder.WriteString("if ($env:GOENV_PATH_ORDER -eq 'back') {\n")
266+
builder.WriteString(fmt.Sprintf(" $env:PATH = \"$env:PATH;%s\"\n", shimsDir))
267+
builder.WriteString("} else {\n")
268+
builder.WriteString(fmt.Sprintf(" $env:PATH = \"%s;$env:PATH\"\n", shimsDir))
261269
builder.WriteString("}\n")
262270
case shellutil.ShellTypeCmd:
263271
fmt.Fprintf(&builder, "SET GOENV_SHELL=%s\n", shell)
@@ -269,12 +277,14 @@ func renderInitScript(shell shellutil.ShellType, cfg *config.Config, noRehash bo
269277
builder.WriteString(" CALL \"%GOENV_RC_FILE%\"\n")
270278
builder.WriteString(")\n")
271279
shimsDir := filepath.Join(cfg.Root, "shims")
272-
builder.WriteString(fmt.Sprintf("IF \"%%PATH:%s=%%\" == \"%%PATH%%\" (\n", shimsDir))
273-
builder.WriteString(" IF \"%GOENV_PATH_ORDER%\" == \"back\" (\n")
274-
builder.WriteString(fmt.Sprintf(" SET PATH=%%PATH%%;%s\n", shimsDir))
275-
builder.WriteString(" ) ELSE (\n")
276-
builder.WriteString(fmt.Sprintf(" SET PATH=%s;%%PATH%%\n", shimsDir))
277-
builder.WriteString(" )\n")
280+
builder.WriteString("REM Remove shims from PATH if present (to re-add in correct position)\n")
281+
builder.WriteString(fmt.Sprintf("CALL SET PATH=%%PATH:%s;=%%\n", shimsDir))
282+
builder.WriteString(fmt.Sprintf("CALL SET PATH=%%PATH:;%s=%%\n", shimsDir))
283+
builder.WriteString("REM Add shims directory to PATH in correct position\n")
284+
builder.WriteString("IF \"%GOENV_PATH_ORDER%\" == \"back\" (\n")
285+
builder.WriteString(fmt.Sprintf(" SET PATH=%%PATH%%;%s\n", shimsDir))
286+
builder.WriteString(") ELSE (\n")
287+
builder.WriteString(fmt.Sprintf(" SET PATH=%s;%%PATH%%\n", shimsDir))
278288
builder.WriteString(")\n")
279289
default:
280290
fmt.Fprintf(&builder, "export GOENV_SHELL=%s\n", shell)
@@ -298,12 +308,23 @@ func renderInitScript(shell shellutil.ShellType, cfg *config.Config, noRehash bo
298308
builder.WriteString("done <<< \"$PATH\"\n")
299309
builder.WriteString("export PATH=\"$_NEW_PATH\"\n")
300310
builder.WriteString("unset _NEW_PATH\n")
301-
builder.WriteString("if [ \"${PATH#*$GOENV_ROOT/shims}\" = \"${PATH}\" ]; then\n")
302-
builder.WriteString(" if [ \"${GOENV_PATH_ORDER:-front}\" = \"back\" ] ; then\n")
303-
builder.WriteString(" export PATH=\"${PATH}:${GOENV_ROOT}/shims\"\n")
304-
builder.WriteString(" else\n")
305-
builder.WriteString(" export PATH=\"${GOENV_ROOT}/shims:${PATH}\"\n")
306-
builder.WriteString(" fi\n")
311+
builder.WriteString("# Remove shims from PATH if present (to re-add in correct position)\n")
312+
builder.WriteString("_NEW_PATH=\"\"\n")
313+
builder.WriteString("while IFS=: read -ra PATHARRAY; do\n")
314+
builder.WriteString(" for i in \"${PATHARRAY[@]}\"; do\n")
315+
builder.WriteString(" case \"$i\" in\n")
316+
builder.WriteString(" \"$GOENV_ROOT\"/shims) ;;\n")
317+
builder.WriteString(" *) _NEW_PATH=\"${_NEW_PATH}${_NEW_PATH:+:}$i\" ;;\n")
318+
builder.WriteString(" esac\n")
319+
builder.WriteString(" done\n")
320+
builder.WriteString("done <<< \"$PATH\"\n")
321+
builder.WriteString("export PATH=\"$_NEW_PATH\"\n")
322+
builder.WriteString("unset _NEW_PATH\n")
323+
builder.WriteString("# Add shims directory to PATH in correct position\n")
324+
builder.WriteString("if [ \"${GOENV_PATH_ORDER:-front}\" = \"back\" ] ; then\n")
325+
builder.WriteString(" export PATH=\"${PATH}:${GOENV_ROOT}/shims\"\n")
326+
builder.WriteString("else\n")
327+
builder.WriteString(" export PATH=\"${GOENV_ROOT}/shims:${PATH}\"\n")
307328
builder.WriteString("fi\n")
308329
}
309330

‎cmd/shell/init_test.go‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,8 @@ func TestInitCommand(t *testing.T) {
112112
"GOENV_RC_FILE=\"${HOME}/.goenvrc\"",
113113
"if [ -e \"${GOENV_RC_FILE:-}\" ]; then",
114114
"source \"${GOENV_RC_FILE}\"",
115-
"if [ \"${PATH#*$GOENV_ROOT/shims}\" = \"${PATH}\" ]; then",
115+
"# Remove shims from PATH if present (to re-add in correct position)",
116+
"# Add shims directory to PATH in correct position",
116117
"export PATH=\"${GOENV_ROOT}/shims:${PATH}\"",
117118
"export PATH=\"${PATH}:${GOENV_ROOT}/shims\"",
118119
"command goenv sh-rehash --only-manage-paths 2>/dev/null",
@@ -145,7 +146,8 @@ func TestInitCommand(t *testing.T) {
145146
"set GOENV_RC_FILE $HOME/.goenvrc",
146147
"if test -e $GOENV_RC_FILE",
147148
"source $GOENV_RC_FILE",
148-
"if not contains $GOENV_ROOT/shims $PATH",
149+
"# Remove shims from PATH if present (to re-add in correct position)",
150+
"# Add shims directory to PATH in correct position",
149151
"set -gx PATH $GOENV_ROOT/shims $PATH",
150152
"command goenv sh-rehash --only-manage-paths 2>/dev/null",
151153
"function goenv",

0 commit comments

Comments
 (0)