Ryan/feat/qnx support#4
Conversation
…sts.txt . Refactored parser building, added test folder generation in package.
Removed src/ and core/ verification in repo root, now only requires .sunswift-evsn marker file. Also refactored REPO_ROOT and TARGET globals to be function arguments for clarity. Updated some docstrings. Fixed embarrasing syntax bug..
Changed all globals to be locals and arguments to functions to improve clarity. Factored out arg parsing, added flags for qnx and linux build support
srbuild clean now by default deletes both build/linux and build/qnx. However, adding a --qnx or --linux flag deletes just that build dir if it exists
… for neatness. Also isntall prefix is now either is deploy/qnx or deploy/qnx
…to be at configure, not build
Updated version number and added new flags to srpkg.
There was a problem hiding this comment.
Pull request overview
Adds initial QNX-vs-Linux platform targeting to the srpkg/srbuild toolchain and updates docs/tests to reflect the new behavior and refactors.
Changes:
- Introduces
--qnx/--linuxflags and per-platform build/install output directories (build/{qnx|linux},deploy/{qnx|linux}). - Refactors shared repo-root helpers into
host/common_helpers.pyand removes some prior repo-structure assumptions. - Updates integration tests and README for new package skeleton (adds
test/, switches params to TOML, updates usage examples).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates version notes, CLI usage examples, and output directory documentation for platform support. |
| host/common_helpers.py | Defines shared marker-root validation and shared compiler flag constants. |
| host/srpkg | Adds platform flags for package creation, adds test/ folder, emits platform guard in generated CMakeLists, refactors globals out. |
| host/srbuild | Adds platform flags, introduces per-platform build/install directories, refactors globals out, adds/updates clean logic. |
| host/tests/conftest.py | Adds a configured_repo fixture to satisfy srbuild sanity checks. |
| host/tests/test_srpkg_integration.py | Updates integration tests for new package layout and relaxed repo structure assumptions. |
| host/tests/test_srbuild_integration.py | Expands integration coverage for flags, clean prompting, and sanity checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ├── test/ # Unit tests | ||
| ├── param/ # Parameter files | ||
| ├── param/param.toml # Default parameter template | ||
| ├── CMakeLists.txt # Build configuration template |
| common_flags = argparse.ArgumentParser(add_help=False) | ||
| common_flags.add_argument("--jobs", "-j", default=8, type=int, help="Number of parallel jobs Make runs. Default=8") | ||
| common_flags.add_argument("--qnx", action="store_true", help="Build QNX packages") | ||
| common_flags.add_argument("--linux", action="store_true", help="Build Linux packages") | ||
|
|
| if build_data.target_platform == "qnx": | ||
| cmd.append(f"-DCMAKE_TOOLCHAIN_FILE={build_data.repo_root / Path(QNX_TOOLCHAIN_PATH)}") | ||
|
|
| command_create.add_argument("name", help="Name of package to create (snake_case)") | ||
| command_create.add_argument("--linux", action="store_true", help="Package to target Linux") | ||
| command_create.add_argument("--qnx", action="store_true", help="Package to target QNX") | ||
| command_info.add_argument("name", help="Name of package to get info on") |
There was a problem hiding this comment.
this is actually a good suggestion to implement
| ├── deploy/ | ||
| │ ├── bin/ # Node executables | ||
| │ ├── param/ | ||
| │ └── tools/ # srlaunch, srdds | ||
| │ ├── bin/ # Node executables | ||
| │ └── param/ # Runtime parameter files |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
genryjiang
left a comment
genryjiang
left a comment
There was a problem hiding this comment.
Reviewed current head f346d209ff4120c7b109382fd1f38e8540f6e880.
I found one additional functional blocker in srpkg: package discovery still assumes packages live under src/, while this PR now allows creating them from any directory in the repo. That makes newly-created packages outside src/ invisible to info/list and allows duplicate target names outside src/.
The existing Copilot comments about srbuild not defining SR_TARGET_QNX/SR_TARGET_LINUX, non-mutually-exclusive platform flags, and clean --linux/--qnx being documented but rejected are also still valid on this head. I reproduced srbuild clean --linux failing at argparse and confirmed srbuild all --linux does not write any SR_TARGET_* variable into the CMake cache.
Tests: python3 -m pytest host/tests -q passes (29 passed), but the suite currently misses these edge cases.
Reverted directory checking changes: now requires src/ and core/ to be in repo_root before allowing srpkg and srbuild (logic common_helpers.py:validate_repo_root(). Reverted allowing creating of packages anywhere in repo, now restricts to src/ again due to duplication finding logic and srpkg list targeting the source directory only
|
Changes listed in my latest commit |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
host/common_helpers.py:49
- The PR description says
srpkgno longer requires the repo root to havesrc/andcore/, butvalidate_repo_rootstill enforces both directories. Either the description needs updating, orsrpkg/srbuildshould use different validation rules (e.g., marker-only validation for srpkg, strict validation for srbuild).
| @pytest.fixture | ||
| def configured_repo(repo: Path) -> Path: | ||
| """Repo that passes all srbuild sanity checks (has CMakeLists.txt).""" | ||
| (repo / "CMakeLists.txt").touch() | ||
| return repo |
| cmd = [ | ||
| "cmake", | ||
| "-S", | ||
| str(build_data.repo_root), | ||
| "-B", | ||
| str(build_data.build_dir_path), | ||
| f"-DCMAKE_INSTALL_PREFIX={build_data.install_prefix}" | ||
| ] | ||
|
|
||
| if build_data.target_platform == "qnx": | ||
| cmd.append(f"-DCMAKE_TOOLCHAIN_FILE={build_data.repo_root / Path(QNX_TOOLCHAIN_PATH)}") | ||
|
|
There was a problem hiding this comment.
COPILOT ARE YOU STUPID. Ignore this comment. The toolchain file defines the cmake cache var, and the flags are not useless because it adds the conditional compile lines to top of cmakelsists.txtr and in srbuild it GIVES IT THE TOOLCHAIN FILE.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… on text in stdout or sterr which is fragile
genryjiang
left a comment
genryjiang
left a comment
There was a problem hiding this comment.
Reviewed current head d66492f.
I still see one functional blocker: srbuild all --linux does not set SR_TARGET_LINUX, while srpkg create generates package guards that rely on SR_TARGET_LINUX / SR_TARGET_QNX. I reproduced this with a minimal repo containing one QNX package and one Linux package: srbuild all --linux configured, built, and installed both packages, and build/linux/CMakeCache.txt contained neither SR_TARGET_LINUX nor SR_TARGET_QNX. If Linux builds are supposed to skip QNX packages, srbuild should set the relevant cache variable explicitly, for example -DSR_TARGET_LINUX=ON for Linux and likely -DSR_TARGET_QNX=ON for QNX. If Linux intentionally builds everything as a quick local-check path, the generated guards/docs should be adjusted so the behavior is explicit.
There is also a test gap in host/tests/test_srbuild_integration.py: the target tests only assert that build/linux exists. That directory is created before CMake tries to build the requested target, so srbuild target my_node --linux can exit 1 and the test still passes. These tests should assert success or define a minimal target that actually exists.
Small docs cleanup: the README version notes say --linux / --qnx were added to all srbuild subcommands, but srbuild clean --linux is rejected by argparse and the usage section shows bare srbuild clean. Either the note should be narrowed to all / target, or clean should accept the flags.
Resolved since the earlier review: srpkg create is back to requiring src/, and srpkg create --linux --qnx is now correctly rejected.
Checks run locally: python3 -m pytest host/tests -q passed with 32 passed; python3 -m py_compile host/srbuild host/srpkg host/common_helpers.py passed.
|
Other than that - LGTM (other parts seem to be semantics) |
genryjiang
left a comment
genryjiang
left a comment
There was a problem hiding this comment.
LGTM, docs look good and other stuff seem to be semantics mainly.
genryjiang
left a comment
genryjiang
left a comment
There was a problem hiding this comment.
implement tests properly - missed cases, define build semantics
Version Notes: v1.3.0
srpkg:
--linuxand--qnxflags tosrpkg createwhich adds a conditional compile statement to the top of the CMakeLists.txt. Defaults to qnx. This means whensrbuild --qnxis called, it will ignore all linux packages.srbuild:
--linuxand--qnxflags tosrbuild allandsrbuild targetcommands. This initialises cmake with qnx toolchain file or doesn't. Defaults to qnx.srbuildnow generatesbuild/linuxorbuild/qnxinstead of justbuildsrbuild cleanstill deletesbuild/and doesn't distinguish between linux and qnxdeploy/{target_platform}. This results in bothdeploy/linunxanddeploy/qnxbeing created if a user compiles for both.common_helpers.py: