Skip to content

Conversation

@xuxin930
Copy link
Contributor

@xuxin930 xuxin930 commented Nov 1, 2024

Summary

implement CMake build of xtensa arch

Impact

new features and forward compatibility

Testing

configure:cmake -B build -DBOARD_CONFIG=iss-hifi4:nsh 
build:cmake --build build
run:xt-run build/nuttx

configure:cmake -B build -DBOARD_CONFIG=iss-hifi4:nsh
build:cmake --build build
run:xt-run build/nuttx

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Nov 1, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 1, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details in almost every section. Here's a breakdown of what's missing and how it can be improved:

Summary:

  • Missing: The summary is far too brief. It states what was done, but not why. What problem does this CMake build system solve? Is the current build system broken, inadequate, or is this simply adding a more modern option? How does the CMake build system work (at a high level)? Which parts of the code were modified to implement this (e.g., the configuration system, Makefiles replaced, etc.)?

Impact:

  • Insufficient Detail: Saying "new features and forward compatibility" is vague. Specifically:
    • New Feature: Explicitly state that "CMake build support is added for the Xtensa architecture."
    • Forward Compatibility: How does this improve forward compatibility? With newer CMake versions? With other build systems? Explain clearly.
  • Missing Impact Assessments: The PR description omits assessment for most impact categories. It needs to address each point explicitly, even if the answer is "NO." For example:
    • User Impact: Will users need to change how they build NuttX for Xtensa? If so, provide clear instructions.
    • Build Impact: The build process definitely changes. Describe how. Are existing Makefiles affected? Are there new configuration options?
    • Hardware Impact: Does this affect any specific Xtensa chips or boards? If not, explicitly state "NO."
    • Documentation Impact: Does documentation need updating? If so, has it been updated in the PR, or is a separate documentation PR planned?
    • Security Impact: Even if the answer is "NO," explicitly state it.
    • Backward Compatibility: Does this break the existing build system for Xtensa? If not, state "NO."
    • Anything else to consider: Are there any known limitations of the CMake build?

Testing:

  • Insufficient Detail: The provided commands are insufficient.
    • Build Host: Specify the OS, CPU architecture, and compiler used for testing. (e.g., "Linux x86_64, GCC 12.2").
    • Target: Be more specific. Instead of "xt-run," indicate the specific Xtensa target and configuration being tested (e.g., "Espressif ESP32 DevKitC, default configuration").
  • Missing Logs: The template clearly requests "Testing logs before change" and "Testing logs after change." These are essential for reviewers to understand what functionality was tested and verify that the changes work as intended. Include relevant output showing successful builds and execution (even if it's just a simple "Hello, World"). If there were build errors before the change, show those too.

Example of an Improved Summary:

This PR implements CMake as an alternative build system for the Xtensa architecture. The current Makefile-based build system, while functional, can be complex and difficult to extend. CMake offers a more modern, cross-platform build system that simplifies the build process, improves maintainability, and enables easier integration with other tools. This implementation involves adding CMakeLists.txt files to the Xtensa architecture directory and modifying the configuration system to support CMake generation. This addresses [NuttX Issue #123](replace with actual issue number if applicable).

By providing these missing details, the PR will be much easier for reviewers to understand, evaluate, and ultimately merge. A complete and thorough PR description greatly improves the chances of a successful contribution.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

@xuxin930 iss-hifi4 is not in mainline, please test in some board existing in mainline

@xiaoxiang781216
Copy link
Contributor

@xuxin930 iss-hifi4 is not in mainline, please test in some board existing in mainline

let's add chip/board in the new pr.

@xiaoxiang781216 xiaoxiang781216 merged commit 6e81b1e into apache:master Nov 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: xtensa Issues related to the Xtensa architecture 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