diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b4bef09..3c8c780 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/install.py b/install.py index 5e74dfd..e675897 100644 --- a/install.py +++ b/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)}" diff --git a/nodes.py b/nodes.py index 26d2d31..a71b959 100644 --- a/nodes.py +++ b/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. ``/acestep.cpp/build/`` (local build alongside the node). + 4. ``/acestep.cpp/build/bin/`` (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( diff --git a/tests/test_install.py b/tests/test_install.py new file mode 100644 index 0000000..05b4938 --- /dev/null +++ b/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 diff --git a/tests/test_nodes.py b/tests/test_nodes.py index 430f1b3..ffb9648 100644 --- a/tests/test_nodes.py +++ b/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