Skip to content

Commit a3f7460

Browse files
tbitcsoz-agent
andcommitted
fix: new config format now activates container execution (#14, #15)
Root cause: use_container was only checked via legacy env_type=='container', but the new config format (env.backend / env.workspace_mode) never sets env_type, so container execution was never activated. Changes: - Add EnvConfig.wants_container property that bridges legacy and new config formats. Returns True when env_type=='container' (legacy), backend is explicitly set (new), or workspace_mode requires a container volume (sync/copy/tmpfs). - Replace all env_type=='container' checks in env.py with cfg.wants_container (do_run, _doctor, _benchmark). - Add robust error handling to _sync: validates engine is in PATH and responsive before attempting container operations. Wraps docker cp/volume operations in try/except with actionable error messages instead of silent crashes. - Add 6 unit tests for wants_container covering legacy, new format, explicit backend, sync mode, bind mode, and empty config. Fixes #14 — west env sync no longer crashes silently Fixes #15 — west env build now uses container when new config format used Co-Authored-By: Oz <oz-agent@warp.dev>
1 parent 4edfa4e commit a3f7460

3 files changed

Lines changed: 117 additions & 6 deletions

File tree

tests/test_config.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,5 +100,48 @@ def test_load_config_handles_empty_file(self):
100100
self.assertIsNone(cfg.image)
101101

102102

103+
def test_wants_container_legacy_container(self):
104+
"""Legacy env.type=container should set wants_container=True."""
105+
from west_env.config import EnvConfig
106+
107+
cfg = EnvConfig({"env": {"type": "container", "container": {"engine": "docker"}}})
108+
self.assertTrue(cfg.wants_container)
109+
110+
def test_wants_container_legacy_native(self):
111+
"""Legacy env.type=native with no backend should set wants_container=False."""
112+
from west_env.config import EnvConfig
113+
114+
cfg = EnvConfig({"env": {"type": "native"}})
115+
self.assertFalse(cfg.wants_container)
116+
117+
def test_wants_container_new_format_explicit_backend(self):
118+
"""New format with explicit backend should set wants_container=True."""
119+
from west_env.config import EnvConfig
120+
121+
cfg = EnvConfig({"env": {"backend": "docker-desktop"}})
122+
self.assertTrue(cfg.wants_container)
123+
124+
def test_wants_container_new_format_sync_mode(self):
125+
"""New format with workspace_mode=sync implies container execution."""
126+
from west_env.config import EnvConfig
127+
128+
cfg = EnvConfig({"env": {"backend": "auto", "workspace_mode": "sync"}})
129+
self.assertTrue(cfg.wants_container)
130+
131+
def test_wants_container_new_format_bind_mode_auto(self):
132+
"""New format with workspace_mode=bind and auto backend is native."""
133+
from west_env.config import EnvConfig
134+
135+
cfg = EnvConfig({"env": {"backend": "auto", "workspace_mode": "bind"}})
136+
self.assertFalse(cfg.wants_container)
137+
138+
def test_wants_container_empty_config(self):
139+
"""Empty config should not want container."""
140+
from west_env.config import EnvConfig
141+
142+
cfg = EnvConfig({})
143+
self.assertFalse(cfg.wants_container)
144+
145+
103146
if __name__ == "__main__":
104147
unittest.main()

west_commands/env.py

Lines changed: 54 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ def do_add_parser(self, parser_adder):
152152

153153
def do_run(self, args, unknown_args):
154154
cfg = load_config(self.topdir)
155-
use_container = args.container or cfg.env_type == "container"
155+
use_container = args.container or cfg.wants_container
156156
passthrough = [a for a in args.args if a not in ("--container",)]
157157
passthrough.extend(a for a in unknown_args if a not in ("--container",))
158158

@@ -270,12 +270,15 @@ def _doctor(self, cfg, use_container):
270270
except Exception:
271271
pass
272272

273-
# Legacy container checks (kept for test compatibility)
273+
# Container checks
274274
if use_container:
275275
ok &= check_container(cfg)
276276
ok &= self._doctor_container_workspace(cfg)
277277
else:
278-
print("\n[INFO] container execution disabled")
278+
if cfg.wants_container:
279+
print("\n[INFO] container execution enabled (new config format)")
280+
else:
281+
print("\n[INFO] container execution disabled")
279282

280283
print()
281284
if ok:
@@ -300,22 +303,67 @@ def _doctor_container_workspace(self, cfg):
300303

301304
def _sync(self, cfg, mode, back=False):
302305
from west_env.sync import WorkspaceSync, _workspace_slug
306+
import subprocess
307+
from shutil import which
303308

304309
topdir = Path(self.topdir).resolve()
305310
ws = WorkspaceSync(workspace_mode=mode)
306311
engine_name = self._engine_name(cfg)
307312
volume = f"west-env-ws-{_workspace_slug(topdir)}"
308313

314+
# Validate engine is available before attempting any container ops
315+
if not which(engine_name):
316+
raise SystemExit(
317+
f"FATAL: '{engine_name}' not found in PATH.\n"
318+
f" Install Docker or Podman, or check your PATH.\n"
319+
f" Run 'west env doctor' for diagnostics."
320+
)
321+
322+
# Verify engine is responsive
323+
try:
324+
subprocess.check_output(
325+
[engine_name, "info"],
326+
stderr=subprocess.DEVNULL,
327+
timeout=15,
328+
)
329+
except subprocess.TimeoutExpired:
330+
raise SystemExit(
331+
f"FATAL: '{engine_name} info' timed out.\n"
332+
f" Is the Docker/Podman daemon running?"
333+
)
334+
except (subprocess.CalledProcessError, FileNotFoundError) as exc:
335+
raise SystemExit(
336+
f"FATAL: '{engine_name} info' failed: {exc}\n"
337+
f" Is the Docker/Podman daemon running?\n"
338+
f" Run 'west env doctor' for diagnostics."
339+
)
340+
309341
if back:
310342
print(f"Syncing artifacts back from container (mode={mode})...")
311343
artifacts_dir = topdir / "artifacts"
312-
ws.sync_from_volume(engine_name, volume, artifacts_dir)
344+
try:
345+
ws.sync_from_volume(engine_name, volume, artifacts_dir)
346+
except subprocess.CalledProcessError as exc:
347+
raise SystemExit(
348+
f"FATAL: artifact sync failed (exit {exc.returncode}).\n"
349+
f" Volume: {volume}\n"
350+
f" Ensure you have run 'west env sync' and built at least once."
351+
)
313352
print(f"[OK] artifacts written to {artifacts_dir}")
314353
else:
315354
print(f"Syncing source to container (mode={mode})...")
316355
ws.warn_if_needed()
317356
if mode in ("sync", "copy", "tmpfs"):
318-
ws.sync_to_volume(topdir, engine_name, volume)
357+
try:
358+
ws.sync_to_volume(topdir, engine_name, volume)
359+
except subprocess.CalledProcessError as exc:
360+
raise SystemExit(
361+
f"FATAL: source sync failed (exit {exc.returncode}).\n"
362+
f" Engine: {engine_name}\n"
363+
f" Volume: {volume}\n"
364+
f" Ensure the container runtime is running and the\n"
365+
f" 'alpine' image is available (run: {engine_name} pull alpine)."
366+
)
319367
print(f"[OK] source synced to volume {volume}")
320368
else:
321369
print("[INFO] bind mode: no sync needed; host path mounted directly")
@@ -375,7 +423,7 @@ def _benchmark(self, cfg, passthrough):
375423
start = time.monotonic()
376424
cmd = ["west", "build", "-b", board] + ([sample] if sample else [])
377425
try:
378-
if cfg.env_type == "container":
426+
if cfg.wants_container:
379427
self._run_container(cfg, cmd)
380428
else:
381429
run_host(cmd)

west_env/config.py

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,26 @@ def __init__(self, data):
113113
if self.jlink_mode not in {"host", "tcp-server", "none"}:
114114
raise ValueError(f"unsupported jlink.mode: {self.jlink_mode}")
115115

116+
@property
117+
def wants_container(self) -> bool:
118+
"""Return True if config implies container-backed execution.
119+
120+
The new config format (env.backend / env.workspace_mode) does not
121+
set ``env.type`` to ``"container"``. This property bridges the
122+
gap so callers don't have to know about both code paths.
123+
"""
124+
# Legacy explicit container mode
125+
if self.env_type == "container":
126+
return True
127+
# New format: explicit non-native backend
128+
if self.backend not in (None, "auto"):
129+
return True
130+
# New format with auto backend: workspace modes that require a
131+
# container volume imply container execution
132+
if self.workspace_mode in ("sync", "copy", "tmpfs"):
133+
return True
134+
return False
135+
116136

117137
def find_config_path(topdir=None):
118138
if topdir is None:

0 commit comments

Comments
 (0)