Skip to content

Conversation

@simbit18
Copy link
Contributor

@simbit18 simbit18 commented Nov 25, 2024

Summary

add toolchains for macOS on Apple Intel

xtensa-esp32s2
xtensa-esp32s3

add toolchains for macOS on Apple Silicon

xtensa-esp32s2
xtensa-esp32s3

add install for ubuntu.sh

xtensa-esp32s2
xtensa-esp32s3

add install for linux.sh

xtensa-esp32s2
xtensa-esp32s3

Impact

Impact on user: No changes to user-facing functionality
Impact on build: Build process remains the same

Testing

github macos-13 and macos-14

macOS on Apple Silicon and Intel esp32s3

====================================================================================
Configuration/Tool: esp32s3-devkit/cxx
2024-11-25 12:39:41
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  Normalize esp32s3-devkit/cxx
====================================================================================
Configuration/Tool: esp32s3-devkit/nsh
2024-11-25 12:41:02
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  Normalize esp32s3-devkit/nsh
====================================================================================

macOS on Apple Silicon and Intel esp32s2

====================================================================================
Configuration/Tool: esp32s2-saola-1/watchdog
2024-11-25 12:20:22
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  Normalize esp32s2-saola-1/watchdog
====================================================================================
Configuration/Tool: esp32s2-saola-1/ostest
2024-11-25 12:22:52
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
  Normalize esp32s2-saola-1/ostest
====================================================================================

add toolchains for macOS on Apple Intel

xtensa-esp32s2
xtensa-esp32s3

add toolchains for macOS on Apple Silicon

xtensa-esp32s2
xtensa-esp32s3

add install for ubuntu.sh

xtensa-esp32s2
xtensa-esp32s3

add install for linux.sh

xtensa-esp32s2
xtensa-esp32s3
@nuttxpr
Copy link

nuttxpr commented Nov 25, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but it's missing some crucial information and could be significantly improved.

Strengths:

  • Provides a summary of the changes (adding toolchains for macOS and Ubuntu/Linux).
  • Addresses the impact on the user and build process.
  • Includes testing logs demonstrating successful builds on macOS for both ESP32S2 and ESP32S3.

Weaknesses:

  • Missing Key Impact Details: The impact section is incomplete. It needs to address:
    • Impact on hardware: Does this affect any specific architectures, boards, or drivers? Even if the answer is no, explicitly state it.
    • Impact on documentation: Does this change require documentation updates? If so, were they provided in the PR?
    • Impact on security: Are there any security implications? Even if the answer is no, explicitly state it.
    • Impact on compatibility: Are there any backward, forward, or interoperability compatibility concerns? Again, even if the answer is no, explicitly state it.
  • Vague Summary: The summary mentions adding toolchains but doesn't explain why this is necessary. What problem does this solve? Is it to support newer versions of macOS? Is it to fix a bug? More context is needed.
  • Unclear "How it Works": The summary lacks a description of how the change was implemented. Where were these toolchains added? Were files modified? New files added?
  • Missing Issue References: Are there any related NuttX or NuttX Apps issues that prompted this change? If so, they should be linked.
  • Insufficient Testing Details: While testing logs are provided, they lack detail.
    • Specify the exact macOS versions (e.g., macOS 13.5) and compiler versions used.
    • Specify the full paths to the configurations tested (e.g., nuttx/configs/esp32s3-devkit/cxx).
    • Ubuntu/Linux testing is mentioned but no logs are provided. Add those logs.
    • Consider adding a brief description of the tests performed. Did you just build, or did you also run any functional tests on the target hardware?
  • "Anything else to consider?" This important question from the template is omitted.

Recommendations for Improvement:

  1. Expand the Summary: Explain the motivation for the change. What problem does it solve? Be more specific about what was changed (files, configurations, etc.).
  2. Complete the Impact Section: Address all the points listed in the template, even if the answer is "NO". Provide justification for your answers.
  3. Provide More Detailed Testing Information: Include specific versions of OS, compilers, and full paths to configurations. Add the missing Ubuntu/Linux testing logs. Describe the tests performed.
  4. Include Issue References: Link any related issues.
  5. Address "Anything else to consider?": Even if there's nothing else, explicitly state that.

By addressing these weaknesses, the PR will be much stronger and easier for reviewers to evaluate. Clear, concise, and complete information is essential for a smooth and efficient review process.

Copy link
Member

@lupyuen lupyuen 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 so much @simbit18! :-)

Copy link
Member

@lupyuen lupyuen left a comment

Choose a reason for hiding this comment

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

Tested OK on macOS M2 Pro thanks!
https://gist.github.com/lupyuen/6b51580a5df6d86872b529b9e0174626

$ git clone https://github.com/simbit18/nuttx --branch simbit18-darwin
$ nuttx/tools/testbuild.sh -A -R -j 12 -e '-Wno-cpp -Werror' testlist/xtensa-02.dat
Configuration/Tool: esp32s2-saola-1/spiflash
  Building NuttX...
  Normalize esp32s2-saola-1/spiflash

Previously xtensa-02 fails with "xtensa-esp32s2-elf-gcc: command not found"

lupyuen added a commit to lupyuen/nuttx-build-farm that referenced this pull request Nov 25, 2024
@simbit18
Copy link
Contributor Author

simbit18 commented Nov 25, 2024

@lupyuen why you do not use the -S option?

nuttx/tools/testbuild.sh -A -R -j 12 -e '-Wno-cpp -Werror' testlist/xtensa-02.dat

option -S
nuttx/tools/testbuild.sh -A -R -S -j 12 -e '-Wno-cpp -Werror' testlist/xtensa-02.dat

#13301 (comment)

PR #13301

@lupyuen
Copy link
Member

lupyuen commented Nov 25, 2024

Hmmm I can't recall why, perhaps because I copied from the GitHub CI Logs for macOS?
https://github.com/NuttX/nuttx/actions/runs/12007155773/job/33467286390#step:7:1382

  cd sources/nuttx/tools/ci
  ./cibuild.sh -i -c -A -R testlist/macos.dat
...
+ /Users/runner/work/nuttx/nuttx/sources/nuttx/tools/testbuild.sh -A -R -j 4 -e '-Wno-cpp -Werror' testlist/macos.dat

@simbit18
Copy link
Contributor Author

simbit18 commented Nov 25, 2024

now if you use it it saves minutes of build time

@lupyuen
Copy link
Member

lupyuen commented Nov 25, 2024

Yep that should be helpful for normal builds. For macOS Build Farm: We're replicating the same Build Environment as GitHub Actions, so we'll stick to the current settings. Thanks!

@simbit18
Copy link
Contributor Author

simbit18 commented Nov 25, 2024

I did not put option -S on github for macOS because it does not have xtensa-01 and xtensa-02

["macos", "sim-01", "sim-02", "sim-03"]

@xiaoxiang781216 xiaoxiang781216 merged commit 00f8310 into apache:master Nov 26, 2024
@simbit18 simbit18 deleted the simbit18-darwin branch November 26, 2024 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: CI Area: Tooling 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