Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBuild system converted to arm-aware ExternalProject/FetchContent flows: CMake now detects an ARM toolchain, fetches or builds NCT_Projects/DCL/TNTK as needed, and drives driver packaging into ChangesBuild-system: Fetch/Host/Target integration and CI wiring
Sequence DiagramsequenceDiagram
participant GH as GitHub Workflow
participant CMake as CMake
participant Fetch as FetchContent / Git (NCT_Projects)
participant Host as Host Tool Builds (DCL / TNTK)
participant Drivers as Drivers ExternalProject
GH->>GH: install system packages (gcc-arm-none-eabi, cmake, make)
GH->>CMake: run cmake configure/build
CMake->>CMake: detect ARM toolchain?
alt ARM toolchain present
CMake->>Fetch: FetchContent NCT_Projects (if needed)
Fetch-->>CMake: provide NCT headers
CMake->>Host: ExternalProject_Add(dcl / tntk) -> build/install
Host-->>CMake: export DCL_BIN_DIR / TNTK_BIN_DIR
CMake->>Drivers: ExternalProject_Add(drivers_target) with tool paths
Drivers-->>CMake: build drivers, packaging -> Einstein.rex
else no ARM toolchain
CMake->>CMake: use prebuilt `_Data_/Einstein.rex`
end
CMake-->>GH: upload `Einstein/_Data_/Einstein.rex` artifact
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Drivers/CMakeLists.txt (1)
17-17: ⚡ Quick winPin all three
GIT_TAGvalues to specific commit SHAs or version tagsAll three
FetchContent/ExternalProjectdeclarations use floating branch refs (main,master). Any new commit pushed upstream silently changes what gets fetched on the next clean configure, making builds non-reproducible and potentially breaking the CI without any local code change.📌 Suggested fix
FetchContent_Declare( nct_projects GIT_REPOSITORY https://github.com/ekoeppen/NCT_Projects.git - GIT_TAG main + GIT_TAG <commit-sha> )ExternalProject_Add(dcl GIT_REPOSITORY https://github.com/pguyot/DCL.git - GIT_TAG master + GIT_TAG <commit-sha> ... )ExternalProject_Add(tntk_ext GIT_REPOSITORY https://github.com/ekoeppen/tntk.git - GIT_TAG master + GIT_TAG <commit-sha> ... )Also applies to: 32-32, 52-52
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Drivers/CMakeLists.txt` at line 17, Search the three FetchContent/ExternalProject declarations that currently set GIT_TAG to floating branches ("main" and "master") and pin each GIT_TAG to a reproducible identifier (a specific commit SHA or a released version tag) instead of the branch name; update the GIT_TAG values for all occurrences (the GIT_TAG entries at the shown locations and the other two occurrences referenced in the comment) so they reference exact SHAs or tags, and ensure the commit SHAs/tags you choose are valid for the corresponding repositories and recorded in project docs or changelog for future maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Drivers/CMakeLists.txt`:
- Around line 47-62: The ExternalProject_Add call for tntk currently always
injects -DCMAKE_INSTALL_RPATH=${CMAKE_BINARY_DIR}/dcl-install/lib; change it so
the RPATH args (-DCMAKE_INSTALL_RPATH=... and
-DCMAKE_BUILD_WITH_INSTALL_RPATH=ON) are only appended to the CMAKE_ARGS when
DCL is being built as the external dependency (i.e., when you create the dcl
external target and set DCL_BINARY_DIR). Concretely, compute a variable like
TntkCMakeArgs (or conditionally push to CMAKE_ARGS) and only include the RPATH
entries when DCL_EXTERNAL or DCL_BINARY_DIR is defined, keep DEPENDS dcl and
keep setting TNTK_BINARY_DIR when external, otherwise omit the RPATH args so
system-installed DCL is used without a hardcoded dcl-install path.
In `@Drivers/README.md`:
- Line 18: Update the phrase "ARM provided downloads" in the Drivers README to
use the compound adjective form "ARM-provided downloads" (i.e., add a hyphen) so
the sentence reads "...for the general ARM-provided downloads, or a [brew
tap]..." to correct the grammar; locate the occurrence in the README text and
replace it accordingly.
---
Nitpick comments:
In `@Drivers/CMakeLists.txt`:
- Line 17: Search the three FetchContent/ExternalProject declarations that
currently set GIT_TAG to floating branches ("main" and "master") and pin each
GIT_TAG to a reproducible identifier (a specific commit SHA or a released
version tag) instead of the branch name; update the GIT_TAG values for all
occurrences (the GIT_TAG entries at the shown locations and the other two
occurrences referenced in the comment) so they reference exact SHAs or tags, and
ensure the commit SHAs/tags you choose are valid for the corresponding
repositories and recorded in project docs or changelog for future maintenance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 978454a1-9120-4bfb-8e8a-4b71f038bdc4
📒 Files selected for processing (4)
.github/workflows/rex.ymlDrivers/CMakeLists.txtDrivers/EinsteinPortEnabler/CMakeLists.txtDrivers/README.md
💤 Files with no reviewable changes (1)
- .github/workflows/rex.yml
ekoeppen
force-pushed
the
rex-build
branch
2 times, most recently
from
May 5, 2026 05:03
1901945 to
836aa4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Drivers/README.md`:
- Around line 12-13: Update the README text to hyphenate the three compound
modifiers: change "cross compiled" to "cross-compiled", "NewtonOS compatible
tool chain" to "NewtonOS-compatible tool chain" (or "toolchain" if preferred by
project style), and "NewtonOS specific tools" to "NewtonOS-specific tools" so
the compound adjectives are properly hyphenated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02641a56-ebff-40e4-8323-95cc6fa6f7f9
📒 Files selected for processing (4)
.github/workflows/rex.ymlDrivers/CMakeLists.txtDrivers/EinsteinPortEnabler/CMakeLists.txtDrivers/README.md
💤 Files with no reviewable changes (1)
- .github/workflows/rex.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- Drivers/EinsteinPortEnabler/CMakeLists.txt
- Drivers/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Drivers/README.md (1)
12-13:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCompound-modifier hyphens still missing on lines 12-13.
"cross compiled", "NewtonOS compatible tool chain", and "NewtonOS specific tools" each need hyphens before their nouns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Drivers/README.md` around lines 12 - 13, Update the sentence in Drivers/README.md (the phrase shown in the diff) to add compound-word hyphens: change "cross compiled" to "cross-compiled", "NewtonOS compatible tool chain" to "NewtonOS-compatible toolchain", and "NewtonOS specific tools" to "NewtonOS-specific tools" so all compound modifiers are hyphenated correctly.
🧹 Nitpick comments (1)
Drivers/CMakeLists.txt (1)
14-15: ⚡ Quick winPin
GIT_TAGto a specific commit hash or release tag for reproducible builds.All three external dependencies (
nct_projects,dcl,tntk_ext) currently track mutable branch heads (main/master). Any upstream commit will silently change build inputs across machines or CI runs, making failures hard to diagnose and bisect.♻️ Proposed fix
FetchContent_Declare( nct_projects GIT_REPOSITORY https://github.com/ekoeppen/NCT_Projects.git - GIT_TAG main + GIT_TAG <commit-sha-or-release-tag> )ExternalProject_Add(dcl GIT_REPOSITORY https://github.com/pguyot/DCL.git - GIT_TAG master + GIT_TAG <commit-sha-or-release-tag> ... )ExternalProject_Add(tntk_ext GIT_REPOSITORY https://github.com/ekoeppen/tntk.git - GIT_TAG master + GIT_TAG <commit-sha-or-release-tag> ... )Also applies to: 28-29, 58-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Drivers/CMakeLists.txt` around lines 14 - 15, The CMake ExternalProject entries currently pin GIT_TAG to mutable branch names ("main"/"master") for repositories like nct_projects (GIT_REPOSITORY https://github.com/ekoeppen/NCT_Projects.git), dcl and tntk_ext; change each GIT_TAG to a specific commit hash or release tag (a full SHA or stable tag) for reproducible builds, updating the GIT_TAG values for the occurrences that reference nct_projects, dcl, and tntk_ext (the three ExternalProject blocks) so all three use immutable commits/tags instead of "main"/"master".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Drivers/README.md`:
- Around line 37-39: The sentence describing tntk is missing the verb "is";
update the README text "If it not installed, the precompiled packages are
included in the REx." to read "If it is not installed, the precompiled packages
are included in the REx." so the tntk description is grammatically correct.
---
Duplicate comments:
In `@Drivers/README.md`:
- Around line 12-13: Update the sentence in Drivers/README.md (the phrase shown
in the diff) to add compound-word hyphens: change "cross compiled" to
"cross-compiled", "NewtonOS compatible tool chain" to "NewtonOS-compatible
toolchain", and "NewtonOS specific tools" to "NewtonOS-specific tools" so all
compound modifiers are hyphenated correctly.
---
Nitpick comments:
In `@Drivers/CMakeLists.txt`:
- Around line 14-15: The CMake ExternalProject entries currently pin GIT_TAG to
mutable branch names ("main"/"master") for repositories like nct_projects
(GIT_REPOSITORY https://github.com/ekoeppen/NCT_Projects.git), dcl and tntk_ext;
change each GIT_TAG to a specific commit hash or release tag (a full SHA or
stable tag) for reproducible builds, updating the GIT_TAG values for the
occurrences that reference nct_projects, dcl, and tntk_ext (the three
ExternalProject blocks) so all three use immutable commits/tags instead of
"main"/"master".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 497c5e1d-1e7f-414b-a0e9-cc263f74c71b
📒 Files selected for processing (5)
.github/workflows/rex.ymlDrivers/CMakeLists.txtDrivers/EinsteinPortEnabler/CMakeLists.txtDrivers/EinsteinPortEnabler/portenabler.pkgDrivers/README.md
💤 Files with no reviewable changes (1)
- .github/workflows/rex.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- Drivers/EinsteinPortEnabler/CMakeLists.txt
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Drivers/EinsteinPortEnabler/CMakeLists.txt`:
- Around line 76-78: The custom post-build command for target "portenabler" uses
the Unix-only rm executable; replace the raw rm call with CMake's portable file
removal helper by invoking ${CMAKE_COMMAND} -E rm -f so the
add_custom_command(TARGET portenabler POST_BUILD ...) becomes cross-platform;
update the first COMMAND (which currently removes
${PROJECT_SOURCE_DIR}/portenabler.ntkc) to use ${CMAKE_COMMAND} -E rm -f
${PROJECT_SOURCE_DIR}/portenabler.ntkc while leaving the ELFTONTK invocation and
other arguments (${ELFTONTK} -via ${PROJECT_SOURCE_DIR}/portenabler.exp -o
${PROJECT_SOURCE_DIR}/portenabler.ntkc $<TARGET_FILE:portenabler>) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95da136a-2ec7-4a3d-9256-3d5b53b3966e
📒 Files selected for processing (9)
.github/workflows/rex.ymlCMakeLists.txtDrivers/CMakeLists.txtDrivers/EinsteinPortEnabler/CMakeLists.txtDrivers/EinsteinPortEnabler/portenabler.pkgDrivers/README.mdDrivers/host/CMakeLists.txtDrivers/target/CMakeLists.txt_Data_/Einstein.rex
💤 Files with no reviewable changes (1)
- .github/workflows/rex.yml
ekoeppen
force-pushed
the
rex-build
branch
5 times, most recently
from
May 7, 2026 16:06
17ecfcf to
09576e2
Compare
Use CMake to fetch the NCT headers, DCL, and tntk to enable building the REx drivers in one go. Restructure the CMake file to build host and target portions separately.
Collaborator
|
Sorry, I was out for the last two months. I'll try to look into this this weekend ;-) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Use CMake to fetch the NCT headers, DCL, and tntk to enable building the REx drivers in one go.
Summary by CodeRabbit
Chores
Documentation