Re: [PR] [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code [nuttx-apps]
lupyuen merged PR #3057: URL: https://github.com/apache/nuttx-apps/pull/3057 -- 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]
Re: [PR] [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code [nuttx-apps]
Donny9 commented on PR #3057: URL: https://github.com/apache/nuttx-apps/pull/3057#issuecomment-2816693000 > Thanks @Donny9 :-) My remarks below (marked as RC until discussion / vote is resolved on the mailing list): > > * Is it really necessary to remove built-in nsh/dd? What are the benefits over breaking build compatibility with os and apps? @cederom Firstly, we need to ensure that only one implementation of dd is retained in NuttX, because having two implementations would increase complexity for developers in terms of choosing which one to use. Moreover, if there are two versions, it could lead to misalignment in dd functionality, resulting in more maintenance work and complaints. I understand that the original reason for introducing the built-app version of dd was that it allowed for independent stack space settings, making it convenient to observe the stack consumption by the file system(If the stack consumed by dd is excessively large, it will result in a larger stack for nsh, thereby reducing the available memory for the system. Therefore, a built-app approach would be preferable). In reality, there is no significant difference for developers between the built-app dd and the command-line dd. Furthermore, there are indeed many other commands in the system that exist as built-apps, such as tar, ssh, scp, sb, rb, etc. > It is impossible to keep both (i.e. nsh/dd as minimalistic and apps/system/dd as fully featured) as exclusive selections? > Why not extend nsh/dd with new features while keep the minimal as default? As mentioned above, having two implementations of dd is highly undesirable. As for the extent to which the simplified version of dd lacks functionality compared to the full version, it is difficult to gauge. > Aside question did NuttX consider `/usr/bin/dd` versus built-in `dd` (something like built-in `time` versus `/usr/bin/time` in Unix)? In my opinion, their main differences are as follows: 1. The startup methods differ. In the former case, after parsing via nsh_parse, dd is executed directly through a function call. In the latter case, a new task is initiated via posix_spawn, and the corresponding executable binary is executed to accomplish the functionality of dd. 2. The execution contexts differ. The former operates within the context of nsh, while the latter runs as an independent new task. > * Documentation needs an update in either case :-P Done -- 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]
Re: [PR] [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code [nuttx-apps]
lupyuen commented on PR #3057: URL: https://github.com/apache/nuttx-apps/pull/3057#issuecomment-2812628752 @Donny9 Please close the voting. Thanks! This Breaking PR compiles with the [NuttX Breaking Changes Handling Process](https://github.com/apache/nuttx/blob/master/CONTRIBUTING.md#113-breaking-changes-handling-process): - [Special CI Checks are successful](https://gist.github.com/search?q=user%3Anuttxpr+81afa9f7608fb522496ba042cde6eb7eaa5156f2&ref=searchresults) - [NuttX Dashboard for This PR](https://nuttx-dashboard.org/d/fe2bqg6uk7nr4a/build-logs-from-nuttx-build-farm?from=now-2d&to=now&timezone=browser&var-arch=$__all&var-subarch=$__all&var-board=$__all&var-config=$__all&var-group=$__all&var-Filters=user%7C%3D%7Cnuttxpr) - Tested OK with OSTest on Real Hardware: [Avaota-A1 A527 Arm64 SBC](https://gist.github.com/lupyuen/97ced6258f285b455e98ad9a6fdf88d5) and [Oz64 SG2000 64-bit RISC-V SBC](https://gist.github.com/lupyuen/d145764319afd64afb141df17065c064) - [No Blocking Binding Vote](https://lists.apache.org/thread/6oygfv6qnzgc600fnrj38xmzmb6bhq1v) - Please respond to the comments by @cederom: https://github.com/apache/nuttx-apps/pull/3057#pullrequestreview-2764483606 - When @cederom has Approved the PR: Please merge NuttX Apps PR, followed by NuttX Kernel PR. That's because: When we merge the NuttX Kernel PR, it will trigger the Daily Build. Which will fail unless NuttX Apps PR has already been merged. -- 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]
Re: [PR] [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code [nuttx-apps]
Donny9 commented on PR #3057: URL: https://github.com/apache/nuttx-apps/pull/3057#issuecomment-2811682623 > @cederom @lupyuen @Donny9 comparing both file with meld I can see they are basically the same code: > > ``` > alan@dev:~/nuttxspace/apps$ meld nshlib/nsh_ddcmd.c system/dd/dd_main.c > ``` > > But I think system/dd will evolve to be more complete and compatible with Linux/Unix over the time. > > For example, nsh_ddcmd.c has CONFIG_NSH_CMDOPT_DD_STATS that doesn't exist in system/dd, so it rings a bell that system/dd is more focused on compatibility vs code size. > > I think that CONFIG_NSH_CMDOPT_DD_STATS could be renamed to CONFIG_SYSTEM_DD_MINIMUM and included in other parts there that aren't essential for a simple dd command. Done~ rename CONFIG_NSH_CMDOPT_DD_STATS to CONFIG_SYSTEM_DD_STATS. -- 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]
Re: [PR] [BREAKING] system/dd: using system dd to instead nsh dd, avoid duplicate code [nuttx-apps]
pussuw commented on PR #3057: URL: https://github.com/apache/nuttx-apps/pull/3057#issuecomment-2809387680 This causes a regression when NSH_FILEAPPS=y, as the full path to /bin/dd must be provided. I'm okay with this, since I put /bin into PATH, but others might not. Just something to keep in mind. -- 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]
