windows fixes

Author: aknysh

docs/fixes/windows-atmos-d-and-toolchain-issues.md

@@ -4,13 +4,12 @@
 
 This document describes Windows-specific issues reported by users and the fixes applied.
 
-| Issue                              | Status             | Description                                      |
-|------------------------------------|--------------------|--------------------------------------------------|
-| `.atmos.d` auto-import             | ✅ Verified Working | Configuration loading works correctly on Windows |
-| Toolchain binary `.exe` extension  | ✅ Fixed            | Binaries now installed with `.exe` extension     |
-| Archive extraction `.exe` handling | ✅ Fixed            | Files extracted correctly from archives          |
-| Asset download URL `.exe` fallback | ✅ Fixed            | Downloads now try `.exe` suffix on Windows       |
-| PowerShell hint message            | ✅ Fixed            | Shows correct `Invoke-Expression` syntax         |
+| Issue                              | Status             | Description                                              |
+|------------------------------------|--------------------|----------------------------------------------------------|
+| `.atmos.d` auto-import             | ✅ Verified Working | Configuration loading works correctly on Windows         |
+| Toolchain binary `.exe` extension  | ✅ Fixed            | Centralized function ensures `.exe` extension on Windows |
+| Archive extraction `.exe` handling | ✅ Fixed            | Files extracted correctly from archives                  |
+| PowerShell hint message            | ✅ Fixed            | Shows correct `Invoke-Expression` syntax                 |
 
 ---
 
@@ -24,28 +23,53 @@ After testing on Windows, the `.atmos.d` auto-import functionality works correct
 
 Enhanced debug logging in `pkg/config/load.go`:
 
-- Logs at **Debug** level when directories are found
-- Users can diagnose issues with `ATMOS_LOGS_LEVEL=Debug`
+- Logs at **Debug** level when directories are found.
+- Users can diagnose issues with `ATMOS_LOGS_LEVEL=Debug`.
 
 ---
 
 ## Issue #2: Toolchain Installation Failures
 
 ### Reported Problems
 
-1. Binary installed without `.exe` extension - causes `terraform --version` to hang
-2. Archive extraction fails for tools like helm - looking for `windows-amd64/helm` instead of `windows-amd64/helm.exe`
-3. Asset download fails for tools like jq - URL missing `.exe` suffix
-4. Hint message shows Unix `eval` syntax instead of PowerShell `Invoke-Expression`
+1. Binary installed without `.exe` extension - causes `terraform --version` to hang.
+2. Archive extraction fails for tools like helm - looking for `windows-amd64/helm` instead of `windows-amd64/helm.exe`.
+3. Hint message shows Unix `eval` syntax instead of PowerShell `Invoke-Expression`.
+
+### Architecture: Centralized Windows Extension Handling
+
+Following [Aqua's Windows support approach](https://aquaproj.github.io/docs/reference/windows-support/), Windows executables need the `.exe` extension to be found by `os/exec.LookPath`. Rather than scattering `.exe` handling across multiple files, we use a single centralized function.
+
+**Key design decisions:**
+
+- **No download URL heuristics**: If a tool's download URL needs `.exe`, the registry should specify it correctly in the asset template. We don't append `.exe` to URLs as a fallback.
+- **Single utility function**: `EnsureWindowsExeExtension()` handles all Windows extension logic in one place.
+- **Registry-driven configuration**: Asset URLs and file names come from the registry definition, not runtime heuristics.
 
 ### Fixes Applied
 
-| File                               | Fix                                                |
-|------------------------------------|----------------------------------------------------|
-| `toolchain/installer/installer.go` | Append `.exe` to binary name on Windows            |
-| `toolchain/installer/extract.go`   | Try `.exe` extension when extracting from archives |
-| `toolchain/installer/download.go`  | Try `.exe` suffix in download URL on Windows       |
-| `toolchain/install_helpers.go`     | Platform-aware hint message                        |
+| File                               | Fix                                                          |
+|------------------------------------|--------------------------------------------------------------|
+| `toolchain/installer/installer.go` | Added `EnsureWindowsExeExtension()` centralized function     |
+| `toolchain/installer/installer.go` | Uses centralized function for binary naming                  |
+| `toolchain/installer/extract.go`   | Uses centralized function when extracting from archives      |
+| `toolchain/install_helpers.go`     | Platform-aware hint message for PowerShell                   |
+
+### Centralized Function
+
+```go
+// EnsureWindowsExeExtension appends .exe to the binary name on Windows if not present.
+// This follows Aqua's behavior where executables need the .exe extension on Windows
+// to be found by os/exec.LookPath.
+func EnsureWindowsExeExtension(binaryName string) string {
+    defer perf.Track(nil, "installer.EnsureWindowsExeExtension")()
+
+    if runtime.GOOS == "windows" && !strings.HasSuffix(strings.ToLower(binaryName), ".exe") {
+        return binaryName + ".exe"
+    }
+    return binaryName
+}
+```
 
 ---
 
@@ -63,6 +87,8 @@ go test ./toolchain/installer/... -v
 
 Test file: `tests/toolchain_custom_commands_test.go`
 
+Uses `testhelpers.NewAtmosRunner` for building and running atmos binary (shared infrastructure).
+
 | Test                                                  | Description                                                    |
 |-------------------------------------------------------|----------------------------------------------------------------|
 | `TestToolchainCustomCommands_InstallAllTools`         | Installs gum, k9s, helm, jq, tofu and verifies `.exe` binaries |

tests/toolchain_custom_commands_test.go

@@ -12,6 +12,8 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/cloudposse/atmos/pkg/perf"
+	"github.com/cloudposse/atmos/tests/testhelpers"
+	"github.com/cloudposse/atmos/toolchain/installer"
 )
 
 // TestToolchainCustomCommands_InstallAllTools verifies that all tools defined in
