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(bugfix): sync CMake SIM Toolchain file #13669

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xuxin930
Copy link
Contributor

Summary

  1. Synchronize CMake's SIM arch Toolchain file and Makefile versions to be consistent

Impact

Synchronous build system, not a new feature

Testing

All sim build

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
-U_WIN32 will cause windows host source such as sim_hostirq.c hearder windows.h exception

Signed-off-by: xuxin19 <xuxin19@xiaomi.com>
@github-actions github-actions bot added Arch: simulator Issues related to the SIMulator Size: S The size of the change in this PR is small labels Sep 27, 2024
@nuttxpr
Copy link

nuttxpr commented Sep 27, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements.

Here's why and how to improve it:

  • Summary:
    • Missing "Why": The summary states WHAT is being changed but not WHY. Is there a specific problem caused by the versions being out of sync? Does this improve maintainability? Be explicit about the benefit.
  • Impact:
    • Too Brief: While stating "Synchronous build system, not a new feature" is a start, it lacks detail. Even if no user-facing changes occur, be thorough:
      • Build Impact: Does this change any build flags, dependencies, or the build process itself?
      • Hardware: Explicitly confirm "NO" impact on any hardware architectures or boards.
      • Documentation: Does this change necessitate any updates to build instructions or documentation? Even if "NO", state it.
      • Compatibility: Be explicit about backward and forward compatibility. Does this change affect how the build system interacts with older NuttX versions or tools?
  • Testing:
    • Insufficient Detail: "All sim build" is not descriptive enough.
      • Host Details: List the specific operating system(s) and compiler(s) used for building.
      • Target Details: Specify the simulator(s) used (e.g., QEMU, Renode) and any relevant configurations.
      • Logs: The testing logs sections are empty. Provide actual logs or output demonstrating the issue before the change and the successful outcome after the change.

Example Improvements

Summary:

Synchronize CMake and Makefile Toolchain Versions for SIM Arch

This change ensures version consistency between the CMake and Makefile-based build systems for the SIM (simulator) architecture, preventing potential build issues and improving maintainability.

Impact:

  • New Feature: NO
  • User Impact: NO
  • Build Impact: NO. The build process and required flags remain unchanged.
  • Hardware Impact: NO. This change only affects the simulator build environment.
  • Documentation Impact: NO. No documentation updates are required.
  • Security Impact: NO
  • Compatibility Impact:
    • Backward Compatibility: YES. This change is compatible with previous NuttX versions.
    • Forward Compatibility: N/A
  • Other Considerations: None.

Testing:

Build Host:
* OS: Ubuntu 20.04
* Compiler: GCC 9.4.0

Target:
* Architecture: SIM
* Simulator: QEMU (qemu-system-arm)

Testing Logs Before Change:

 [Provide actual build logs showing inconsistency or errors]

Testing Logs After Change:

 [Provide build logs demonstrating successful, consistent builds]

-U__linux__
-U__sun__
-U__unix__
-U__ENVIRONMENT_MAC_OS_X_VERSION_MIN_REQUIRED__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@xuxin930 xuxin930 Sep 27, 2024

Choose a reason for hiding this comment

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

@anchao OK, let's unify the SIM option into Toolchain file and remove setting in SIM arch cmakelists.txt

@xuxin930 xuxin930 marked this pull request as draft September 27, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: simulator Issues related to the SIMulator Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants