nuttxpr commented on PR #14833: URL: https://github.com/apache/nuttx/pull/14833#issuecomment-2481821314
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) This PR does *not* fully meet the NuttX requirements. While it provides some information, it lacks crucial details and context, making it difficult to review effectively. Here's a breakdown of the issues and how to fix them: **Summary:** * **Insufficient explanation:** Simply listing reverted commits doesn't explain *why* the revert is necessary. What problems did those commits introduce? What was the original intention of those commits, and why did they fail? What specific functionality is being restored by this revert? Explain the "writable romfs" feature and why removing it is beneficial. * **Missing context:** What use cases were affected by the buggy writable romfs implementation? This helps reviewers understand the impact and importance of the revert. * **No issue references:** Are there any related NuttX issues that prompted this revert? If so, link them here. **Impact:** * **Vague descriptions:** Saying "romfs, remove the code configed by FS_ROMFS_WRITEABLE" isn't helpful. Be explicit. Will users notice any functional changes? Will any configurations need to be adjusted? Will any applications break? If the answer to any impact question is "YES", *explain why and how*. For example: * **Impact on user:** YES. Users who were relying on the writable romfs feature will no longer be able to modify files in the romfs filesystem. They will need to use a different filesystem (e.g., littlefs) if write access is required. * **Impact on build:** Possibly YES. If any build configurations depended on `FS_ROMFS_WRITEABLE`, they will need to be updated. Explain how. * **Consider all impact categories:** Even if the answer is "NO", explicitly state it for each category (e.g., "Impact on security: NO"). This shows you've considered each aspect. **Testing:** * **Insufficient detail:** "test about romfs" is far too vague. What specific tests were run? Provide commands used, expected output, and actual output. * **Missing logs:** Include *actual* log output before and after the change, demonstrating the fix. Don't just leave placeholder comments. * **Missing build/target information:** Specify the *exact* build host (OS version, compiler version) and target (architecture, board, configuration) used for testing. For example: * Build Host: Linux (Ubuntu 22.04), GCC 11.3.0 * Target: sim:nsh **Example of an improved Summary:** > This PR reverts a series of commits (list commits here) that introduced a writable romfs implementation. This feature proved to be unstable and caused [describe the specific problems, e.g., data corruption, system crashes]. Reverting these commits restores the original read-only behavior of romfs. This addresses issue #[NuttX issue number, if applicable]. The writable romfs feature was intended to [explain the original goal], but due to [explain the reason for failure], it was deemed unsuitable. This revert removes the `FS_ROMFS_WRITEABLE` configuration option and associated code. By providing more specific information and context in each section, you'll make it much easier for reviewers to understand your changes, assess their impact, and confidently approve your PR. Remember to follow the NuttX Contributing Guidelines carefully. -- 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