@@ -338,38 +340,36 @@ func TestToolchainCustomCommands_ExecuteWithDependencies(t *testing.T) {
 	}
 }
 
-// buildAtmosBinary builds the atmos binary and returns its path.
-// It builds to a temporary directory to avoid polluting the source tree.
+// getAtmosRunner returns a shared AtmosRunner for tests.
+// It uses a package-level runner to avoid rebuilding atmos for each test.
+var sharedRunner *testhelpers.AtmosRunner
+
+// buildAtmosBinary builds the atmos binary using the shared AtmosRunner and returns its path.
 func buildAtmosBinary(t *testing.T) string {
 	t.Helper()
 
-	// Get the repo root (4 levels up from fixtures/scenarios/toolchain-custom-commands).
-	repoRoot, err := filepath.Abs("../../../..")
-	require.NoError(t, err, "Failed to get repo root")
-
-	// Build to temp directory.
-	tmpDir := t.TempDir()
-	binaryName := "atmos"
-	if runtime.GOOS == "windows" {
-		binaryName = "atmos.exe"
+	if sharedRunner == nil {
+		sharedRunner = testhelpers.NewAtmosRunner("")
 	}
-	binaryPath := filepath.Join(tmpDir, binaryName)
 
-	// Build atmos.
-	cmd := exec.Command("go", "build", "-o", binaryPath, ".")
-	cmd.Dir = repoRoot
-	cmd.Env = append(os.Environ(), "CGO_ENABLED=0")
-	output, err := cmd.CombinedOutput()
-	require.NoError(t, err, "Failed to build atmos: %s", string(output))
+	err := sharedRunner.Build()
+	require.NoError(t, err, "Failed to build atmos")
 
-	return binaryPath
+	// Register cleanup only once.
+	t.Cleanup(func() {
+		if sharedRunner != nil {
+			sharedRunner.Cleanup()
+			sharedRunner = nil
+		}
+	})
+
+	return sharedRunner.BinaryPath()
 }
 
 // getBinaryPath constructs the expected binary path for an installed tool.
 func getBinaryPath(toolsDir, owner, repo, version, binaryName string) string {
-	if runtime.GOOS == "windows" && !strings.HasSuffix(binaryName, ".exe") {
-		binaryName += ".exe"
-	}
+	// Use the centralized function for Windows .exe extension handling.
+	binaryName = installer.EnsureWindowsExeExtension(binaryName)
 	return filepath.Join(toolsDir, "bin", owner, repo, version, binaryName)
 }
 

toolchain/exec_test.go

@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/cloudposse/atmos/pkg/schema"
+	"github.com/cloudposse/atmos/toolchain/installer"
 )
 
 // --- Fake Installer for testing ---.
@@ -88,7 +89,8 @@ func TestRunExecCommand_Success(t *testing.T) {
 		t.Fatalf("expected no error, got %v", err)
 	}
 
-	expected := filepath.Join(tempDir, ".tools", "bin", "hashicorp", "terraform", "1.13.1", "terraform")
+	binaryName := installer.EnsureWindowsExeExtension("terraform")
+	expected := filepath.Join(tempDir, ".tools", "bin", "hashicorp", "terraform", "1.13.1", binaryName)
 	if calledExecPath != expected {
 		t.Errorf("expected exec path %q, got %q", expected, calledExecPath)
 	}

