Fix cmake binary output dir + enable build CI on PRs

.github/workflows/ci.yml

@@ -45,11 +45,11 @@ jobs:
   # -------------------------------------------------------------------------
   # Build test: clone acestep.cpp and compile the C++ binaries via install.py
   # Tests the automatic build that ComfyUI Manager triggers on node install.
+  # Runs on every push to main, every PR, and on manual workflow_dispatch.
   # -------------------------------------------------------------------------
   build-test:
     name: Build acestep.cpp binaries (install.py)
     runs-on: ubuntu-latest
-    if: github.event_name == 'workflow_dispatch'
     permissions:
       contents: read
     steps:
@@ -66,8 +66,15 @@ jobs:
       - name: Verify binaries exist
         run: |
           for bin in ace-qwen3 dit-vae; do
-            test -f "acestep.cpp/build/$bin" \
-              && echo "✓ $bin" \
-              || (echo "✗ $bin not found" && exit 1)
+            # install.py passes -DCMAKE_RUNTIME_OUTPUT_DIRECTORY so binaries
+            # land in build/ directly; also check build/bin/ as a fallback for
+            # any build done without that flag (mirrors _binary_in_build logic).
+            if test -f "acestep.cpp/build/$bin"; then
+              echo "✓ $bin (build/)"
+            elif test -f "acestep.cpp/build/bin/$bin"; then
+              echo "✓ $bin (build/bin/)"
+            else
+              echo "✗ $bin not found in build/ or build/bin/" && exit 1
+            fi
           done
 

install.py

@@ -70,6 +70,26 @@ def _run(cmd, cwd):
         )
 
 
+def _binary_exists(build_dir, name):
+    """Return True if *name* exists in build_dir or build_dir/bin.
+
+    ggml's CMakeLists.txt sets CMAKE_RUNTIME_OUTPUT_DIRECTORY to
+    ``${CMAKE_BINARY_DIR}/bin`` when it is used as a subdirectory (i.e. the
+    normal case here), which causes the built executables to land in
+    ``build/bin/`` rather than ``build/``.  We therefore check both locations
+    so that binaries produced by either the new cmake configure command (which
+    now passes CMAKE_RUNTIME_OUTPUT_DIRECTORY explicitly) or by an older /
+    manual build are found correctly.
+    """
+    for candidate in (
+        os.path.join(build_dir, name),
+        os.path.join(build_dir, "bin", name),
+    ):
+        if os.path.isfile(candidate):
+            return True
+    return False
+
+
 # ---------------------------------------------------------------------------
 # Main installation routine
 # ---------------------------------------------------------------------------
@@ -87,9 +107,11 @@ def install() -> None:
             )
             return
 
-    # Skip rebuild if both binaries already exist
+    # Skip rebuild if both binaries already exist.
+    # ggml's CMakeLists.txt (when used as a subdirectory) redirects executables
+    # to build/bin/ via CMAKE_RUNTIME_OUTPUT_DIRECTORY, so check both locations.
     build_dir = os.path.join(REPO_DIR, "build")
-    if all(shutil.which(b, path=build_dir) for b in BINARIES):
+    if all(_binary_exists(build_dir, b) for b in BINARIES):
         print(
             f"[acestep-cpp] Binaries already present in {build_dir} — skipping build.",
             flush=True,
@@ -116,7 +138,14 @@ def install() -> None:
 
     os.makedirs(build_dir, exist_ok=True)
     print("[acestep-cpp] Running CMake configure …", flush=True)
-    _run(["cmake", ".."] + _cmake_flags(backend), cwd=build_dir)
+    # Pass CMAKE_RUNTIME_OUTPUT_DIRECTORY explicitly so ggml's CMakeLists.txt
+    # (which defaults to ${CMAKE_BINARY_DIR}/bin when used as a subdirectory)
+    # does not redirect the ace-qwen3 and dit-vae executables into build/bin/.
+    _run(
+        ["cmake", "..", f"-DCMAKE_RUNTIME_OUTPUT_DIRECTORY={build_dir}"]
+        + _cmake_flags(backend),
+        cwd=build_dir,
+    )
 
     # Build
     jobs = str(multiprocessing.cpu_count())
@@ -127,7 +156,7 @@ def install() -> None:
     )
 
     # Verify
