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

rv-virt/python: Enable Python's socket module on defconfig #15457

Merged

Conversation

tmedicci
Copy link
Contributor

@tmedicci tmedicci commented Jan 7, 2025

Summary

  • Documentation: Update Python documentation for socket module

Update Python documentation after supporting Python's socket module to reflect the commands used to run the RISC-V QEMU.

  • rv-virt/python: Enable Python's socket module on defconfig

Enables the Python's socket module. This allows applications to be built to interact with POSIX-compatible sockets.

Impact

Update rv-virt:python defconfig to be used with Python's socket module introduced by apache/nuttx-apps#2928. Documentation was updated to reflect those changes.

Testing

Internal CI testing + rv-virt:python (testing instructions are the same of apache/nuttx-apps#2928)

Enables the Python's socket module. This allows applications to be
built to interact with POSIX-compatible sockets.
Update Python documentation after supporting Python's socket module
to reflect the commands used to run the RISC-V QEMU.
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Board: risc-v Size: M The size of the change in this PR is medium labels Jan 7, 2025
@nuttxpr
Copy link

nuttxpr commented Jan 7, 2025

[Experimental Bot, please feedback here]

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

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the changes. Referencing the related NuttX Apps PR is helpful.
  • Impact Description: The impact section covers several areas and explains the changes.
  • Testing Performed: Confirmation of testing and referencing prior testing instructions is good.

Areas for Improvement (to make it even better):

  • Explicit "Yes/No" Answers: While the impact description is good, explicitly stating "YES" or "NO" at the beginning of each impact subsection would improve readability and ensure all points are addressed. For example:
    • Impact on user: YES (Users will need to adapt by using the updated commands for the RISC-V QEMU as documented.)
    • Impact on build: YES (The rv-virt:python defconfig is modified.)
    • Impact on hardware: NO
    • Impact on documentation: YES (Documentation is updated to reflect the changes in the QEMU commands and the availability of the socket module.)
    • Impact on security: NO (Assuming no security implications were introduced. If there are any, they MUST be documented)
    • Impact on compatibility: Potentially YES (if any existing scripts rely on previous QEMU behavior). Clarify this point.
  • Testing Logs: While referencing the other PR's testing instructions is acceptable, including some example logs (even abbreviated ones) directly in the PR description is strongly recommended. This provides immediate context and evidence of successful testing.
  • Build Host Details: Be more specific about the Build Host(s) used for testing. For example: "Linux, x86_64, GCC 12.2.1"
  • Target Details: Be more explicit with the Target(s) tested: "sim:rv-virt:python"

By addressing these minor points, the PR will be even clearer and easier to review.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @tmedicci amazing work!! :-)

Lets just wait for the CI to complete :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 154a91c into apache:master Jan 8, 2025
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Documentation Improvements or additions to documentation Board: risc-v Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants