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

cmake(build):add missing basic and nxlooper cmake script #2857

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

xuxin930
Copy link
Contributor

Summary

add interpreter basic and nxlooper missing CMake build

Impact

fix ci fail in https://github.com/apache/nuttx/actions/runs/11880169863/job/33103008538?pr=14756

Testing

cmake -B build -DBOARD_CONFIG=sim:minibasic -GNinja
cmake -B build -DBOARD_CONFIG=sim:alsa -GNinja

refresh pass

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@nuttxpr
Copy link

nuttxpr commented Nov 18, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it addresses some aspects, it lacks crucial details. Here's a breakdown of what's missing and how to improve it:

Missing Information in Summary:

  • Why is the change necessary? Simply stating "add interpreter basic and nxlooper missing CMake build" doesn't explain the underlying problem. Why were these CMake builds missing? Was it an oversight, a bug, a new feature request? Explain the motivation.
  • What functional part of the code is being changed? Be specific. Mention the affected files/modules (e.g., interpreter, nxlooper, CMakeLists.txt).
  • How does the change exactly work? Don't just say what changed, explain how it works. For example, "Added CMake build rules for the interpreter and nxlooper components by including the necessary source files and dependencies in the CMakeLists.txt files. This allows these components to be built as part of the standard NuttX build process."

Missing Information in Impact:

  • While the CI failure is mentioned, this is more of a testing detail than an impact assessment. Address all the specific impact points:
    • New feature? Existing feature changed? If this adds interpreter support, state that. If it just fixes a build issue, clarify.
    • Impact on user: Will users now have access to a new interpreter? Will any existing user workflows be affected?
    • Impact on build: YES. Describe how the build process changes. E.g., "The interpreter and nxlooper will now be built by default for the specified configurations."
    • Impact on hardware: Likely NO. But state this explicitly.
    • Impact on documentation: If the interpreter is a new feature, or if the build process changes for users, documentation updates are likely needed.
    • Impact on security: Consider if introducing the interpreter or changing the build could have security implications. Even if the answer is NO, state it explicitly.
    • Impact on compatibility: Likely NO, but state it explicitly.

Missing Information in Testing:

  • Build Host(s): Provide details about your build environment.
  • Target(s): "sim:minibasic" and "sim:alsa" are good starting points, but mention the architecture (e.g., sim:qemu-x86_64:minibasic).
  • Testing logs before change: You need to show the failure that you're fixing. Include the relevant error messages from the CI.
  • Testing logs after change: Showing that the build now completes successfully is important. A concise summary of the build output or confirmation that there are no errors is sufficient. Don't paste entire build logs.

Example of an Improved PR Description:

## Summary

This PR addresses a missing CMake build configuration for the interpreter and nxlooper components, which was causing CI failures (https://github.com/apache/nuttx/actions/runs/11880169863/job/33103008538?pr=14756).  Previously, these components were not included in the standard NuttX build process for the `sim:minibasic` and `sim:alsa` configurations. This change adds the necessary CMake build rules to include the interpreter and nxlooper source files and dependencies in the respective `CMakeLists.txt` files.

## Impact

* **New feature added?** YES (if the interpreter wasn't previously available), or NO (if it existed but couldn't be built).  Be explicit.
* **Impact on user:** YES (if new feature) Users can now build and use the interpreter on simulated `minibasic` and `alsa` configurations. NO (if just build fix) No user-facing changes other than a fixed build.
* **Impact on build:** YES. The interpreter and nxlooper are now included in the build for `sim:minibasic` and `sim:alsa` configurations.
* **Impact on hardware:** NO.
* **Impact on documentation:** YES (if new feature). Documentation should be updated to describe how to use the new interpreter functionality. NO (if just build fix) Assuming no functional changes.
* **Impact on security:** NO.
* **Impact on compatibility:** NO.
* **Anything else to consider?**  None.

## Testing

I confirm that changes are verified on my local setup and work as intended:
* **Build Host:** Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
* **Target(s):** `sim:qemu-x86_64:minibasic`, `sim:qemu-x86_64:alsa`


Testing logs before change:

[ERROR] CMake Error at .../CMakeLists.txt: (some error message indicating missing interpreter/nxlooper build)
... other relevant error messages ...


Testing logs after change:

Build completed successfully.

(Or a brief summary confirming successful compilation and linking of the interpreter/nxlooper).

By providing more comprehensive information, reviewers can more easily understand the purpose, impact, and validation of your changes, leading to a faster and smoother review process.

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 @xuxin930 :-)

@xiaoxiang781216 xiaoxiang781216 merged commit fee82bd into apache:master Nov 18, 2024
25 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.

4 participants