Ryan/reversion#3
Conversation
…git repo root and validates it. Reverted srpkg for future updates
…ions in srutils. Added .gitignore
… add autofill main file
|
nice work, just a few thoughts:
Other than that, LGTM |
|
Yep will keep for now, but can change later |
…idate_name to not accept empty strings, fixed typo in readme
There was a problem hiding this comment.
Pull request overview
This PR updates the host-side tooling to share common repo-root detection/validation logic, expands srpkg’s scaffolding output (TOML params + main.cpp template), and adds CI-backed integration tests.
Changes:
- Added
host/srutils.pyand refactoredsrpkg/srbuildto use shared repo-root resolution + validation. - Updated
srpkgpackage skeleton generation (param TOML +main.cpptemplate; removed older config/param JSON behavior). - Added pytest integration tests and a GitHub Actions workflow to run them.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
target/srlaunch |
Removes repo-root checks; derives deploy/ location from script path. |
host/srutils.py |
New shared helper module for repo-root resolution/validation and error exits. |
host/srpkg |
Refactors repo checks to srutils; updates generated package structure and templates. |
host/srbuild |
Refactors repo checks to srutils; adjusts parallel build invocation. |
host/tests/conftest.py |
Adds fixtures to create a temporary git repo with expected structure. |
host/tests/test_srpkg_integration.py |
Adds end-to-end integration tests for srpkg. |
host/tests/test_srbuild_integration.py |
Adds end-to-end integration tests for srbuild. |
.github/workflows/test.yml |
Adds CI job to run pytest integration tests. |
README.md |
Updates version header and adds v1.2.0 notes. |
.gitignore |
Adds standard Python/IDE/OS ignores and CLI artifact ignores. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| BIN_PATH = DEPLOY_ROOT/"bin" | ||
|
|
||
| if not (DEPLOY_ROOT/"bin").exists() and not (DEPLOY_ROOT/"param").exists(): | ||
| print(f"Error: {__file__} not in deploy/tools directory") |
There was a problem hiding this comment.
The deploy/tools location check only prints an error and then continues executing. This can lead to confusing later failures (e.g., missing BIN_PATH) while still showing a “not in deploy/tools” message. Consider failing fast here (e.g., call die()/sys.exit) once the location check fails.
| print(f"Error: {__file__} not in deploy/tools directory") | |
| print(f"Error: {__file__} not in deploy/tools directory") | |
| sys.exit(1) |
| DEPLOY_ROOT = Path(__file__).resolve().parents[1] | ||
| BIN_PATH = DEPLOY_ROOT/"bin" | ||
|
|
||
| if not (DEPLOY_ROOT/"bin").exists() and not (DEPLOY_ROOT/"param").exists(): |
There was a problem hiding this comment.
The condition if not (DEPLOY_ROOT/"bin").exists() and not (DEPLOY_ROOT/"param").exists() will only flag an error when both directories are missing. If the script is misplaced but one of these directories happens to exist, the check won’t trigger. If the intent is to ensure srlaunch is under deploy/tools, validate the expected structure more directly (e.g., check Path(__file__).parent.name == "tools" and that DEPLOY_ROOT/"bin" exists).
| DEPLOY_ROOT = Path(__file__).resolve().parents[1] | |
| BIN_PATH = DEPLOY_ROOT/"bin" | |
| if not (DEPLOY_ROOT/"bin").exists() and not (DEPLOY_ROOT/"param").exists(): | |
| SCRIPT_DIR = Path(__file__).resolve().parent | |
| DEPLOY_ROOT = SCRIPT_DIR.parents[0] | |
| BIN_PATH = DEPLOY_ROOT/"bin" | |
| if SCRIPT_DIR.name != "tools" or not (DEPLOY_ROOT/"bin").exists() or not (DEPLOY_ROOT/"param").exists(): |
| def resolve_repo_root(cwd: Path = Path.cwd()) -> Path: | ||
| candidate = git_toplevel(cwd) | ||
| if not candidate.exists() or not candidate.is_dir(): | ||
| raise RuntimeError("Error: repo root does not exist or is not a directory") | ||
| return candidate |
There was a problem hiding this comment.
resolve_repo_root(cwd: Path = Path.cwd()) captures Path.cwd() at import time (default args are evaluated once). If any caller relies on the default, it may resolve the wrong directory. Prefer cwd: Path | None = None and set cwd = Path.cwd() inside the function.
| def fill_main(paths: PkgPaths) -> None: | ||
| text = f"""#include "my_node.hpp" // TODO: change to your node | ||
|
|
||
| int main(int argc, char* argv[]) {{ | ||
| // TODO: initialise SRNode | ||
| // TODO: Spin SRNode and catch exceptions | ||
| return 0; | ||
| }} |
There was a problem hiding this comment.
The generated src/main.cpp template hardcodes #include "my_node.hpp", but the package generator doesn’t create that header (and the name won’t match most packages). This makes newly generated packages fail to compile by default. Consider generating a matching header (e.g., {pkg_name}.hpp) and including it, or removing the include / using an include that is guaranteed to exist.
| ## Version Notes: v1.2.0 | ||
| - Cleaned up repo root checks from Henry's merge | ||
| - Removed repo root checks from `srlaunch`. | ||
| - Added `host/srutils.py` for common logic between `srpkg` and `srbuild` | ||
| - Added toml param file generation and autofilling, as well as autofilling a main file template in `srpkg` | ||
| - Added integration tests for `srpkg` and `srlaunch` |
There was a problem hiding this comment.
The v1.2.0 notes claim “Added integration tests for srpkg and srlaunch”, but this PR adds integration tests for srpkg and srbuild (and no srlaunch tests are present under host/tests). Please update the version notes to match what was actually added, or add the missing srlaunch tests if intended.
Version Notes: v1.2.0
srlaunch.host/srutils.pyfor common logic betweensrpkgandsrbuildsrpkgsrpkgandsrlaunch