nuttxpr commented on PR #15393: URL: https://github.com/apache/nuttx/pull/15393#issuecomment-2566089495
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR description mostly meets the NuttX requirements, but could be slightly improved. **Strengths:** * **Clear Summary:** Explains the "why" (broken defconfig), "what" (updating defconfig), and "how" (using `refresh.sh`). The link to the related PR is helpful. * **Specific Impact:** Clearly identifies the impact on CI and the specific job affected. * **Detailed Testing:** Provides links and log snippets to demonstrate the before/after behavior. The Gist for "after" is especially useful. The explanation of the remaining `sim:rpproxy_virtio` failure is also important context. **Areas for Improvement:** * **Summary: Functional Area:** While it mentions defconfig, it could be more specific about the functional area of the code. E.g., "Build System (defconfig for sim:rpserver_virtio)". * **Impact: Be Explicit with NOs:** Instead of just mentioning the one impact, explicitly state "NO" for all other impacts (build process, hardware, documentation, security, compatibility). This makes it easier for reviewers to quickly assess the scope of the change. * **Testing: Local Setup:** While you provide CI logs, the "Build Host(s)" and "Target(s)" sections aren't filled out. Even if testing is primarily done in CI, mention the local environment where the `refresh.sh` script was run. Something like: "Build Host: Linux, x86_64, GCC (Docker image ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest)". Target is "sim". * **Testing Logs: Conciseness:** The "after" log is very short, which is good. The "before" log could be trimmed down to the essential parts showing the failure. The important part is the diff showing the missing config options. **Suggested Revision (incorporating the above improvements):** ## Summary This PR fixes the build system by updating the defconfig for `sim:rpserver_virtio`, which was broken by changes introduced in https://github.com/apache/nuttx/pull/15336. This update is performed using the `refresh.sh` script. ## Impact * Is new feature added? NO * Is existing feature changed? NO * Impact on user (will user need to adapt to change)? NO * Impact on build (will build process change)? NO * Impact on hardware (will arch(s) / board(s) / driver(s) change)? NO * Impact on documentation (is update required / provided)? NO * Impact on security (any sort of implications)? NO * Impact on compatibility (backward/forward/interoperability)? NO * Anything else to consider? This PR fixes the defconfig check for `sim:rpserver_virtio` in CI Job sim-03. CI Job sim-03 still fails due to `sim:rpproxy_virtio`, which will be addressed in a separate PR. ## Testing I confirm that changes are verified and works as intended: * Build Host(s): Linux, x86_64, GCC (Docker image ghcr.io/apache/nuttx/apache-nuttx-ci-linux:latest) * Target(s): sim Testing logs before change (showing missing config options): ``` < CONFIG_BLK_RPMSG_SERVER=y < CONFIG_CLK_RPMSG=y ... (other missing options) ``` Testing logs after change (showing successful normalization): ```bash Configuration/Tool: sim/rpserver_virtio Normalize sim/rpserver_virtio ``` By making these small adjustments, the PR becomes even clearer and easier for reviewers to process. It explicitly addresses all the required points in a concise and organized way. -- 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
