nuttxpr commented on PR #16196: URL: https://github.com/apache/nuttx/pull/16196#issuecomment-2798684235
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. This PR review assesses the CMake build system introduction for the Raspberry Pi Pico against the NuttX requirements. **Summary:** The summary is well-written and explains the purpose, affected code areas, and implementation approach clearly. The inspiration note is helpful. **Impact:** The impact assessment is mostly thorough. However, the "Impact on Documentation" should be YES, even if demonstrating general CMake usage exists. This PR introduces a *specific board's* CMake build, requiring at least a mention in the board's documentation. Clarify that existing general CMake documentation is *relevant* but not *sufficient*. **Testing:** The testing section needs improvement. * **Incomplete Build Host Information:** Specify the toolchain version (arm-none-eabi-gcc *version*). WSL version is also relevant. * **Incomplete Target Information:** "nsh configuration" is too vague. Specify the exact `.config` file or the steps to generate it. Are you using the default `defconfig`? * **Build Errors:** The build clearly failed. A PR should ideally not have failing builds. The identified undefined macros are a significant issue. Investigate *why* these macros aren't defined. Are they missing from `defconfig`? Is the CMake build process not correctly incorporating the configuration? Simply stating "I tried adding these" is insufficient; detail *where* and *how* you tried to add them. Provide the changes you attempted. * **"CMake is working well" Contradiction:** The logs show a failing build. You can't claim CMake is working well if it doesn't produce a working binary. The build errors must be resolved before this PR is ready. * **Lack of "Before Change" Context:** The "before change" log simply shows CMake failing because the Pico's CMake files didn't exist. This is expected and doesn't provide a useful comparison. Instead, describe how the Pico was built *before* this change (likely Make-based). A successful Make-based build log would be helpful as a baseline. * **Missing Functional Testing:** Even if the build succeeded, you need to demonstrate that the resulting binary functions correctly on the Pico. Provide logs or other evidence of successful operation after the CMake build. **Does this PR meet the NuttX Requirements?** No. While the PR makes good progress, the failing build and incomplete testing prevent it from meeting the requirements. The following actions are needed: 1. **Fix the build errors:** Investigate and resolve the undefined macro issue. Document the solution clearly. 2. **Complete the testing:** * Provide full build host and target details. * Include a successful Make-based build log ("before change"). * Demonstrate successful flashing and operation of the CMake-built binary on the Pico. 3. **Update documentation:** Mention the new CMake build option in the relevant Pico documentation. By addressing these points, the PR will have a much better chance of being accepted. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
