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

xtensa/esp32: Remove duplicate board Make.defs #14855

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

zyfeier
Copy link
Contributor

@zyfeier zyfeier commented Nov 19, 2024

Summary

Remove duplicate board Make.defs.

In PR #14805 we found board Make.defs
will be included twice, so remove one of them.

Impact

ESP32 board

Testing

esp32-devkitc:sotest build success.

$(TOPDIR)/Make.defs was already include in common/Makefile.

Signed-off-by: zhangyuan29 <zhangyuan29@xiaomi.com>
@github-actions github-actions bot added Board: xtensa Size: S The size of the change in this PR is small labels Nov 19, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 19, 2024

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is the change necessary? Just saying "duplicate Make.defs" isn't enough. Explain why having a duplicate is a problem. Does it cause build errors? Inconsistent behavior? Bloat the build system? Be specific.
  • What functional part of the code is being changed? Be more precise than "ESP32 board." Which directory? Which files? e.g., boards/risc-v/esp32c3/esp32-devkitc.
  • How exactly does the change work? Which Make.defs file was removed? Where was the redundant inclusion happening?

Missing Information in Impact:

  • You need to answer all the impact questions with YES/NO and provide descriptions for any "YES" answers. For example:
    • Impact on user: Likely NO, but explain why. e.g., "No, users should not experience any functional changes."
    • Impact on build: YES. "The build system will no longer include the duplicate Make.defs, reducing build time (potentially slightly) and eliminating a potential source of errors." Quantify the build time reduction if possible.
    • Impact on hardware: NO, unless the duplicate Make.defs was causing hardware-specific issues. Explain.
    • Impact on documentation: Maybe NO, but if the documentation referenced the removed file, it might need updating.
    • Impact on security: Likely NO. Explain.
    • Impact on compatibility: Likely NO. Explain.

Missing Information in Testing:

  • Build Host(s): Provide details about your build host OS, CPU architecture, and compiler version. Be specific. e.g., "Linux Ubuntu 22.04, x86_64, GCC 11.2.0"
  • Target(s): Be more specific than "esp32-devkitc:sotest." Which ESP32 chip? Which configuration? e.g., "esp32c3, sotest configuration"
  • Testing logs: "build success" is insufficient. Provide actual logs, even if abbreviated. Show the relevant parts of the build output before and after the change, demonstrating the duplicate inclusion being resolved. If the change affects build time, include timings.

Example of an improved PR description:

Summary

This PR removes a duplicate inclusion of the board Make.defs file for the esp32-devkitc board. The duplicate inclusion was identified in PR #14805 and was causing unnecessary processing during the build, potentially leading to subtle errors and increased build time. This change removes the redundant inclusion in the boards/risc-v/esp32c3/esp32-devkitc/Make.defs file.

Impact

  • Is new feature added? Is existing feature changed?: No, this is a bug fix.
  • Impact on user: NO. Users should not experience any functional changes.
  • Impact on build: YES. The build process will be slightly faster due to the removal of redundant processing. Build time was reduced by approximately X seconds (provide actual measurement). The risk of subtle build errors due to the duplicate inclusion is also eliminated.
  • Impact on hardware: NO. This change only affects the build system and has no direct impact on hardware behavior.
  • Impact on documentation: NO. The change is internal to the build system and does not require documentation updates.
  • Impact on security: NO. This change has no security implications.
  • Impact on compatibility: NO. This change maintains backward and forward compatibility.
  • Anything else to consider?: None.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux Ubuntu 22.04, x86_64, GCC 11.2.0
  • Target(s): esp32c3, esp32-devkitc, sotest configuration

Testing logs before change:

... (relevant snippet showing duplicate inclusion, and build time) ...
make: Leaving directory '/home/user/nuttx/boards/risc-v/esp32c3/esp32-devkitc'
real    0m10.234s
user    0m5.123s
sys     0m1.234s

Testing logs after change:

... (relevant snippet showing the fix and the improved build time) ...
make: Leaving directory '/home/user/nuttx/boards/risc-v/esp32c3/esp32-devkitc'
real    0m10.101s  <-- Slightly faster
user    0m5.001s
sys     0m1.111s

@tmedicci
Copy link
Contributor

Testing internally. Please, let's wait our internal CI results.

@tmedicci
Copy link
Contributor

Testing internally. Please, let's wait our internal CI results.

Everything is fine with it!

@xiaoxiang781216 xiaoxiang781216 merged commit b524179 into apache:master Nov 19, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: xtensa 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.

4 participants