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

i486/Toolchain.defs: move toolchain releated option to Toolchain.defs #14640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

CV-Bowen
Copy link
Contributor

@CV-Bowen CV-Bowen commented Nov 4, 2024

Summary

Toolchain related options should be in Toolchain.defs

Impact

Only core refactor and minor changes, should be no impact

Testing

qmeu-i486:nsh

Toolchain related options should be in Toolchain.defs

Signed-off-by: Bowen Wang <[email protected]>
@github-actions github-actions bot added Arch: x86 Issues related to the x86 architecture Board: x86 Size: S The size of the change in this PR is small labels Nov 4, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements in form, but lacks crucial details. While it follows the template, it provides insufficient information to properly assess the changes.

Here's why it's incomplete and what's missing:

  • Summary: "Toolchain related options should be in Toolchain.defs" is a statement of intent, not a summary. What options were moved? Why were they moved? What problem did this solve? What files were modified?
  • Impact: While it claims "no impact," this needs justification. Even refactoring can have unintended consequences. More thorough testing and explanation are needed. The claim of "minor changes" is vague.
  • Testing: "qemu-i486:nsh" is insufficient. What tests were run? Simply stating the target platform and shell doesn't demonstrate functionality. Show specific commands and their output before and after the change. The "Testing logs" sections are empty. These need concrete examples demonstrating the change's effect (or lack thereof).

In short, the PR author needs to provide more specifics. They need to show, not just tell, what changed and how it was tested. While using the template is a good start, it's not enough without the supporting details.

@xiaoxiang781216
Copy link
Contributor

@CV-Bowen please fix ci error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: x86 Issues related to the x86 architecture Board: x86 Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants