nuttxpr commented on PR #14892: URL: https://github.com/apache/nuttx/pull/14892#issuecomment-2491429457
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR appears to mostly meet the NuttX requirements, but there are some areas that could be improved for clarity and completeness. **Strengths:** * **Clear Problem Description:** The summary clearly explains the issue with `arm-none-eabi-ar` argument length limitations on Windows and the subsequent compilation error related to `nuttx/config.h`. * **Solution Provided:** The proposed workaround of overwriting `CMAKE_${lang}_RESPONSE_FILE_FLAG` is described. * **Testing Evidence:** Testing logs and a link to GitHub Actions workflows are provided, showing successful builds on both Linux and Windows (msys2). * **References:** Related NuttX issues are linked. **Areas for Improvement:** * **Summary - What functional part is changed?:** Be more specific than "Arm toolchain for Windows". Specify that this changes CMake configuration related to response file generation. * **Summary - How does the change exactly work?:** Explain *why* overwriting `CMAKE_${lang}_RESPONSE_FILE_FLAG` with `$DEFINES $INCLUDES $FLAGS` resolves the issue. What is different about this approach that avoids the problem? * **Impact - on build:** While you say it remains the same, the *internal* build process *is* changing. Clarify that the user-facing process is the same but the CMake configuration is modified. * **Impact - on hardware:** Specify the target architecture (Arm) that this change affects. It's implicit, but stating it explicitly is helpful. * **Testing - Before Change Logs:** Provide logs demonstrating the original error. This is crucial for showing the effectiveness of the fix. * **Testing - More Detail:** "local msys2" isn't very descriptive. Specify the msys2 environment (e.g., MSYS2 MinGW 64-bit) and its version. Also, include the full CMake configuration command used. Consistency between local and CI testing is important. The CI shows `-DBOARD_CONFIG=nucleo-l152re:nsh`, but the local test doesn't mention this. Ensure they're the same. * **Consider Alternatives:** Investigate why `CMAKE_NINJA_FORCE_RESPONSE_FILE=ON` isn't sufficient. Is there a way to address the root cause instead of working around it? This might involve changes to the NuttX build system itself rather than just the CI configuration. Even if you ultimately stick with the current workaround, documenting the investigation would strengthen the PR. **Revised Impact Section Example (Illustrative):** * **Impact on user:** NO. Users should not experience any changes in functionality or build procedures. * **Impact on build:** YES. While the user-facing build process remains the same, this PR modifies the internal CMake configuration by overwriting `CMAKE_${lang}_RESPONSE_FILE_FLAG` to work around an argument length limitation with `arm-none-eabi-ar` on Windows. * **Impact on hardware:** YES. This specifically affects builds targeting Arm architectures when using an x86 Windows host with the MSYS2 environment. * **Impact on documentation:** NO. (Or YES if you update any documentation). * ... (rest of impact section) By addressing these points, the PR will be more comprehensive and easier for reviewers to understand and approve. Providing clear "before" and "after" examples, explaining the *why* behind the change, and providing more detail about the testing environment will significantly improve the quality of the PR. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
