Skip to content

Commit 7f418dc

Browse files
gnodetgnodet
committed
Fix system tool
When MVX_USE_SYSTEM_<TOOL>=true: * Checks if the tool is in PATH → uses it directly * If not in PATH but *_HOME is set → uses *_HOME/bin and adds it to PATH * If *_HOME is set but invalid → falls back to PATH detection * Always sets *_HOME environment variable for tools that need it (like Java for Maven)
1 parent 0f87adb commit 7f418dc

File tree

5 files changed

+143
-56
lines changed

5 files changed

+143
-56
lines changed

‎pkg/tools/base_tool.go‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -352,11 +352,29 @@ func (b *BaseTool) SetupHomeEnvironment(version string, cfg config.ToolConfig, e
352352
return nil
353353
}
354354

355+
// If binPath is empty, this might be a system tool already in PATH
356+
// Try to get the HOME directory from the system tool detection
357+
if binPath == "" && UseSystemTool(b.toolName) {
358+
homeEnvVar := getSystemToolHomeEnvVar(b.toolName)
359+
if homeEnvVar != "" && homeEnvVar == envVarName {
360+
// Try to get the system tool home directory
361+
if homeDir, _, err := getSystemToolHome(b.toolName, b.GetBinaryName(), homeEnvVar); err == nil {
362+
envVars[envVarName] = homeDir
363+
util.LogVerbose("Set %s=%s for system %s %s", envVarName, homeDir, b.toolName, version)
364+
return nil
365+
}
366+
}
367+
// If we can't determine HOME, that's okay - the tool might work from PATH alone
368+
return nil
369+
}
370+
355371
// *_HOME should point to the installation directory, not the bin directory
356-
if strings.HasSuffix(binPath, "/bin") {
357-
homeDir := strings.TrimSuffix(binPath, "/bin")
358-
envVars[envVarName] = homeDir
359-
util.LogVerbose("Set %s=%s for %s %s", envVarName, homeDir, b.toolName, version)
372+
if binPath != "" {
373+
if strings.HasSuffix(binPath, "/bin") || strings.HasSuffix(binPath, "\\bin") {
374+
homeDir := strings.TrimSuffix(strings.TrimSuffix(binPath, "/bin"), "\\bin")
375+
envVars[envVarName] = homeDir
376+
util.LogVerbose("Set %s=%s for %s %s", envVarName, homeDir, b.toolName, version)
377+
}
360378
}
361379

362380
return nil
@@ -644,11 +662,21 @@ func (b *BaseTool) ListInstalledVersions(distribution string) []InstalledVersion
644662
// StandardIsInstalled provides standard installation check for tools
645663
func (b *BaseTool) StandardIsInstalled(versionSpec string, cfg config.ToolConfig, getPath func(string, config.ToolConfig) (string, error)) bool {
646664
if UseSystemTool(b.toolName) {
665+
// Try primary binary name in PATH first
647666
if _, err := exec.LookPath(b.GetBinaryName()); err == nil {
648667
util.LogVerbose("System %s is available in PATH (MVX_USE_SYSTEM_%s=true)", b.toolName, strings.ToUpper(b.toolName))
649668
return true
650669
}
651670

671+
// If not in PATH, try to get HOME environment variable (which can derive from HOME env var or PATH)
672+
homeEnvVar := getSystemToolHomeEnvVar(b.toolName)
673+
if homeEnvVar != "" {
674+
if _, _, err := getSystemToolHome(b.toolName, b.GetBinaryName(), homeEnvVar); err == nil {
675+
util.LogVerbose("System %s is available via %s (MVX_USE_SYSTEM_%s=true)", b.toolName, homeEnvVar, strings.ToUpper(b.toolName))
676+
return true
677+
}
678+
}
679+
652680
util.LogVerbose("System %s not available: not found in environment variables or PATH", b.toolName)
653681
return false
654682
}
@@ -795,14 +823,26 @@ func (b *BaseTool) StandardGetPath(version string, cfg config.ToolConfig, getIns
795823
}
796824
// Check if we should use system tool instead of mvx-managed tool
797825
if UseSystemTool(b.toolName) {
798-
// Try primary binary name in PATH
826+
// Try to find system tool - first check if it's in PATH
799827
if _, err := exec.LookPath(b.GetBinaryName()); err == nil {
800828
util.LogVerbose("Using system %s from PATH (MVX_USE_SYSTEM_%s=true)", b.toolName, strings.ToUpper(b.toolName))
801829
b.setCachedPath(cacheKey, "", nil)
802830
return "", nil
803831
}
804832

805-
systemErr := SystemToolError(b.toolName, fmt.Errorf("MVX_USE_SYSTEM_%s=true but system %s not available", strings.ToUpper(b.toolName), b.toolName))
833+
// If not in PATH, try to get HOME environment variable and use its bin directory
834+
homeEnvVar := getSystemToolHomeEnvVar(b.toolName)
835+
if homeEnvVar != "" {
836+
_, binDir, err := getSystemToolHome(b.toolName, b.GetBinaryName(), homeEnvVar)
837+
if err == nil {
838+
util.LogVerbose("Using system %s from %s/bin (MVX_USE_SYSTEM_%s=true, adding to PATH)", b.toolName, homeEnvVar, strings.ToUpper(b.toolName))
839+
b.setCachedPath(cacheKey, binDir, nil)
840+
return binDir, nil
841+
}
842+
}
843+
844+
// Neither in PATH nor HOME available
845+
systemErr := SystemToolError(b.toolName, fmt.Errorf("MVX_USE_SYSTEM_%s=true but system %s not available (not in PATH and %s not set or invalid)", strings.ToUpper(b.toolName), b.toolName, homeEnvVar))
806846
b.setCachedPath(cacheKey, "", systemErr)
807847
return "", systemErr
808848
}

‎pkg/tools/java.go‎

Lines changed: 6 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -31,22 +31,6 @@ type DiscoDistribution struct {
3131
Available bool `json:"available"`
3232
}
3333

34-
// getSystemJavaHome returns the system JAVA_HOME if available and valid
35-
func getSystemJavaHome() (string, error) {
36-
javaHome := os.Getenv(EnvJavaHome)
37-
if javaHome == "" {
38-
return "", SystemToolError(ToolJava, fmt.Errorf("%s environment variable not set", EnvJavaHome))
39-
}
40-
41-
// Check if JAVA_HOME points to a valid Java installation
42-
javaExe := filepath.Join(javaHome, "bin", getJavaBinaryName())
43-
44-
if _, err := os.Stat(javaExe); err != nil {
45-
return "", SystemToolError(ToolJava, fmt.Errorf("Java executable not found at %s: %w", javaExe, err))
46-
}
47-
48-
return javaHome, nil
49-
}
5034

5135
// getSystemJavaVersion returns the version of the system Java installation
5236
func getSystemJavaVersion(javaHome string) (string, error) {
@@ -260,17 +244,6 @@ func (j *JavaTool) getDownloadURLWithChecksum(version, distribution string) (str
260244

261245
// IsInstalled checks if the specified version is installed
262246
func (j *JavaTool) IsInstalled(version string, cfg config.ToolConfig) bool {
263-
if UseSystemTool(j.toolName) {
264-
// Try primary binary name in PATH
265-
if _, err := exec.LookPath(j.GetBinaryName()); err == nil {
266-
util.LogVerbose("System %s is available in PATH (MVX_USE_SYSTEM_%s=true)", j.toolName, strings.ToUpper(j.toolName))
267-
return true
268-
}
269-
270-
util.LogVerbose("System %s not available: not found in environment variables or PATH", j.toolName)
271-
return false
272-
}
273-
274247
// Resolve the full version string
275248
distribution := cfg.Distribution
276249
if distribution == "" {
@@ -282,7 +255,7 @@ func (j *JavaTool) IsInstalled(version string, cfg config.ToolConfig) bool {
282255
return false
283256
}
284257

285-
// Use standardized installation check with Java-specific environment variables
258+
// Use standardized installation check (includes system tool detection)
286259
return j.StandardIsInstalled(fullVersion, cfg, j.GetPath)
287260
}
288261

@@ -312,8 +285,9 @@ func (j *JavaTool) getJavaHomeUncached(version string, cfg config.ToolConfig) (s
312285

313286
// If using system Java, return system JAVA_HOME if available (no version compatibility check)
314287
if UseSystemTool(ToolJava) {
315-
if systemJavaHome, err := getSystemJavaHome(); err == nil {
316-
util.LogVerbose("Using system Java from %s: %s (MVX_USE_SYSTEM_JAVA=true)", EnvJavaHome, systemJavaHome)
288+
homeEnvVar := getSystemToolHomeEnvVar(ToolJava)
289+
if systemJavaHome, _, err := getSystemToolHome(ToolJava, j.GetBinaryName(), homeEnvVar); err == nil {
290+
util.LogVerbose("Using system Java from %s: %s (MVX_USE_SYSTEM_JAVA=true)", homeEnvVar, systemJavaHome)
317291
return systemJavaHome, nil
318292
} else {
319293
return "", EnvironmentError(ToolJava, version, fmt.Errorf("MVX_USE_SYSTEM_JAVA=true but system Java not available: %w", err))
@@ -383,7 +357,7 @@ func (j *JavaTool) GetBinaryName() string {
383357

384358
// GetPath returns the binary path for the specified version (for PATH management)
385359
func (j *JavaTool) GetPath(version string, cfg config.ToolConfig) (string, error) {
386-
// Use standardized path resolution with Java-specific environment variables
360+
// Use standardized path resolution (includes system tool detection)
387361
return j.StandardGetPath(version, cfg, j.getInstalledPath)
388362
}
389363

@@ -496,7 +470,7 @@ func (j *JavaTool) GetDisplayName() string {
496470

497471
// SetupEnvironment sets up Java-specific environment variables (implements EnvironmentProvider)
498472
func (j *JavaTool) SetupEnvironment(version string, cfg config.ToolConfig, envManager *EnvironmentManager) error {
499-
// Convert EnvironmentManager to map for the existing helper
473+
// Use the standard SetupHomeEnvironment which works for both system and mvx-managed tools
500474
envVars := envManager.ToMap()
501475
err := j.SetupHomeEnvironment(version, cfg, envVars, EnvJavaHome, j.GetPath)
502476
// Update the environment manager with any changes

‎pkg/tools/java_test.go‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -78,18 +78,25 @@ func TestJavaSystemDetector(t *testing.T) {
7878
}
7979
}()
8080

81-
// Test when JAVA_HOME is not set
81+
// Test when JAVA_HOME is not set and java is not in PATH
8282
os.Unsetenv("JAVA_HOME")
83-
_, err := getSystemJavaHome()
84-
if err == nil {
85-
t.Error("getSystemJavaHome() should return error when JAVA_HOME is not set")
83+
// Note: This test might pass if java is in PATH, which is fine
84+
// We're testing that the generic function handles the case gracefully
85+
javaBinaryName := BinaryJava
86+
if runtime.GOOS == "windows" {
87+
javaBinaryName = BinaryJava + ".exe"
88+
}
89+
_, _, err := getSystemToolHome(ToolJava, javaBinaryName, EnvJavaHome)
90+
if err != nil {
91+
// This is expected if java is not in PATH and JAVA_HOME is not set
92+
t.Logf("getSystemToolHome() returned error as expected: %v", err)
8693
}
8794

8895
// Test when JAVA_HOME points to non-existent directory
8996
os.Setenv("JAVA_HOME", "/non/existent/path")
90-
_, err = getSystemJavaHome()
97+
_, _, err = getSystemToolHome(ToolJava, javaBinaryName, EnvJavaHome)
9198
if err == nil {
92-
t.Error("getSystemJavaHome() should return error when JAVA_HOME points to non-existent directory")
99+
t.Error("getSystemToolHome() should return error when JAVA_HOME points to non-existent directory")
93100
}
94101
}
95102

‎pkg/tools/manager.go‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -772,13 +772,6 @@ func (m *Manager) SetupEnvironment(cfg *config.Config) (map[string]string, error
772772

773773
// Add tool-specific environment variables and PATH entries
774774
for toolName, toolConfig := range cfg.Tools {
775-
// Check if user wants to use system tool instead
776-
systemEnvVar := fmt.Sprintf("MVX_USE_SYSTEM_%s", strings.ToUpper(toolName))
777-
if os.Getenv(systemEnvVar) == "true" {
778-
util.LogVerbose("Skipping %s environment setup: %s=true (using system tool)", toolName, systemEnvVar)
779-
continue
780-
}
781-
782775
// Resolve version
783776
resolvedVersion, err := m.resolveVersion(toolName, toolConfig)
784777
if err != nil {
@@ -789,7 +782,7 @@ func (m *Manager) SetupEnvironment(cfg *config.Config) (map[string]string, error
789782
resolvedConfig := toolConfig
790783
resolvedConfig.Version = resolvedVersion
791784

792-
// Check if installed (using cache)
785+
// Check if installed (using cache) - this works for both system and mvx-managed tools
793786
if !m.isToolInstalled(toolName, resolvedVersion, resolvedConfig) {
794787
util.LogVerbose("Skipping tool %s: not installed", toolName)
795788
continue // Skip uninstalled tools
@@ -802,18 +795,24 @@ func (m *Manager) SetupEnvironment(cfg *config.Config) (map[string]string, error
802795
continue
803796
}
804797

805-
// Get tool path and add to PATH
798+
// Get tool path and add to PATH (returns empty string for system tools already in PATH)
806799
toolPath, err := tool.GetPath(resolvedVersion, resolvedConfig)
807800
if err != nil {
808801
util.LogVerbose("Skipping tool %s: failed to get path: %v", toolName, err)
809802
continue
810803
}
811804

812-
// Add tool bin directory to PATH
813-
envManager.AddToPath(toolPath)
814-
util.LogVerbose("Added %s bin path to PATH: %s", toolName, toolPath)
805+
// Add tool bin directory to PATH (only if path is not empty)
806+
// For system tools already in PATH, toolPath will be empty and we skip adding it
807+
if toolPath != "" {
808+
envManager.AddToPath(toolPath)
809+
util.LogVerbose("Added %s bin path to PATH: %s", toolName, toolPath)
810+
} else {
811+
util.LogVerbose("System %s is in PATH, not adding to PATH", toolName)
812+
}
815813

816814
// Set tool-specific environment variables (HOME directories, etc.)
815+
// This is important for system tools too - they may need HOME variables set
817816
if envProvider, ok := tool.(EnvironmentProvider); ok {
818817
if err := envProvider.SetupEnvironment(resolvedVersion, resolvedConfig, envManager); err != nil {
819818
util.LogVerbose("Failed to setup environment for %s %s: %v", toolName, resolvedVersion, err)

‎pkg/tools/system.go‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,21 @@ import (
44
"fmt"
55
"os"
66
"os/exec"
7+
"path/filepath"
78
"strings"
9+
10+
"github.com/gnodet/mvx/pkg/util"
811
)
912

13+
// toolHomeEnvVars maps tool names to their HOME environment variable names
14+
var toolHomeEnvVars = map[string]string{
15+
ToolJava: EnvJavaHome,
16+
ToolMaven: EnvMavenHome,
17+
ToolMvnd: EnvMvndHome,
18+
ToolNode: EnvNodeHome,
19+
ToolGo: EnvGoRoot, // Go uses GOROOT instead of GO_HOME
20+
}
21+
1022
// UseSystemTool checks if a system tool should be used instead of downloading
1123
// by checking the MVX_USE_SYSTEM_<TOOL> environment variable
1224
func UseSystemTool(toolName string) bool {
@@ -19,6 +31,61 @@ func getSystemToolEnvVar(toolName string) string {
1931
return fmt.Sprintf("MVX_USE_SYSTEM_%s", strings.ToUpper(toolName))
2032
}
2133

34+
// getSystemToolHomeEnvVar returns the HOME environment variable name for a tool, if any
35+
func getSystemToolHomeEnvVar(toolName string) string {
36+
return toolHomeEnvVars[toolName]
37+
}
38+
39+
// getSystemToolHome returns the system tool home directory if available and valid
40+
// It first checks the HOME environment variable (e.g., JAVA_HOME, MAVEN_HOME), then falls back to finding the tool in PATH
41+
// Returns the home directory and bin directory if found, or an error if not available
42+
func getSystemToolHome(toolName, binaryName, homeEnvVar string) (homeDir string, binDir string, err error) {
43+
// First, try HOME environment variable if specified
44+
if homeEnvVar != "" {
45+
homeDir = os.Getenv(homeEnvVar)
46+
if homeDir != "" {
47+
// Check if HOME points to a valid installation
48+
binDirPath := filepath.Join(homeDir, "bin")
49+
binaryPath := filepath.Join(binDirPath, binaryName)
50+
if _, err := os.Stat(binaryPath); err == nil {
51+
util.LogVerbose("Found system %s via %s=%s", toolName, homeEnvVar, homeDir)
52+
return homeDir, binDirPath, nil
53+
}
54+
// HOME is set but doesn't point to a valid installation
55+
util.LogVerbose("%s is set to %s but %s executable not found at %s, trying PATH", homeEnvVar, homeDir, binaryName, binaryPath)
56+
}
57+
}
58+
59+
// Fallback: try to find tool in PATH and derive home directory from it
60+
binaryPath, err := exec.LookPath(binaryName)
61+
if err != nil {
62+
if homeEnvVar != "" {
63+
return "", "", SystemToolError(toolName, fmt.Errorf("%s not found: %s environment variable not set or invalid and %s not found in PATH", toolName, homeEnvVar, binaryName))
64+
}
65+
return "", "", SystemToolError(toolName, fmt.Errorf("%s not found in PATH", binaryName))
66+
}
67+
68+
// Resolve the absolute path (follow symlinks)
69+
absPath, err := filepath.EvalSymlinks(binaryPath)
70+
if err != nil {
71+
absPath = binaryPath
72+
}
73+
74+
// Derive home directory from binary path
75+
// Binary is typically at $HOME/bin/binary (or bin/binary.exe on Windows)
76+
binDirPath := filepath.Dir(absPath)
77+
if filepath.Base(binDirPath) != "bin" {
78+
// Some tools might be installed directly without a bin directory structure
79+
// In this case, we can't derive a home directory, but we can still use the binary
80+
util.LogVerbose("Found %s at %s but not in a 'bin' directory, using directory as-is", toolName, absPath)
81+
return filepath.Dir(binDirPath), binDirPath, nil
82+
}
83+
84+
homeDir = filepath.Dir(binDirPath)
85+
util.LogVerbose("Derived %s home directory=%s from binary at %s", toolName, homeDir, absPath)
86+
return homeDir, binDirPath, nil
87+
}
88+
2289
// SystemToolInfo contains information about a detected system tool
2390
type SystemToolInfo struct {
2491
Path string // Full path to the tool executable

0 commit comments

Comments
 (0)