nuttxpr commented on PR #14747: URL: https://github.com/apache/nuttx/pull/14747#issuecomment-2470721392
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR does not fully meet the NuttX requirements. Here's why: * **Insufficient Testing Information:** The testing section is inadequate. Saying "modified the tests in local apps and it's feasible" isn't enough. You MUST provide: * Specific build hosts used (OS, CPU, Compiler version). * Specific target(s) (architecture, board, configuration). * Actual log output *before* the change. * Actual log output *after* the change demonstrating the improvement. This is crucial for reviewers to verify the change works as intended. * **Vague Impact Description:** While you mention forward compatibility and no backward compatibility issues, you need to be more specific. Does this change affect *any* existing CMakeLists.txt files? If not, explicitly state that. If so, how many and will they need modification? * **Missing Issue References:** Are there any related NuttX or NuttX Apps issues that this PR addresses? If so, link them. If not, explicitly state "N/A". * **Clarity on "nuttx_cmake module":** Is this a completely *new* module? Or modifications to an existing one? Be explicit. The phrasing "wrapped CMake native method" is unclear. Explain what native methods are being wrapped and why. Here's how you can improve the PR description: ## Example of Improved PR Description ## Summary [Enhancement] This PR introduces a new CMake module, `nuttx_cmake`, to simplify and modernize the NuttX build system. This module provides helper functions that wrap common CMake commands, reducing boilerplate code and improving readability. Specifically, it provides functions like `nuttx_library_ifdef`, `nuttx_wildcard_sources`, `nuttx_sources_ifdef`, `nuttx_compile_options`, and `nuttx_compile_options_ifdef`. This simplifies the creation and configuration of libraries and applications within the NuttX build system, as demonstrated in the example below. **Example:** ```cmake # Before: apps/libfoo/CMakeLists.txt # ... (lengthy CMake code) ... # After: apps/libfoo/CMakeLists.txt nuttx_library_ifdef(CONFIG_LIB_FOO foo) # ... (simplified CMake code) ... ``` Related NuttX Issue: N/A Related NuttX Apps Issue/PR: N/A ## Impact * **New Feature Added:** Yes, the `nuttx_cmake` module. * **Impact on User:** YES. Users can now leverage the simplified CMake commands in their `CMakeLists.txt` files. While existing CMakeLists.txt files *should* continue to work without modification, users are encouraged to adopt the new functions for improved maintainability. (Note: Quantify if any existing files *must* be modified). * **Impact on Build:** YES. The build system now includes the `nuttx_cmake` module. * **Impact on Hardware:** NO. * **Impact on Documentation:** YES. Documentation updates are required to explain the new CMake module and its usage. This documentation will be provided as part of this PR. (Specify where the documentation is). * **Impact on Security:** NO. * **Impact on Compatibility:** Forward compatible. Backward compatible with existing CMakeLists.txt files (Clarify this point thoroughly). * **Anything else to consider:** N/A ## Testing I confirm that changes are verified on my local setup and work as intended: * **Build Host(s):** Ubuntu 22.04, x86_64, GCC 11.3.0 * **Target(s):** `sim:nsh` **Testing logs before change (apps/examples/hello/CMakeLists.txt):** ``` # (Paste actual build log output here) ``` **Testing logs after change (apps/examples/hello/CMakeLists.txt):** ``` # (Paste actual build log output showing the effect of the change, e.g., reduced build time or simplified log messages) ``` By providing concrete details and following the NuttX requirements carefully, you will increase the likelihood of your PR being accepted. Remember to replace the example details with your specific information. -- 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]
