Update how binaries are detected by macadam #263
Conversation
WalkthroughThe changes refactor binary discovery from config-based to environment-based lookup via a new Changes
Sequence DiagramsequenceDiagram
participant checkBinaryArg
participant findBinary
participant findBindir
participant ENV as Environment
participant FS as Filesystem
checkBinaryArg->>findBinary: findBinary(name)
alt CONTAINERS_HELPER_BINARY_DIR set
findBinary->>ENV: getenv("CONTAINERS_HELPER_BINARY_DIR")
ENV-->>findBinary: override_dir
findBinary->>FS: search override_dir first
end
findBinary->>findBindir: findBindir()
findBindir->>FS: readlink(executable) + filepath.Dir
findBindir-->>findBinary: $BINDIR resolved
findBinary->>FS: search getBinariesDirs() (platform-specific)
alt Binary found
FS-->>findBinary: binary_path
findBinary-->>checkBinaryArg: path
else Not found
FS-->>findBinary: error
findBinary-->>checkBinaryArg: error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes involve refactored binary discovery logic ( Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (2)pkg/preflights/preflights_other.go (1)
pkg/preflights/preflights_windows.go (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/preflights/preflights.go (2)
30-46: Harden external call with a timeout.
exec.Command(..., "--help")can hang; use a short timeout to avoid blocking preflights.@@ -import ( +import ( + "context" @@ - cmd := exec.Command(binary, "--help") - out, err := cmd.CombinedOutput() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, binary, "--help") + out, err := cmd.CombinedOutput() if err != nil { - slog.Error("failed to run binary", "path", binary, "error", err) + // keep going: many CLIs return non‑zero for --help + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("%s --help timed out after 5s", binary) + } + slog.Error("failed to run binary", "path", binary, "error", err) }
48-67: Improve not-found UX and (optionally) allow PATH fallback.
- Current error lists dirs as a Go slice; suggest a more user-friendly message with a clear remediation hint.
- Optional: behind an env toggle, fall back to PATH for dev setups without reintroducing containers.conf semantics.
@@ -import ( +import ( @@ - "path/filepath" + "path/filepath" + "strings" @@ func findBinary(name string) (string, error) { @@ for _, path := range dirList { @@ } - return "", fmt.Errorf("could not find %q in one of %v", name, dirList) + // Optional PATH fallback when explicitly enabled + if v := os.Getenv("MACADAM_FIND_BINARY_USE_PATH"); strings.EqualFold(v, "1") || strings.EqualFold(v, "true") { + if lp, err := exec.LookPath(name); err == nil { + return lp, nil + } + } + + return "", fmt.Errorf( + "could not find %q in: %s. Set CONTAINERS_HELPER_BINARY_DIR=/path/to/dir to override.", + name, strings.Join(dirList, ", "), + )If you enable the PATH fallback, please confirm no tests depend on the stricter behavior.
pkg/preflights/preflights_unix.go (1)
21-25: Clarify platform in comment.This vfkit note applies to darwin; on Linux the stub returns nil. Tweak comment to avoid confusion.
-// macadam/podman needs a vfkit binary which supports the --cloud-init -// argument to inject ssh keys in RHEL cloud images +// darwin-only: vfkit must support --cloud-init to inject SSH keys in cloud images. +// On non-darwin builds this check is a no-op.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)pkg/preflights/preflights.go(3 hunks)pkg/preflights/preflights_darwin.go(1 hunks)pkg/preflights/preflights_unix.go(1 hunks)pkg/preflights/preflights_windows.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pkg/preflights/preflights_darwin.go (1)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
VMProvider(63-85)
pkg/preflights/preflights_windows.go (1)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
VMProvider(63-85)
pkg/preflights/preflights_unix.go (1)
vendor/github.com/containers/podman/v5/pkg/machine/vmconfigs/config.go (1)
VMProvider(63-85)
🔇 Additional comments (10)
pkg/preflights/preflights_unix.go (2)
12-19: Gvproxy capability check — LGTM.Minimal and clear; message guides users to v0.8.3+. No containers.conf references.
31-41: Approve Linux helper binary dirs in preflights_unix.go
Dirs align with containers/common; pkg/preflights contains no containers.conf or FindHelperBinary references. Revisit if containers/common updates.Makefile (1)
39-39: Approve changes and confirm cleanup: No stale uses of MACADAM_LDFLAGS or HELPER_BINARIES_DIR remain in the repository.pkg/preflights/preflights_darwin.go (5)
1-11: LGTM! Build tag and imports are correct.The darwin build tag properly gates this code to macOS, and the imports are minimal and appropriate for the functionality.
22-32: LGTM! Conditional check for AppleHvVirt is appropriate.The function correctly guards vfkit validation by checking the VM provider type, only validating when AppleHvVirt is in use. The error message provides clear version guidance.
34-42: LGTM! LibKrun conditional check is appropriate.The function correctly guards krunkit validation by checking the VM provider type, only validating when LibKrun is in use. The error message provides clear installation guidance.
13-20: No changes needed:checkBinaryArgcorrectly invokes the binary’s help output, checks for the-servicesflag, and the wrapped error messages are clear and free of podman-specific references.
44-63: $BINDIR placeholder is expanded by containers/common — no action required.containers/common implements $BINDIR expansion (bindirPrefix = "$BINDIR"; findBindir uses os.Executable() + filepath.EvalSymlinks and replaces the prefix when scanning dirs). See vendor/github.com/containers/common/pkg/config/config.go (findBindir + replacement logic, ~lines 1059–1072 and 1088–1103) and vendor/github.com/containers/common/pkg/config/config_darwin.go (contains "$BINDIR/../libexec/podman").
pkg/preflights/preflights_windows.go (2)
1-7: LGTM! Build tag and imports are correct.The windows build tag properly gates this code to Windows, and the minimal import is appropriate.
21-28: LGTM! Windows binary path is correctly specified.The function returns the standard Windows Podman installation path, properly escaped with double backslashes. The comment references the upstream source, which helps maintain consistency.
| func checkGvproxyVersion(provider vmconfigs.VMProvider) error { | ||
| return nil | ||
| } | ||
|
|
||
| func checkVfkitVersion(provider vmconfigs.VMProvider) error { | ||
| return nil | ||
| } | ||
|
|
||
| func checkKrunKitAvailability(provider vmconfigs.VMProvider) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify whether Windows requires these binary checks.
All three preflight checks are stubbed to return nil, which means:
- No gvproxy version validation on Windows
- No vfkit version validation on Windows (likely correct, as vfkit is macOS-specific)
- No krunkit availability check on Windows (likely correct, as krunkit is Linux/macOS-specific)
Confirm whether gvproxy validation is intentionally skipped on Windows or if it should be implemented similarly to the Darwin version.
🏁 Script executed:
#!/bin/bash
# Description: Verify if gvproxy is used on Windows and whether version checks are needed
# Expected: Find gvproxy usage patterns on Windows to determine if validation is required
# Check for Windows-specific gvproxy usage
rg -n -C5 'gvproxy|ForwarderBinaryName' --type=go
# Look for Windows-specific machine/provider implementations
fd -e go -g '*windows*.go' --exec rg -l 'gvproxy|StartNetworking'
# Check if there are any Windows VM providers that require these binaries
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) VMType() define.VMType {
$$$
}'Length of output: 76188
Implement Windows gvproxy version validation
In pkg/preflights/preflights_windows.go, replace the stubbed checkGvproxyVersion with a call to checkBinaryArg(machine.ForwarderBinaryName, "-services") (as done in preflights_unix.go/preflights_darwin.go), returning an error instructing users to upgrade to gvproxy v0.8.3 or newer.
🤖 Prompt for AI Agents
In pkg/preflights/preflights_windows.go around lines 9 to 19, the stubbed
checkGvproxyVersion should be implemented; replace the stub with a call to
checkBinaryArg(machine.ForwarderBinaryName, "-services") (matching the
implementations in preflights_unix.go/preflights_darwin.go), return any error
from checkBinaryArg wrapped or converted into a clear error message telling the
user to upgrade to gvproxy v0.8.3 or newer if the check fails, and ensure
imports/types used in that call are available in this file.
| // macadam/podman needs a gvproxy version which supports the --services | ||
| // argument | ||
| func checkGvproxyVersion(provider vmconfigs.VMProvider) error { | ||
| if err := checkBinaryArg(machine.ForwarderBinaryName, "-services"); err != nil { |
There was a problem hiding this comment.
There’s a slight inconsistency between -services here and --services in the comment. And this is actually no longer required since the switch to 5.6 even if this will be needed again in some form when crc starts using macadam.
There was a problem hiding this comment.
I didn't get if you wanted it removed or keep it as it could be useful for crc. I just fixed the comment for the moment.
There was a problem hiding this comment.
Given the ongoing convos around crc using macadam, we can keep it for now, even though I think podman no longer adds the flag.
| // We use the same to detect the binaries | ||
| return []string{ | ||
| // Relative to the binary directory | ||
| "$BINDIR/../libexec/podman", |
There was a problem hiding this comment.
Do we still have the code to resolve $BINDIR to the binary directory? This seems to be handled by FindHelperBinary so I guess not. This is needed when macadam is installed by the installer
There was a problem hiding this comment.
Nice catch, I missed the resolve code. Added a simplified version as we only check the BINDIR once and there is no need to cache its value
in following commit macadam will implement a custom func to detect a binary on some specific directories. By splitting preflights it makes the code cleaner and easier to maintain as each platform works differently and also require different binaries Signed-off-by: lstocchi <[email protected]>
this patch adds a custom findBinary func to stop relying on the upstream config.FindHelperBinary which uses entries from the containers.conf file. Macadam does not use that file. This work is to simplify how the search works but still maintaining compatibility with the locations used by podman to reuse its gvproxy, vfkit, libkrun binaries. Signed-off-by: lstocchi <[email protected]>
so far to force macadam to use its own "installed" gvproxy/vfkit/libkrun we forced podman to look at their paths by setting the variable config.additionalHelperBinariesDir. With our own findBinary implementation this is not needed anymore as we search directly the path /opt/macadam/bin as first option Signed-off-by: Luca Stocchi <[email protected]>
2 participants
This patch updates the way macadam detect binaries to stop using the upstream config.FindHelperBinary which uses entries from the containers.conf.
Macadam does not use that file.
This work is to simplify how the search is done but still maintain compatibility with the locations used by podman to re-use its binaries like gvproxy.
This also cleans up the error message displayed if one required binary is not available. It resolves #153
Summary by CodeRabbit
New Features
CONTAINERS_HELPER_BINARY_DIRenvironment variable to customize helper binary search paths.Chores