nuttxpr commented on PR #15196: URL: https://github.com/apache/nuttx/pull/15196#issuecomment-2544299683
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR appears to meet the basic NuttX requirements, but could be improved. Here's a breakdown: **Strengths:** * **Clear Summary of "Why":** The rationale for using SPDX identifiers for SBOM generation is clearly stated. * **Concise Impact Description:** Highlighting "SBOM" as the impact area is direct. * **Testing Mention:** Referencing CI indicates testing is being performed. **Weaknesses & Suggestions for Improvement:** * **Missing Details in Summary:** * **"What functional part is changed?":** Be specific. E.g., "The license headers/files," or "A new Kconfig option," or "The build system." * **"How does the change work?":** Explain the *mechanism*. E.g., "A new SPDX identifier `NuttX-PublicDomain` is defined and used in all applicable files," or "A script is added to automatically generate SPDX tags." * **Issue References:** If this relates to a specific issue (even a self-filed one), link it. * **Insufficient Impact Assessment:** * **User Impact:** While likely minimal, explicitly state "NO" or briefly explain any potential effects. * **Build Impact:** "NO" or explain any changes to the build process (e.g., new dependencies, additional steps). * **Hardware Impact:** "NO" (presumably). * **Documentation Impact:** "YES" if any documentation needs updating to reflect the SPDX changes or if documentation about the SBOM generation process is added. Explain the changes. * **Security Impact:** "NO" or justify. The legal discussion about Public Domain vs. licensing is important context but doesn't directly address the security implications of this specific change. * **Compatibility Impact:** "NO" or explain any potential breaks. * **Inadequate Testing Description:** * **Build Hosts:** List the specific OS, CPU architecture, and compiler versions used for testing. * **Targets:** Specify the architectures and board configurations tested (e.g., `sim:nsh`, `qemu-cortexm3:hello`). * **Logs:** The "CI" reference is insufficient. While full logs might be excessive, provide snippets or summaries of the relevant output demonstrating the change's effect (e.g., "SBOM generation tool now correctly identifies NuttX components"). If the CI has public logs, link to them. **Example of Improved Testing Section:** ``` Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux (Ubuntu 22.04), x86_64, GCC 11.3.0 * Target(s): sim:nsh, qemu-cortexm3:hello Testing logs before change (SBOM generation with `tool_name`): ``` Component "NuttX" identified as "NOASSERTION" ``` Testing logs after change (SBOM generation with `tool_name`): ``` Component "NuttX" identified as "NuttX-PublicDomain" ``` (See also CI logs: [link to CI run]) ``` By adding these details, the PR will be much stronger and easier for reviewers to evaluate. -- 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]
