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

Move debug_info target implementation to apps #2924

Closed

Conversation

eren-terzioglu
Copy link
Contributor

Summary

Related to apache/nuttx#15304
Move debug_info target implementation into apps. Update suggested from apache/nuttx#15304 (comment)

Impact

Common layer update

Testing

Any config can be used after configuration step with make debug_info command

@nuttxpr
Copy link

nuttxpr commented Dec 30, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. While it provides a summary and links a related PR, it lacks specifics in several areas.

Here's a breakdown of what's missing and how to improve it:

  • Summary: While it mentions moving the debug_info target, it doesn't explain why this move is necessary. What problem does it solve? What are the benefits? It also doesn't describe how the change works in detail. What code was moved? What mechanisms are used to integrate it into apps?

  • Impact: "Common layer update" is too vague. Be specific. While it might not impact the user directly, it does impact the build process (since the target is moved). This needs a "YES" and a description of how the build process changes. Does the user need to do anything differently? Are there new dependencies? Consider other impacts as well. Does this affect any specific architectures, boards, or drivers? Does it have security implications (e.g., by exposing more information)? Does it impact compatibility with existing scripts or tools?

  • Testing: The testing section is inadequate. Simply stating "Any config can be used" isn't enough. You need to provide concrete examples of the configurations you tested. List the specific:

    • Build Host(s): e.g., "Linux, x86_64, GCC 12.2.0"
    • Target(s): e.g., "sim:nsh", "stm32f4discovery:hello"
    • Testing logs: Include actual logs, both before and after the change. Demonstrate that the debug_info target works as expected in the new location. Show the output of the command, and what information it provides. Ideally, show how the output differs (or is consistent) between the old and new implementations.

In short, while the PR touches on the required sections, it lacks the necessary detail and concrete evidence to demonstrate the change's validity and impact. Providing this information will significantly improve the review process and increase the likelihood of acceptance.

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.

2 participants