Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

boards: Replace CONFIG_NSH_LINELEN with CONFIG_LINE_MAX #15541

Conversation

JianyuWang0623
Copy link
Contributor

Summary

boards: Replace CONFIG_NSH_LINELEN with CONFIG_LINE_MAX
Details: apache/nuttx-apps#2943
1. find boards/ -name defconfig | xargs sed -i 's/CONFIG_NSH_LINELEN/CONFIG_LINE_MAX/g'
2. ./tools/refresh.sh --silent --defaults all

Impact

boards/*

Testing

  1. Selftest with refresh.sh
  2. CI

Details: apache/nuttx-apps#2943

1. find boards/ -name defconfig | xargs sed -i 's/CONFIG_NSH_LINELEN/CONFIG_LINE_MAX/g'
2. ./tools/refresh.sh --silent --defaults all

Signed-off-by: wangjianyu3 <[email protected]>
@nuttxpr
Copy link

nuttxpr commented Jan 14, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although some sections could be more detailed.

Here's a breakdown with suggestions for improvement:

  • Summary: While the "what" is clear (replacing CONFIG_NSH_LINELEN with CONFIG_LINE_MAX), the why is missing. Explain the reason for this change. Is CONFIG_NSH_LINELEN deprecated? Does CONFIG_LINE_MAX offer advantages? Mentioning the related NuttX Apps PR is good.

  • Impact: Simply stating "boards/*" is insufficient. Be explicit about the impact. Does this change affect all boards? While it likely doesn't impact the user, build process, or hardware functionality directly, explicitly stating "NO" with brief justifications for each (e.g., "Build: NO - Only changes configuration, not build process itself") strengthens the PR. Documentation: YES if any documentation refers to CONFIG_NSH_LINELEN; otherwise NO. Security, Compatibility: Likely NO, but state it explicitly.

  • Testing: "Selftest with refresh.sh" is vague. What does this selftest entail? What were you checking? Provide more specific commands and expected outcomes. "CI" is also insufficient. Which CI checks were run and passed? Ideally, provide links to successful CI runs. While before/after logs aren't strictly necessary for a configuration change like this, providing snippets demonstrating the change in behavior (e.g., before: line limited to X characters; after: line limited to Y characters) would further enhance the PR.

Example of improved Impact and Testing sections:

Impact:

  • Is new feature added? NO
  • Is existing feature changed? YES (NSH line length configuration now uses a standardized setting)
  • Impact on user: NO (Users should not experience any functional changes unless they were relying on a non-default CONFIG_NSH_LINELEN setting. The new CONFIG_LINE_MAX provides a consistent experience.)
  • Impact on build: NO (Only configuration files are changed, not the build process itself)
  • Impact on hardware: NO (No hardware changes are involved)
  • Impact on documentation: YES (Documentation referencing CONFIG_NSH_LINELEN needs updating to CONFIG_LINE_MAX)
  • Impact on security: NO
  • Impact on compatibility: NO (Should be backward compatible as long as CONFIG_LINE_MAX provides sufficient line length)
  • Anything else to consider? None

Testing:
Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
Target: sim:qemu-x86_64

Tested the change by building and running NSH on the qemu-x86_64 target.
Before the change, the maximum line length in NSH was limited by CONFIG_NSH_LINELEN.
After the change, the maximum line length is now controlled by CONFIG_LINE_MAX.

Verification steps:

  1. Built NuttX for qemu-x86_64 with the original configuration (using CONFIG_NSH_LINELEN).
  2. Ran NSH and observed the line length limit.
  3. Applied the patch.
  4. Rebuilt NuttX for qemu-x86_64 with the new configuration (using CONFIG_LINE_MAX).
  5. Ran NSH and verified that the line length limit now corresponds to CONFIG_LINE_MAX.

CI: All CI checks passed. [Link to CI run]

By adding these clarifications, the PR becomes more robust and easier for reviewers to understand and approve.

@xiaoxiang781216 xiaoxiang781216 added the breaking change This change requires a mitigation entry in the release notes. label Jan 14, 2025
@xiaoxiang781216 xiaoxiang781216 merged commit f3b31ef into apache:master Jan 14, 2025
39 checks passed
JianyuWang0623 added a commit to JianyuWang0623/nuttx-apps that referenced this pull request Jan 15, 2025
JianyuWang0623 added a commit to JianyuWang0623/nuttx that referenced this pull request Jan 15, 2025
JianyuWang0623 added a commit to JianyuWang0623/nuttx-apps that referenced this pull request Jan 15, 2025
NSH_LINELEN is replaced by POSIX standard LINE_MAX.

apache/nuttx#15541
apache#2943

Signed-off-by: wangjianyu3 <[email protected]>
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Jan 16, 2025
NSH_LINELEN is replaced by POSIX standard LINE_MAX.

apache/nuttx#15541
#2943

Signed-off-by: wangjianyu3 <[email protected]>
JianyuWang0623 added a commit to JianyuWang0623/openvela-nuttx-apps that referenced this pull request Jan 16, 2025
NSH_LINELEN is replaced by POSIX standard LINE_MAX.

apache/nuttx#15541
apache/nuttx-apps#2943

Signed-off-by: wangjianyu3 <[email protected]>
JianyuWang0623 added a commit to JianyuWang0623/openvela-nuttx-apps that referenced this pull request Jan 18, 2025
NSH_LINELEN is replaced by POSIX standard LINE_MAX.

apache/nuttx#15541
apache/nuttx-apps#2943

Signed-off-by: wangjianyu3 <[email protected]>
xiaoxiang781216 pushed a commit to open-vela/nuttx-apps that referenced this pull request Jan 18, 2025
NSH_LINELEN is replaced by POSIX standard LINE_MAX.

apache/nuttx#15541
apache/nuttx-apps#2943

Signed-off-by: wangjianyu3 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants