-
Notifications
You must be signed in to change notification settings - Fork 359
feat(native): add rebuild-on-change for NativeModule #1599
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jeff-hykin
wants to merge
13
commits into
dev
Choose a base branch
from
jeff/feat/native_rebuild
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
d2e9446
feat(native): add rebuild-on-change for NativeModule
jeff-hykin bc5b44b
fix: resolve relative rebuild_on_change paths against module cwd and …
jeff-hykin f2b7b0a
improve native build
jeff-hykin 18c56ea
CI code cleanup
jeff-hykin e01688c
fixup pathing
jeff-hykin 9ec44ec
Merge branch 'jeff/feat/native_rebuild' of github.com:dimensionalOS/d…
jeff-hykin cb5041b
-
jeff-hykin c9fac15
Merge remote-tracking branch 'origin/dev' into jeff/feat/native_rebuild
jeff-hykin ee8e936
chore: regenerate uv.lock after merge with dev
jeff-hykin eb9a913
fix(change_detect): add threading.Lock alongside fcntl.flock
jeff-hykin 4ed36cc
fix(native): include class qualname in build cache key
jeff-hykin 5b7a68e
fix(native): prevent thread leaks in crash test
jeff-hykin 68dd4bc
fix(test): isolate LCM multicast in flaky tests
jeff-hykin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,140 @@ | ||
| # Copyright 2026 Dimensional Inc. | ||
| # | ||
| # Licensed under the Apache License, Version 2.0 (the "License"); | ||
| # you may not use this file except in compliance with the License. | ||
| # You may obtain a copy of the License at | ||
| # | ||
| # http://www.apache.org/licenses/LICENSE-2.0 | ||
| # | ||
| # Unless required by applicable law or agreed to in writing, software | ||
| # distributed under the License is distributed on an "AS IS" BASIS, | ||
| # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| """Tests for NativeModule rebuild-on-change integration.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from pathlib import Path | ||
| import stat | ||
|
|
||
| import pytest | ||
|
|
||
| from dimos.core.native_module import NativeModule, NativeModuleConfig | ||
| from dimos.utils.change_detect import PathEntry | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def _use_tmp_cache(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: | ||
| """Redirect the change-detection cache to a temp dir for every test.""" | ||
| monkeypatch.setattr( | ||
| "dimos.utils.change_detect._get_cache_dir", | ||
| lambda: tmp_path / "cache", | ||
| ) | ||
|
|
||
|
|
||
| @pytest.fixture() | ||
| def build_env(tmp_path: Path) -> dict[str, Path]: | ||
| """Set up a temp directory with a source file, executable path, and marker path.""" | ||
| src = tmp_path / "src" | ||
| src.mkdir() | ||
| (src / "main.c").write_text("int main() { return 0; }") | ||
|
|
||
| exe = tmp_path / "mybin" | ||
| marker = tmp_path / "build_ran.marker" | ||
|
|
||
| # Build script: create the executable and a marker file | ||
| build_script = tmp_path / "build.sh" | ||
| build_script.write_text(f"#!/bin/sh\ntouch {exe}\nchmod +x {exe}\ntouch {marker}\n") | ||
| build_script.chmod(build_script.stat().st_mode | stat.S_IEXEC) | ||
|
|
||
| return {"src": src, "exe": exe, "marker": marker, "build_script": build_script} | ||
|
|
||
|
|
||
| class _RebuildConfig(NativeModuleConfig): | ||
| executable: str = "" | ||
| rebuild_on_change: list[PathEntry] | None = None | ||
|
|
||
|
|
||
| class _RebuildModule(NativeModule[_RebuildConfig]): | ||
| default_config = _RebuildConfig | ||
|
|
||
|
|
||
| def _make_module(build_env: dict[str, Path]) -> _RebuildModule: | ||
| """Create a _RebuildModule pointing at the temp build env.""" | ||
| return _RebuildModule( | ||
| executable=str(build_env["exe"]), | ||
| build_command=f"sh {build_env['build_script']}", | ||
| rebuild_on_change=[str(build_env["src"])], | ||
| cwd=str(build_env["src"]), | ||
| ) | ||
|
|
||
|
|
||
| def test_rebuild_on_change_triggers_build(build_env: dict[str, Path]) -> None: | ||
| """When source files change, the build_command should re-run.""" | ||
| mod = _make_module(build_env) | ||
| try: | ||
| exe = build_env["exe"] | ||
| marker = build_env["marker"] | ||
|
|
||
| # First build: exe doesn't exist → build runs | ||
| mod._maybe_build() | ||
| assert exe.exists() | ||
| assert marker.exists() | ||
| marker.unlink() | ||
|
|
||
| # No change → build should NOT run | ||
| mod._maybe_build() | ||
| assert not marker.exists() | ||
|
|
||
| # Modify source → build SHOULD run | ||
| (build_env["src"] / "main.c").write_text("int main() { return 1; }") | ||
| mod._maybe_build() | ||
| assert marker.exists(), "Build should have re-run after source change" | ||
| finally: | ||
| mod.stop() | ||
|
|
||
|
|
||
| def test_no_change_skips_rebuild(build_env: dict[str, Path]) -> None: | ||
| """When sources haven't changed, build_command must not run again.""" | ||
| mod = _make_module(build_env) | ||
| try: | ||
| marker = build_env["marker"] | ||
|
|
||
| # Initial build | ||
| mod._maybe_build() | ||
| assert marker.exists() | ||
| marker.unlink() | ||
|
|
||
| # Second call — nothing changed | ||
| mod._maybe_build() | ||
| assert not marker.exists(), "Build should have been skipped (no source changes)" | ||
| finally: | ||
| mod.stop() | ||
|
|
||
|
|
||
| def test_rebuild_on_change_none_skips_check(build_env: dict[str, Path]) -> None: | ||
| """When rebuild_on_change is None, no change detection happens at all.""" | ||
| exe = build_env["exe"] | ||
| marker = build_env["marker"] | ||
|
|
||
| mod = _RebuildModule( | ||
| executable=str(exe), | ||
| build_command=f"sh {build_env['build_script']}", | ||
| rebuild_on_change=None, | ||
| cwd=str(build_env["src"]), | ||
| ) | ||
| try: | ||
| # Initial build | ||
| mod._maybe_build() | ||
| assert exe.exists() | ||
| assert marker.exists() | ||
| marker.unlink() | ||
|
|
||
| # Modify source — but rebuild_on_change is None, so no rebuild | ||
| (build_env["src"] / "main.c").write_text("int main() { return 1; }") | ||
| mod._maybe_build() | ||
| assert not marker.exists(), "Should not rebuild when rebuild_on_change is None" | ||
| finally: | ||
| mod.stop() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did_change()always writes the new hash to the cache file as a side effect before returning. So when it returnsTruehere (change detected, rebuild needed), the cache is already updated to the new hash. If the build subsequently fails (e.g.RuntimeErroris raised at line 288), the exception propagates up and the cache is left pointing at the new file state.On the very next call to
_maybe_build():exe.exists()→True(old binary is still there)did_change(...)→False(cache already reflects the new file state)needs_rebuild = False→ the method returns earlyThe module silently continues running the stale executable and will never retry the build, even if you fix the build error and call
start()again.The root fix is to not update the cache during the "check" phase, and only commit the hash after a successful build. One approach is to split
did_changeinto a read-onlyhas_changedcheck and a separateupdate_cachewrite, calling them independently:and after a successful build: