fix(driver-podman): avoid panic when HOME is unset on macOS#2327
fix(driver-podman): avoid panic when HOME is unset on macOS#2327mesutoezdil wants to merge 4 commits into
Conversation
|
This is missing test coverage; would you add a test for it? Also, would you mind expanding on this use case?
|
f9b5e52 to
1c6948e
Compare
|
I think there's a better solution here, where we can fix this issue and also fix a latent bug, which is that presently the gateway can auto-detect Podman by probing known Podman sockets, but both the podman and vm drivers could fail to take advantage of the auto-detected socket because the drivers only check this Basically, we should align the Podman socket selection with my recent change to the Docker socket selection (cf4decc). That change enhanced Docker socket selection but also made sure the client used the selected/auto-detected socket when no socket was defined in configuration. It's a bigger change, but I think it's much more valuable than changing this panic behavior. The path I think we should take is to make Podman mirror Docker’s configuration model:
It should probe podman_socket_candidates() and return the first responsive Podman socket. Then implement is_podman_available() using it.
Its default becomes None, matching DockerComputeConfig. An explicit TOML, CLI, or environment value becomes Some(path).
(this removes the default because Podman has no default, really; the installation mechanism determines the default) That gives the same precedence as Docker, but without the fallback:
The resulting symmetry is: That centralizes discovery in openshell-core and leaves each driver responsible only for constructing its client from the resolved socket. If you want to take on this change, feel free, otherwise let me know and I can take it. An added bonus would look for |
Align Podman socket selection with the existing Docker model: explicit config wins, otherwise probe openshell-core for a responsive socket. This also fixes the original HOME-unset panic, since resolution no longer hardcodes a per-OS default path. - add detect_podman_socket() in openshell-core, mirroring detect_docker_socket - PodmanComputeConfig.socket_path is now Option<PathBuf>, no default - remove default_socket_path() (podman driver) and podman_socket_path() (vm driver), both replaced by the shared detector - update server env override and CLI for the new Option type - add tests: responsive-candidate detection in openshell-core, and config-error (not panic) when no socket is configured or reachable
1c6948e to
919e818
Compare
|
added |
|
Additional findings from
|
Extract socket resolution into resolve_socket_path, taking the detector as a parameter so tests do not depend on real env vars or the host's actual Podman state. Replace the flaky env-mutating test with three deterministic cases: explicit wins, detected is used when absent, and neither source errors. Fix the socket_path doc comment to describe it from a config user's point of view, matching DockerComputeConfig's docstring. Drop a comment that only made sense next to the Docker driver code. Update the Podman README and gateway docs: they described a fixed per-OS default path that no longer exists, replace with the actual probe-then-fail behavior.
all implemented |
|
/ok to test 39d8265 |
|
Final nit from The README’s default description is self-contradictory and incomplete. It says “auto-detected if unset” but then says the detector probes “this variable itself.” If the variable is unset, it Simpler and more durable wording would be:
|
Previous wording was self-contradictory (says auto-detect on unset, then lists the same var as a probed candidate) and omitted the Linux /run/user/uid/podman/podman.sock candidate.
hm yeah correct, done |
|
/ok to test 7e1ef86 |
|
Label |


default_socket_path used expect("HOME must be set on macOS"), which panics if HOME is not set (for example under minimal launchd/service contexts).
This falls back to the passwd entry for the current user (same approach as the dirs/home crates) instead of panicking. Only panics if neither HOME nor a passwd entry is available, which should not happen in practice.
No new dependency, nix is already a workspace dependency and already used this way elsewhere in the codebase.