-    missing = [b for b in BINARIES if not shutil.which(b, path=build_dir)]
+    missing = [b for b in BINARIES if not _binary_exists(build_dir, b)]
     if missing:
         raise RuntimeError(
             f"Build finished but expected binaries not found: {', '.join(missing)}"

nodes.py

@@ -82,6 +82,24 @@ def _coerce_float(value: Any, default: float) -> float:
     if isinstance(value, str):
         return float(value) if value.strip() else default
     return float(value)
+def _binary_in_build(build_dir: str, name: str) -> Optional[str]:
+    """Return the path to *name* if it exists in *build_dir* or *build_dir*/bin.
+
+    ggml's CMakeLists.txt sets ``CMAKE_RUNTIME_OUTPUT_DIRECTORY`` to
+    ``${CMAKE_BINARY_DIR}/bin`` when it is used as a cmake subdirectory (the
+    normal case for acestep.cpp).  The cmake configure command now passes this
+    variable explicitly, but we still check *build_dir*/bin/ so that existing
+    installations built before this fix are found without a forced rebuild.
+    """
+    for candidate in (
+        os.path.join(build_dir, name),
+        os.path.join(build_dir, "bin", name),
+    ):
+        if os.path.isfile(candidate):
+            return candidate
+    return None
+
+
 def get_binary_path(binary_name: str) -> Optional[str]:
     """
     Locate an acestep.cpp binary.
@@ -90,6 +108,8 @@ def get_binary_path(binary_name: str) -> Optional[str]:
       1. Explicit path from config.json ``binary_paths`` mapping.
       2. System PATH (via shutil.which).
       3. ``<node_dir>/acestep.cpp/build/<binary_name>`` (local build alongside the node).
+      4. ``<node_dir>/acestep.cpp/build/bin/<binary_name>`` (ggml's default output
+         directory when used as a cmake subdirectory).
     """
     config = load_config()
     explicit = config.get("binary_paths", {}).get(binary_name)
@@ -100,11 +120,8 @@ def get_binary_path(binary_name: str) -> Optional[str]:
     if on_path:
         return on_path
 
-    local = os.path.join(os.path.dirname(__file__), "acestep.cpp", "build", binary_name)
-    if os.path.isfile(local):
-        return local
-
-    return None
+    build_base = os.path.join(os.path.dirname(__file__), "acestep.cpp", "build")
+    return _binary_in_build(build_base, binary_name)
 
 
 # ---------------------------------------------------------------------------
@@ -438,7 +455,14 @@ def build(
         os.makedirs(build_dir, exist_ok=True)
 
         # --- CMake configure --------------------------------------------------
-        cmake_cmd = ["cmake", ".."] + self._cmake_flags(resolved)
+        # Pass CMAKE_RUNTIME_OUTPUT_DIRECTORY explicitly so that ggml's
+        # CMakeLists.txt (which defaults to ${CMAKE_BINARY_DIR}/bin when used
+        # as a subdirectory) does not redirect ace-qwen3 and dit-vae into
+        # build/bin/ instead of build/.
+        cmake_cmd = [
+            "cmake", "..",
+            f"-DCMAKE_RUNTIME_OUTPUT_DIRECTORY={build_dir}",
+        ] + self._cmake_flags(resolved)
         log.append(f"[AcestepCPP] Configuring: {' '.join(cmake_cmd)}")
         if not shutil.which("cmake"):
             raise RuntimeError(
@@ -454,11 +478,13 @@ def build(
         self._run(build_cmd, cwd=build_dir, log_lines=log)
 
         # --- Verify outputs ---------------------------------------------------
-        missing = []
-        for binary in self._BINARIES:
-            path = os.path.join(build_dir, binary)
-            if not os.path.isfile(path):
-                missing.append(binary)
+        # _binary_in_build checks both build/ (new cmake configure with explicit
+        # output dir) and build/bin/ (ggml's default when
+        # CMAKE_RUNTIME_OUTPUT_DIRECTORY is not set).
+        missing = [
+            binary for binary in self._BINARIES
+            if _binary_in_build(build_dir, binary) is None
+        ]
 
         if missing:
             raise RuntimeError(

tests/test_install.py

@@ -0,0 +1,68 @@
+"""Tests for install.py helpers.
+
+Validates the _binary_exists function that checks both build/ and build/bin/
+to handle ggml's CMAKE_RUNTIME_OUTPUT_DIRECTORY default behaviour.
+"""
+
+import importlib.util
+import sys
+from pathlib import Path
+
+import pytest
+
+# Load install.py by absolute path (it is a standalone script, not a package).
+_repo_root = Path(__file__).parent.parent
+_install_spec = importlib.util.spec_from_file_location(
+    "install",
+    str(_repo_root / "install.py"),
+)
+_install_module = importlib.util.module_from_spec(_install_spec)
+# Prevent install() from running on import by making __name__ != "__main__"
+sys.modules["install"] = _install_module
+_install_spec.loader.exec_module(_install_module)
+
+_binary_exists = _install_module._binary_exists
+
+
+# ---------------------------------------------------------------------------
+# _binary_exists
+# ---------------------------------------------------------------------------
+
+class TestBinaryExists:
+    """_binary_exists checks build/ and build/bin/ (ggml's default output dir)."""
+
+    def test_found_directly_in_build(self, tmp_path):
+        """Binary placed directly in build/ is detected (new cmake configure)."""
+        (tmp_path / "ace-qwen3").write_text("mock")
+        assert _binary_exists(str(tmp_path), "ace-qwen3") is True
+
+    def test_found_in_build_bin(self, tmp_path):
+        """Binary placed in build/bin/ (ggml default) is detected."""
+        bin_dir = tmp_path / "bin"
+        bin_dir.mkdir()
+        (bin_dir / "ace-qwen3").write_text("mock")
+        assert _binary_exists(str(tmp_path), "ace-qwen3") is True
+
+    def test_not_found_returns_false(self, tmp_path):
+        """Returns False when binary is absent from both locations."""
+        assert _binary_exists(str(tmp_path), "ace-qwen3") is False
+
+    def test_both_locations_present(self, tmp_path):
+        """Returns True when binary exists in build/ (checked before build/bin/)."""
+        (tmp_path / "ace-qwen3").write_text("in_build")
+        bin_dir = tmp_path / "bin"
+        bin_dir.mkdir()
+        (bin_dir / "ace-qwen3").write_text("in_bin")
+        assert _binary_exists(str(tmp_path), "ace-qwen3") is True
+
+    def test_wrong_name_not_found(self, tmp_path):
+        """Binary with a different name is not falsely matched."""
+        (tmp_path / "dit-vae").write_text("mock")
+        assert _binary_exists(str(tmp_path), "ace-qwen3") is False
+
+    def test_dit_vae_in_bin(self, tmp_path):
+        """Works for dit-vae as well as ace-qwen3."""
+        bin_dir = tmp_path / "bin"
+        bin_dir.mkdir()
+        (bin_dir / "dit-vae").write_text("mock")
+        assert _binary_exists(str(tmp_path), "dit-vae") is True

tests/test_nodes.py

@@ -4,6 +4,7 @@
 
 import os
 import sys
+import tempfile
 
 import pytest
 
@@ -278,3 +279,87 @@ def test_cmake_flags_cuda(self):
 
     def test_cmake_flags_blas(self):
         assert "-DGGML_BLAS=ON" in nodes.AcestepCPPBuilder._cmake_flags("blas")
+
+
+# ===========================================================================
+# _binary_in_build — shared helper for multi-location binary search
+# ===========================================================================
+
+class TestBinaryInBuild:
+    """_binary_in_build checks both build/ and build/bin/ (ggml default)."""
+
+    def test_found_directly(self, tmp_path):
+        binary = tmp_path / "ace-qwen3"
+        binary.write_text("mock")
+        assert nodes._binary_in_build(str(tmp_path), "ace-qwen3") == str(binary)
+
+    def test_found_in_bin_subdir(self, tmp_path):
+        bin_dir = tmp_path / "bin"
+        bin_dir.mkdir()
+        binary = bin_dir / "ace-qwen3"
+        binary.write_text("mock")
+        assert nodes._binary_in_build(str(tmp_path), "ace-qwen3") == str(binary)
+
+    def test_prefers_direct_over_bin(self, tmp_path):
+        direct = tmp_path / "ace-qwen3"
+        direct.write_text("direct")
+        bin_dir = tmp_path / "bin"
+        bin_dir.mkdir()
+        (bin_dir / "ace-qwen3").write_text("in_bin")
+        assert nodes._binary_in_build(str(tmp_path), "ace-qwen3") == str(direct)
+
+    def test_returns_none_when_absent(self, tmp_path):
+        assert nodes._binary_in_build(str(tmp_path), "ace-qwen3") is None
+
+
+# ===========================================================================
+# get_binary_path — multi-location search
+# ===========================================================================
+
+class TestGetBinaryPath:
+    """get_binary_path must honour explicit config paths and system PATH;
+    the local build/ vs build/bin/ logic is delegated to _binary_in_build
+    and covered by TestBinaryInBuild above."""
+
+    def test_explicit_config_path_returned(self, tmp_path, monkeypatch):
+        """Binary path from config.json binary_paths is returned directly."""
+        binary = tmp_path / "ace-qwen3"
+        binary.write_text("mock")
+        monkeypatch.setattr(
+            nodes, "load_config",
+            lambda: {"binary_paths": {"ace-qwen3": str(binary)}},
+        )
+        assert nodes.get_binary_path("ace-qwen3") == str(binary)
+
+    def test_explicit_config_path_missing_file_ignored(self, tmp_path, monkeypatch):
+        """Config binary_paths entry is skipped when the file does not exist."""
+        monkeypatch.setattr(
+            nodes, "load_config",
+            lambda: {"binary_paths": {"ace-qwen3": str(tmp_path / "nonexistent")}},
+        )
+        monkeypatch.setattr(nodes.shutil, "which", lambda *a, **kw: None)
+        # No local build files either — result should be None
+        result = nodes.get_binary_path("ace-qwen3")
+        assert result is None
+
+    def test_system_path_lookup(self, monkeypatch):
+        """Binary found on the system PATH is returned."""
+        monkeypatch.setattr(nodes, "load_config", lambda: {})
+        monkeypatch.setattr(
+            nodes.shutil, "which", lambda name, **kw: f"/usr/bin/{name}"
+        )
+        result = nodes.get_binary_path("ace-qwen3")
+        assert result == "/usr/bin/ace-qwen3"
+
+    def test_returns_none_when_nowhere(self, monkeypatch):
+        """Returns None when binary is absent from config, PATH, and local build."""
+        monkeypatch.setattr(nodes, "load_config", lambda: {})
+        monkeypatch.setattr(nodes.shutil, "which", lambda *a, **kw: None)
+        # Redirect the node __file__ into an empty temp dir so no local build
+        # file is found.
+        with tempfile.TemporaryDirectory() as tmp:
+            monkeypatch.setattr(
+                nodes, "__file__", os.path.join(tmp, "nodes.py")
+            )
+            result = nodes.get_binary_path("ace-qwen3")
+        assert result is None