build(cmake): refactor CMake, switch to STB submodule, use CImg2Num in WASM#273
Merged
Conversation
- Fix install/export logic - Separate Emscripten build path - Add proper INSTALL_INTERFACE usage - Remove deprecated Python bindings - Improve example build control
- Add STB as third_party submodule - Remove CMake file(DOWNLOAD) usage - Update example include paths - Improve reproducibility and offline builds - Update docs
- WASM now depends on CImg2Num instead of Img2Num - Remove direct C++ namespace usage in wrapper - Use extern "C" for exported functions - Strengthen layered architecture (core → C API → bindings)
github-actions
Bot
added
documentation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds an STB git submodule, refactors top-level and package CMake install/export/config generation, converts core and C bindings to non-explicit-shared library targets, switches JS/WASM to link against the C ABI bindings and include C headers, removes Python bindings, updates example include paths, and adds/updates Doxygen/internal/public docs and build scripts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)✅ Copyable Unit Test edits generated.
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
example-apps/console-cpp/CMakeLists.txt (1)
16-19: Minor: Update outdated comment.The comment says "downloaded STB headers" but STB is now a submodule, not downloaded at build time.
📝 Proposed fix
-# Include the downloaded STB headers +# Include STB headers from submodule target_include_directories(console_cpp_app PRIVATE ${CMAKE_SOURCE_DIR}/third_party )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-cpp/CMakeLists.txt` around lines 16 - 19, The comment above the target_include_directories(console_cpp_app PRIVATE ${CMAKE_SOURCE_DIR}/third_party) is outdated—change "Include the downloaded STB headers" to reflect that STB is provided as a submodule (e.g., "Include STB and other third-party headers from submodule"), so update the comment text to accurately describe the source of the headers.bindings/js/CMakeLists.txt (1)
22-23: Key change for C ABI compliance—consider updating the comment.Switching to
CImg2Numcorrectly enforces the C ABI boundary for WASM exports, addressing Issue#264. The comment still says "Link core library object files" but this now links the C bindings library.📝 Proposed fix
-# Link core library object files +# Link C bindings library (provides C ABI surface for WASM exports) target_link_libraries(img2num_wasm PRIVATE CImg2Num)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/js/CMakeLists.txt` around lines 22 - 23, Update the misleading comment above target_link_libraries so it reflects that img2num_wasm is now linking the C bindings library (CImg2Num) to enforce the C ABI for WASM exports; locate the target_link_libraries call for img2num_wasm and replace the "Link core library object files" comment with a short description like "Link C bindings library (C ABI) for WASM exports" to make intent clear.example-apps/console-c/CMakeLists.txt (1)
16-19: Minor: Update outdated comment.Same as the C++ example—the comment references "downloaded headers" but STB is now a submodule.
📝 Proposed fix
-# Include the downloaded headers directly +# Include STB headers from submodule target_include_directories(console_c_app PRIVATE ${CMAKE_SOURCE_DIR}/third_party )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@example-apps/console-c/CMakeLists.txt` around lines 16 - 19, The comment above the target_include_directories(console_c_app PRIVATE ${CMAKE_SOURCE_DIR}/third_party) is outdated; change the comment text to indicate STB is included as a submodule rather than "downloaded headers" and ensure it accurately describes why we include ${CMAKE_SOURCE_DIR}/third_party for console_c_app.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/c/CMakeLists.txt`:
- Line 15: The add_library declaration for the C ABI target currently omits the
SHARED keyword causing CMake to default to a static lib; update the library
definitions by adding the explicit SHARED type to the add_library call for the C
binding target (add_library(CImg2Num SHARED ...)) and apply the same change for
the core target (add_library(Img2Num SHARED ...)) so both targets are built as
shared libraries regardless of BUILD_SHARED_LIBS.
In `@CMakeLists.txt`:
- Around line 39-43: The generated version file name is incorrect and not
installed: change the write_basic_package_version_file output from
"my_libConfigVersion.cmake" to "Img2NumConfigVersion.cmake" (call site:
write_basic_package_version_file) and add an install step to install that
version file alongside the package config (ensure the installed filename matches
Img2NumConfigVersion.cmake and is placed in the same DESTINATION as the
Img2NumConfig.cmake/Img2NumConfigVersion.cmake export), so downstream
find_package(Img2Num 0.0.0) works; update the existing install(EXPORT
...)/install(FILES ...) invocation to include the new version file.
In `@core/CMakeLists.txt`:
- Around line 32-34: The current install(DIRECTORY include/ DESTINATION
include/img2num) unconditionally installs everything including internal headers
under core/include/internal/ which must be excluded; update the install rule for
public headers by either adding exclusion flags (e.g., add EXCLUDE_FROM_ALL and
a REGEX that matches "^.*/internal/.*" EXCLUDE to the existing install(DIRECTORY
include/ ...) invocation) or replace the broad DIRECTORY install with explicit
installs of public headers (use install(FILES ...) for top-level public headers
like img2num.h and install(DIRECTORY core/include/img2num/ ...) for the public
subdir), ensuring that no files from core/include/internal/ are installed.
---
Nitpick comments:
In `@bindings/js/CMakeLists.txt`:
- Around line 22-23: Update the misleading comment above target_link_libraries
so it reflects that img2num_wasm is now linking the C bindings library
(CImg2Num) to enforce the C ABI for WASM exports; locate the
target_link_libraries call for img2num_wasm and replace the "Link core library
object files" comment with a short description like "Link C bindings library (C
ABI) for WASM exports" to make intent clear.
In `@example-apps/console-c/CMakeLists.txt`:
- Around line 16-19: The comment above the
target_include_directories(console_c_app PRIVATE
${CMAKE_SOURCE_DIR}/third_party) is outdated; change the comment text to
indicate STB is included as a submodule rather than "downloaded headers" and
ensure it accurately describes why we include ${CMAKE_SOURCE_DIR}/third_party
for console_c_app.
In `@example-apps/console-cpp/CMakeLists.txt`:
- Around line 16-19: The comment above the
target_include_directories(console_cpp_app PRIVATE
${CMAKE_SOURCE_DIR}/third_party) is outdated—change "Include the downloaded STB
headers" to reflect that STB is provided as a submodule (e.g., "Include STB and
other third-party headers from submodule"), so update the comment text to
accurately describe the source of the headers.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.gitmodulesCMakeLists.txtbindings/c/CMakeLists.txtbindings/js/CMakeLists.txtbindings/js/src/wasm_wrapper.cppbindings/py/CMakeLists.txtbindings/py/src/python_wrapper.cppcore/CMakeLists.txtdocs/docs/contributing/setup-and-dependencies/index.mddocs/docs/internal/core/internal-code/graph/explained.mdexample-apps/console-c/CMakeLists.txtexample-apps/console-cpp/CMakeLists.txtthird_party/stb
💤 Files with no reviewable changes (2)
- bindings/py/CMakeLists.txt
- bindings/py/src/python_wrapper.cpp
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
README.md (1)
71-73: Consider replacing deprecatedvalignattribute with modern CSS.The
valignattribute is deprecated in HTML5. While GitHub's Markdown renderer still supports it, consider using modern CSS styling for better future compatibility.♻️ Proposed alternative using inline CSS
-[<img src="https://github.com/Ryan-Millard.png" width="20" height="20" valign="middle" alt="Ryan-Millard avatar"> `@Ryan-Millard`](https://github.com/Ryan-Millard) $\cdot$ -[<img src="https://github.com/hjmillard.png" width="20" height="20" valign="middle" alt="hjmillard avatar"> `@hjmillard`](https://github.com/hjmillard) $\cdot$ -[<img src="https://github.com/krasner.png" width="20" height="20" valign="middle" alt="krasner avatar"> `@krasner`](https://github.com/krasner) +[<img src="https://github.com/Ryan-Millard.png" width="20" height="20" style="vertical-align: middle;" alt="Ryan-Millard avatar"> `@Ryan-Millard`](https://github.com/Ryan-Millard) $\cdot$ +[<img src="https://github.com/hjmillard.png" width="20" height="20" style="vertical-align: middle;" alt="hjmillard avatar"> `@hjmillard`](https://github.com/hjmillard) $\cdot$ +[<img src="https://github.com/krasner.png" width="20" height="20" style="vertical-align: middle;" alt="krasner avatar"> `@krasner`](https://github.com/krasner)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 71 - 73, The README uses the deprecated valign attribute on the avatar <img> tags; remove the valign="middle" attributes from those <img> elements and replace with modern CSS (e.g., add style="vertical-align: middle;" or a utility/class such as class="avatar align-middle" and define vertical-align in CSS) so the avatars remain vertically centered without using deprecated HTML attributes; update the three affected <img> tags (the ones showing Ryan-Millard, hjmillard, and krasner) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/CMakeLists.txt`:
- Line 14: The project intends to build a shared library but currently calls
add_library(Img2Num ${CORE_SRC}) which leaves the library type implicit; update
the add_library invocation for the Img2Num target to explicitly request a shared
library by adding the SHARED keyword so it becomes add_library(Img2Num SHARED
${CORE_SRC}) to avoid depending on BUILD_SHARED_LIBS and ensure consistent
packaging.
- Around line 25-35: The install rules currently miss RUNTIME DESTINATION for
the Img2Num target and use EXCLUDE_FROM_ALL on the header-directory install;
update the target install for Img2Num (the install(TARGETS Img2Num EXPORT
Img2NumTargets ...) block) to include a RUNTIME DESTINATION (e.g., RUNTIME
DESTINATION lib) so Windows DLLs are installed, and remove the EXCLUDE_FROM_ALL
property from the install(DIRECTORY include/ ...) rule so headers are installed
by default; keep the existing LIBRARY, ARCHIVE and INCLUDES DESTINATION entries.
In `@README.md`:
- Line 54: Update the README.md link target that currently points to
"CONTIRBUTING.md" to the correct "CONTRIBUTING.md"; locate the link text in
README.md (the string "CONTIRBUTING.md" inside the markdown link) and correct
the filename so the hyperlink points to the existing CONTRIBUTING.md file on the
repository.
---
Nitpick comments:
In `@README.md`:
- Around line 71-73: The README uses the deprecated valign attribute on the
avatar <img> tags; remove the valign="middle" attributes from those <img>
elements and replace with modern CSS (e.g., add style="vertical-align: middle;"
or a utility/class such as class="avatar align-middle" and define vertical-align
in CSS) so the avatars remain vertically centered without using deprecated HTML
attributes; update the three affected <img> tags (the ones showing Ryan-Millard,
hjmillard, and krasner) accordingly.
…ed on public and internal code
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/docs/internal/bindings/c/api-reference.md (1)
2-22:⚠️ Potential issue | 🟠 MajorInternal C bindings reference page is pointing to the wrong API surface.
This page now identifies as C++ and links/embeds the public
/docs/c/api/route, which conflicts withdocs/internal/bindings/c/intent and misroutes internal docs users.Proposed correction
-title: C++ API Reference +title: Internal C API Reference description: > - This page provides a full-page view of the Img2Num's C++ API reference + This page provides a full-page view of the Img2Num C bindings API reference generated by Doxygen. Use the fullscreen button to expand the view for easier navigation and code browsing. @@ -<a href="/Img2Num/info/docs/c/api/" onClick={(e) => { e.preventDefault(); window.location.href = "/Img2Num/info/docs/c/api/"; }}> Doxygen documentation </a>{' '} directly. +<a href="/Img2Num/info/docs/internal/bindings/c/api/" onClick={(e) => { e.preventDefault(); window.location.href = "/Img2Num/info/docs/internal/bindings/c/api/"; }}> Doxygen documentation </a>{' '} directly. @@ - src="/Img2Num/info/docs/c/api/" - title="C++ API Reference (Doxygen)" + src="/Img2Num/info/docs/internal/bindings/c/api/" + title="Internal C API Reference (Doxygen)" @@ -This page is a direct proxy for the{' '} <a href="/Img2Num/info/docs/c/api/" onClick={(e) => { e.preventDefault(); window.location.href = "/Img2Num/info/docs/c/api/"; }}> Doxygen documentation </a> generated from the C++ core directory. +This page is a direct proxy for the{' '} <a href="/Img2Num/info/docs/internal/bindings/c/api/" onClick={(e) => { e.preventDefault(); window.location.href = "/Img2Num/info/docs/internal/bindings/c/api/"; }}> Doxygen documentation </a> generated from the C bindings directory.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/internal/bindings/c/api-reference.md` around lines 2 - 22, The page incorrectly labels and links to the public C++ API instead of the internal C bindings; update the title, description, anchor link href/onClick targets, and the FullscreenIframe src (references: title string, the two <a href=...> anchors, and the FullscreenIframe component's src prop) to point to the internal C bindings route and wording (e.g., change "C++ API Reference" and "/Img2Num/info/docs/c/api/" to the internal C bindings path and a matching "C API Reference (Internal)" description) so the page serves the internal docs surface instead of the public C++ API.
🧹 Nitpick comments (3)
docs/docs/c/index.md (1)
1-6: Add minimal body content to avoid an empty landing page.This page currently has only front matter, so
/c/will render blank. Add a short intro plus links to the C docs/API pages.Proposed content addition
--- title: Img2Num C sidebar_label: C sidebar_position: 3 --- + +Welcome to the Img2Num C documentation. + +- [C API Reference](./api-reference) +- [C Guides](./docs/)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/docs/c/index.md` around lines 1 - 6, The page currently contains only front matter (title: "Img2Num C", sidebar_label: "C") and renders empty; add a short introductory paragraph under the front matter describing the C SDK (one or two sentences) and include a brief bulleted or inline list of links to the main C docs/API pages (e.g., "Getting Started", "API Reference", "Examples") so the /c/ landing page is not blank; update the file named index.md in the C docs folder by inserting this minimal body content below the existing front matter.bindings/c/Doxyfile.public (1)
9-9: Consider whethersrcshould be in INPUT for public documentation.Including
srcin the INPUT for public documentation may expose implementation details in the generated docs. WhileEXTRACT_PRIVATE = NOandINTERNAL_DOCS = NOshould filter most internal content, you might want to verify that only intended public documentation is generated.If the source files contain documentation for public APIs that aren't in headers, this is fine. Otherwise, consider limiting INPUT to just
includeand the.doxfiles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindings/c/Doxyfile.public` at line 9, The Doxygen INPUT currently lists "src" alongside "include" and the .dox files which may expose implementation details; review the INPUT entry (the INPUT = include src ../../doxygen/img2num.h.dox doxygen/home_page.dox line) and either remove "src" or restrict it (e.g., only include specific public source files) so that generated public docs only include intended API docs, while verifying EXTRACT_PRIVATE and INTERNAL_DOCS settings still block internals if you keep any source files; update the INPUT to "include" plus the .dox files or a curated list of public sources accordingly.docs/scripts/doxygen.js (1)
48-48: Hardcoded.gitkeepcontent refers only to "C++ API docs".The
.gitkeepcontent mentions "C++ API docs" but this file is written to all output directories including C bindings, JS bindings, and internal docs. Consider making the content generic or parameterized.✏️ Suggested fix
mkdirSync(outDir, { recursive: true }); - writeFileSync(join(outDir, ".gitkeep"), "See the `doxygen.js` docs script about this.\n\nThis is for Doxygen's HTML output (C++ API docs)."); + writeFileSync(join(outDir, ".gitkeep"), "See the `doxygen.js` docs script about this.\n\nThis is for Doxygen's HTML output.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/scripts/doxygen.js` at line 48, The .gitkeep file written by writeFileSync(join(outDir, ".gitkeep"), ...) contains a hardcoded "C++ API docs" message but outDir is used for multiple doc types; change the content to a generic or parameterized message (e.g., reference the doc type variable or a neutral phrase like "API docs generated by Doxygen") so the note applies to C++, C bindings, JS bindings, and internal docs—update the string passed to writeFileSync accordingly and, if available, use an existing variable (such as a docType or target identifier) to compose the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/docs/c/api-reference.md`:
- Around line 2-22: The page incorrectly labels the API as C++ in the
frontmatter and body; change all occurrences of "C++" to "C" so labels and copy
match the docs/c location and target. Update the frontmatter title and
description (replace "C++ API Reference" and "Img2Num's C++ API reference" with
"C API Reference" / "Img2Num's C API reference"), update the FullscreenIframe
title prop (currently "C++ API Reference (Doxygen)") to "C API Reference
(Doxygen)", and adjust the inline anchor link text and the "About this page"
sentence to reference the C Doxygen documentation instead of C++.
In `@docs/scripts/doxygen.js`:
- Around line 52-56: The existence check uses DOXYFILE_PATH (a relative path)
while DOXYFILE_DIR is built from ROOT_DIR (absolute), causing inconsistent
resolution; update DOXYFILE_PATH to be constructed using join(ROOT_DIR, "..",
srcDir, fileName) (same pattern as DOXYFILE_DIR) so existsSync checks the
absolute path, and keep DOXYFILE_DIR computed from ROOT_DIR and srcDir for
consistency; adjust references to DOXYFILE_PATH, DOXYFILE_DIR, join, existsSync,
ROOT_DIR, srcDir, and fileName accordingly.
---
Outside diff comments:
In `@docs/docs/internal/bindings/c/api-reference.md`:
- Around line 2-22: The page incorrectly labels and links to the public C++ API
instead of the internal C bindings; update the title, description, anchor link
href/onClick targets, and the FullscreenIframe src (references: title string,
the two <a href=...> anchors, and the FullscreenIframe component's src prop) to
point to the internal C bindings route and wording (e.g., change "C++ API
Reference" and "/Img2Num/info/docs/c/api/" to the internal C bindings path and a
matching "C API Reference (Internal)" description) so the page serves the
internal docs surface instead of the public C++ API.
---
Nitpick comments:
In `@bindings/c/Doxyfile.public`:
- Line 9: The Doxygen INPUT currently lists "src" alongside "include" and the
.dox files which may expose implementation details; review the INPUT entry (the
INPUT = include src ../../doxygen/img2num.h.dox doxygen/home_page.dox line) and
either remove "src" or restrict it (e.g., only include specific public source
files) so that generated public docs only include intended API docs, while
verifying EXTRACT_PRIVATE and INTERNAL_DOCS settings still block internals if
you keep any source files; update the INPUT to "include" plus the .dox files or
a curated list of public sources accordingly.
In `@docs/docs/c/index.md`:
- Around line 1-6: The page currently contains only front matter (title:
"Img2Num C", sidebar_label: "C") and renders empty; add a short introductory
paragraph under the front matter describing the C SDK (one or two sentences) and
include a brief bulleted or inline list of links to the main C docs/API pages
(e.g., "Getting Started", "API Reference", "Examples") so the /c/ landing page
is not blank; update the file named index.md in the C docs folder by inserting
this minimal body content below the existing front matter.
In `@docs/scripts/doxygen.js`:
- Line 48: The .gitkeep file written by writeFileSync(join(outDir, ".gitkeep"),
...) contains a hardcoded "C++ API docs" message but outDir is used for multiple
doc types; change the content to a generic or parameterized message (e.g.,
reference the doc type variable or a neutral phrase like "API docs generated by
Doxygen") so the note applies to C++, C bindings, JS bindings, and internal
docs—update the string passed to writeFileSync accordingly and, if available,
use an existing variable (such as a docType or target identifier) to compose the
message.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
bindings/c/Doxyfilebindings/c/Doxyfile.publicbindings/js/Doxyfile.internalcore/Doxyfilecore/Doxyfile.internalcore/Doxyfile.publicdocs/.gitignoredocs/docs/c/api-reference.mddocs/docs/c/docs/index.mddocs/docs/c/index.mddocs/docs/cpp/api-reference.mddocs/docs/cpp/docs/index.mddocs/docs/cpp/index.mddocs/docs/internal/bindings/c/api-reference.mddocs/docs/js/docs/index.mddocs/docs/js/index.mddocs/scripts/doxygen.jsdocs/static/docs/c/api/.gitkeepdocs/static/docs/cpp/api/.gitkeepdoxygen/Doxyfile.basepackages/js/tsconfig.typedoc.json
💤 Files with no reviewable changes (2)
- core/Doxyfile
- bindings/c/Doxyfile
✅ Files skipped from review due to trivial changes (5)
- docs/docs/c/docs/index.md
- docs/docs/cpp/index.md
- docs/docs/js/index.md
- docs/static/docs/cpp/api/.gitkeep
- docs/docs/cpp/docs/index.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/CMakeLists.txt (1)
25-30:⚠️ Potential issue | 🟠 MajorInstall rule misses Windows runtime artifact for shared builds.
Line 25 installs
LIBRARY/ARCHIVE, but ifImg2Numis built as shared, Windows.dllis a runtime artifact and won’t be installed withoutRUNTIME DESTINATION.🔧 Suggested fix
install(TARGETS Img2Num EXPORT Img2NumTargets + RUNTIME DESTINATION lib LIBRARY DESTINATION lib ARCHIVE DESTINATION lib INCLUDES DESTINATION include/img2num )#!/bin/bash # Verify whether Img2Num install rules include runtime artifact placement. # Expected: # 1) add_library(Img2Num ...) exists # 2) install(TARGETS Img2Num ...) should include RUNTIME DESTINATION when shared builds are supported. rg -n -C3 --type=cmake '\badd_library\s*\(\s*Img2Num\b' rg -n -C8 --type=cmake '\binstall\s*\(\s*TARGETS\s+Img2Num\b|RUNTIME DESTINATION|LIBRARY DESTINATION|ARCHIVE DESTINATION' rg -n -C3 --type=cmake '\bBUILD_SHARED_LIBS\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/CMakeLists.txt` around lines 25 - 30, The install rule for the target Img2Num omits RUNTIME DESTINATION so Windows DLLs from shared builds won't be installed; update the install(TARGETS Img2Num ...) invocation to include a RUNTIME DESTINATION (e.g., bin or appropriate runtime folder) alongside the existing LIBRARY and ARCHIVE destinations so runtime artifacts (.dll) are installed for shared builds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/components/C_Cpp_Page.jsx`:
- Around line 29-32: The CodeBlock in C_Cpp_Page.jsx uses an invalid Prism
language identifier language="build"; change this to a supported shell language
(e.g., language="bash" or language="shell") to enable proper syntax
highlighting. Locate the CodeBlock component instance (title="Initialise CMake
and build the project") and replace the language prop value "build" with "bash"
(or "shell") so Docusaurus/Prism will apply correct highlighting.
- Line 23: Heading text "Include an Img2num Header" uses inconsistent casing;
update the heading in the C_Cpp_Page component so "Img2num" becomes "Img2Num" to
match project branding (i.e., replace the string in the <h3> element that
currently reads "Include an Img2num Header" with "Include an Img2Num Header").
In `@docs/src/md/_partials/installing-git-submodule.md`:
- Around line 29-32: The fenced code block currently uses the wrong language tag
(`cmake`) for shell commands; update the block opener to use the shell/highlight
tag (e.g., change the backtick fence from ```cmake title="Initialise CMake and
build the project" to ```bash title="Initialise CMake and build the project") so
the lines `cmake -B build .` and `cmake --build build` are highlighted as shell
commands; keep the title intact and leave the block content unchanged.
---
Duplicate comments:
In `@core/CMakeLists.txt`:
- Around line 25-30: The install rule for the target Img2Num omits RUNTIME
DESTINATION so Windows DLLs from shared builds won't be installed; update the
install(TARGETS Img2Num ...) invocation to include a RUNTIME DESTINATION (e.g.,
bin or appropriate runtime folder) alongside the existing LIBRARY and ARCHIVE
destinations so runtime artifacts (.dll) are installed for shared builds.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/CMakeLists.txtdocs/docs/c/index.mddocs/docs/cpp/index.mddocs/docs/js/index.mddocs/src/components/C_Cpp_Page.jsxdocs/src/md/_partials/installing-git-submodule.mddocs/src/md/_partials/library-features.md
✅ Files skipped from review due to trivial changes (1)
- docs/src/md/_partials/library-features.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/docs/cpp/index.md
- docs/docs/c/index.md
Ryan-Millard
force-pushed
the
build/packaging
branch
from
d4e4575 to
2b7f368
Compare
Removed section detailing repository contents and added Skillicons for technology icons.
Ryan-Millard
force-pushed
the
build/packaging
branch
from
3391393 to
69fce63
Compare
Contributor
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
This comment was marked as resolved.
This comment was marked as resolved.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
Contributor
|
Agent ran but didn't generate any test files. Tests may already exist or changes don't require additional tests. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
Owner
Author
|
@coderabbitai, this is a monolithic file that will prove to be problematic from an architectural standpoint in the future. Please turn this into multiple smaller GitHub Actions files so these can run as workflows for the appropriate triggers. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Refactoring Plan: Multiple GitHub Actions WorkflowsI'll create separate workflow files for each test category:
Each workflow will:
These workflows provide:
🧠 Learnings used💡 Use |
Owner
Author
@CodeRabbit, you did not refactor anything or offer any valuable addition. Please create a PR for this so I can check if your GitHub Action proposition is worthwhile. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Task: Create Modular GitHub Actions WorkflowsI'll create 6 separate workflow files to replace the monolithic test approach:
This will create 6 separate, focused GitHub Actions workflows that provide better modularity and maintainability compared to a monolithic approach. Each workflow runs only when relevant files change, and they can all run in parallel for faster feedback. 🧠 Learnings used💡 Use |
Owner
Author
@coderabbitai where did you put those files? I don't see a pull request. |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
Plan: Create Modular GitHub Actions WorkflowsI'll create 6 separate GitHub Actions workflow files in the
This will now trigger the code generation agent to create these 6 workflow files. Once they're created, the changes will be ready for you to review and merge! 🧠 Learnings used💡 Use |
This was referenced Apr 29, 2026
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
1 participant
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.
What was changed & why
A simple CMake update to enable better packaging.
Prepares the project for v0.0.0
Fixes: #264
Changes
Nothing breaking.
Shuffled some build pieces around for better library management & refactored WASM build.
Testing & Verification
Code runs. Unfortunately, we don't have tests yet.
Additional Resources
To run it, you'll also need to update the STB submodule:
Summary by CodeRabbit
Chores
New Features
Changed
Removed Features
Documentation