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

interpreters/python: fix patch to set _PyRuntime attribute #2992

Merged

Conversation

tmedicci
Copy link
Contributor

Summary

  • interpreters/python: fix patch to set _PyRuntime attribute

This commit also adds the check for the __NuttX__ macro to the patch file that allows setting an attribute to the _PyRuntime structure. The __NuttX__ macro is guaranteed to be present when building any application for NuttX.

Impact

Impact on user: YES. Local changes to the Python application can use the __NuttX__ macro.

Impact on build: YES. Local changes to the Python application can use the __NuttX__ macro to enable/disable features.

Impact on hardware: NO

Impact on documentation: NO

Impact on security: NO

Impact on compatibility: NO

Testing

It can be tested with rv-virt:python (the only board currently supporting Python) by checking if the attribute is set to the _PyRuntime structure.

Building

Build it with:

make -j distclean && ./tools/configure.sh rv-virt:python && make -j$(nproc)

Results

Check the generated nuttx.map file: it should shows that the _PyRuntime structure is at section .PyRuntime:

 .PyRuntime     0x80d70078    0x247a8 /home/tiago/Documents/work/espressif/projects/nuttx/nuttxspace_alfa/apps/interpreters/python/install/target/libpython3.13.a(pylifecycle.o)
                0x80d70078                _PyRuntime

This commit also adds the check for the `__NuttX__` macro to the
patch file that allows setting an attribute to the `_PyRuntime`
structure. The `__NuttX__` macro is guaranteed to be present when
building any application for NuttX.
@nuttxpr
Copy link

nuttxpr commented Feb 12, 2025

[Experimental Bot, please feedback here]

Yes, this PR mostly meets the NuttX requirements, but could be improved. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change.
  • Impact Assessment: The impact section addresses all the required points, even if briefly.
  • Testing Information: Provides build instructions and a method to verify the change.

Weaknesses:

  • Missing Issue References: The summary should link to related NuttX issues if applicable. Even if there isn't a directly related issue, consider creating one to track the bug or feature request this PR addresses.
  • Vague Impact Descriptions: While the impact section checks the boxes, the explanations are too brief. For "Impact on user" and "Impact on build," explain how the user benefits from using the __NuttX__ macro. What features can they enable/disable? Provide a concrete example.
  • Insufficient Testing Logs: The "Testing logs before change" and "Testing logs after change" sections are empty. You should include actual output demonstrating the change in behavior. Show what happens before the patch and after the patch. The nuttx.map output is helpful, but it doesn't demonstrate the functional impact of the change. Consider adding Python code that uses the __NuttX__ macro and show its output before and after the change.
  • Limited Testing Scope: Testing is only mentioned for rv-virt:python. If Python is supported on other platforms, ideally, those should also be tested. If not feasible, explain why.

Recommendations:

  1. Add Issue References: If no issue exists, create one and link it to the PR. This provides valuable context and traceability.
  2. Elaborate on Impact: Provide specific examples of how users and the build process are affected. For instance, "Users can now use the __NuttX__ macro within their Python applications to conditionally include NuttX-specific code, such as accessing hardware peripherals. This allows for more portable Python code that can adapt to different environments." For build impact, mention something like, "The build system now checks for the __NuttX__ macro when compiling Python applications, allowing users to define this macro to enable or disable optional features during compilation."
  3. Include Real Testing Logs: Show the actual output of relevant commands or programs before and after the change. This provides concrete evidence that the change works as intended. Include Python code snippets and their output demonstrating the use of __NuttX__.
  4. Broaden Testing (If Possible): Test on other platforms that support Python if feasible. If not, justify why testing is limited to rv-virt:python.

By addressing these weaknesses, the PR will be significantly stronger and more likely to be accepted.

Copy link

@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 on Python and perfect reporting, respect! :-)

@tmedicci
Copy link
Contributor Author

We already have 4 reviewers and no pending questions, can it be merged?

@lupyuen lupyuen merged commit db8542d into apache:master Feb 14, 2025
37 checks passed
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.

6 participants