toolchain/installer/download.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 
 	log "github.com/charmbracelet/log"
@@ -177,41 +176,9 @@ func (i *Installer) tryFallbackVersion(tool *registry.Tool, version, assetURL st
 		return assetPath, nil
 	}
 
-	// On Windows, try appending .exe to the asset URL as a last resort.
-	// Some tools (like jq) have Windows binaries with .exe suffix in the download URL.
-	if runtime.GOOS == "windows" {
-		return i.tryWindowsExeFallback(assetURL, fallbackURL)
-	}
-
 	return "", fmt.Errorf("%w: tried %s and %s: %w", ErrHTTPRequest, assetURL, fallbackURL, err)
 }
 
-// tryWindowsExeFallback attempts to download with .exe extension appended to the URL.
-// Some tools have Windows binaries named with .exe suffix (e.g., jq-windows-amd64.exe).
-func (i *Installer) tryWindowsExeFallback(assetURL, fallbackURL string) (string, error) {
-	defer perf.Track(nil, "Installer.tryWindowsExeFallback")()
-
-	// Try original URL with .exe.
-	exeURL := assetURL + ".exe"
-	log.Debug("Trying Windows .exe fallback", "url", exeURL)
-	assetPath, err := i.downloadAsset(exeURL)
-	if err == nil {
-		return assetPath, nil
-	}
-
-	// Try fallback URL with .exe.
-	if fallbackURL != "" && fallbackURL != assetURL {
-		fallbackExeURL := fallbackURL + ".exe"
-		log.Debug("Trying Windows .exe fallback for version fallback", "url", fallbackExeURL)
-		assetPath, err = i.downloadAsset(fallbackExeURL)
-		if err == nil {
-			return assetPath, nil
-		}
-	}
-
-	return "", fmt.Errorf("%w: tried %s and %s: %w", ErrHTTPRequest, assetURL, fallbackURL, ErrHTTP404)
-}
-
 // isHTTP404 returns true if the error is a 404 from downloadAsset.
 func isHTTP404(err error) bool {
 	return errors.Is(err, ErrHTTP404)

toolchain/installer/extract.go

@@ -247,9 +247,7 @@ func (i *Installer) extractFilesFromDir(tempDir, binaryPath string, tool *regist
 			}
 			src = srcWithExe
 			// Also update dst to include .exe if it doesn't already.
-			if runtime.GOOS == "windows" && !strings.HasSuffix(strings.ToLower(dst), ".exe") {
-				dst += ".exe"
-			}
+			dst = EnsureWindowsExeExtension(dst)
 		}
 
 		if err := MoveFile(src, dst); err != nil {

toolchain/installer/installer.go

@@ -38,8 +38,23 @@ const (
 	logFieldOwner   = "owner"
 	logFieldRepo    = "repo"
 	logFieldVersion = "version"
+
+	// Windows constants.
+	windowsExeExt = ".exe"
 )
 
+// EnsureWindowsExeExtension appends .exe to the binary name on Windows if not already present.
+// This follows Aqua's behavior where executables need the .exe extension on Windows
+// to be found by os/exec.LookPath.
+func EnsureWindowsExeExtension(binaryName string) string {
+	defer perf.Track(nil, "installer.EnsureWindowsExeExtension")()
+
+	if runtime.GOOS == "windows" && !strings.HasSuffix(strings.ToLower(binaryName), windowsExeExt) {
+		return binaryName + windowsExeExt
+	}
+	return binaryName
+}
+
 // ToolResolver defines an interface for resolving tool names to owner/repo pairs
 // This allows for mocking in tests and flexible resolution in production.
 type ToolResolver interface {
@@ -447,10 +462,8 @@ func (i *Installer) extractAndInstall(tool *registry.Tool, assetPath, version st
 	// Determine the binary name using shared resolution logic.
 	binaryName := resolveBinaryName(tool)
 
-	// On Windows, executables need the .exe extension to be found by the shell.
-	if runtime.GOOS == "windows" && !strings.HasSuffix(strings.ToLower(binaryName), ".exe") {
-		binaryName += ".exe"
-	}
+	// Ensure Windows executables have .exe extension.
+	binaryName = EnsureWindowsExeExtension(binaryName)
 
 	binaryPath := filepath.Join(versionDir, binaryName)