nuttxpr commented on PR #2856: URL: https://github.com/apache/nuttx-apps/pull/2856#issuecomment-2481152735
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) No, this PR description does not fully meet the NuttX requirements. While it addresses some aspects, it lacks crucial details and context. Here's a breakdown of what's missing: * **Summary:** * **Insufficient "Why":** Simply stating "add benchmarks osperf miss CMake build" doesn't explain *why* this change is necessary. Is it a new feature, a bug fix, a performance improvement? What problem does it solve? * **Missing "What/How":** What part of the code is changed? How do the benchmarks work? How was the CMake build fixed? Be specific. * **Missing References:** The PR mentions fixing a CI failure in #14756, but it should explicitly link to that PR using the markdown link format: `#14756`. * **Impact:** * **Incomplete Sections:** The PR uses the "fix ci fail" comment in the Impact section. This should be properly categorized within the Impact checklist. For example, it likely impacts the *build* process and potentially *compatibility*. Each "YES" answer *must* have a description. For example: * Impact on build: YES (Fixes a CMake build issue that was causing CI failures on qemu-armv8a/nsh_smp) * Impact on compatibility: Potentially YES (if the fix affects other boards or configurations, this needs to be stated). * **Missing Considerations:** Even if most impact categories are "NO," explicitly state "NO" and provide a brief justification. For instance: * Impact on user: NO (This is a build system change and does not affect user functionality). * Impact on hardware: NO (No changes to hardware support). * Impact on documentation: Maybe YES (If the CMake build change requires documentation updates, explain what needs to be updated). If NO, explain why. * **Testing:** * **Incomplete Information:** Only the build command is provided. This isn't sufficient. * **Missing Host Details:** Specify the build host OS, CPU architecture, and compiler version. * **Missing Target Details:** Be more precise about the target. "arch(sim,RISC-V,ARM,..)" isn't specific enough. For example: "QEMU emulating ARMv8-A (qemu-armv8a/nsh_smp config)". * **Missing Logs:** The template specifically requests "Testing logs before change" and "Testing logs after change." Include relevant log snippets that demonstrate the problem before the change and the successful outcome after the change. "refresh pass" is not a log. Show the CI output or relevant build/test output. **Example of an improved summary:** ``` ## Summary This PR fixes a CMake build issue that was causing CI failures for the osperf miss benchmarks on the qemu-armv8a/nsh_smp configuration (see #14756). Previously, the CMakeLists.txt file was missing [explain the missing part]. This change adds [explain what was added to the CMakeLists.txt] to correctly integrate the osperf miss benchmarks into the build process. ``` By providing more context, detail, and following the template closely, the PR will be much easier to review and understand. This leads to faster approvals and a higher quality codebase. -- 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]
