Skip to content

Conversation

@simbit18
Copy link
Contributor

@simbit18 simbit18 commented Nov 11, 2024

Summary

added shell script darwin_arm64.sh for macOS Apple Silicon

updated toolchains for macOS on Apple Silicon

  • arm64
  • aarch64
  • xpack-riscv
  • xtensa-esp32

#14691

Impact

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

Testing

tested only on github workflow with only [macos]

macos-14
github macos-14

added shell script darwin_arm64.sh  for macOS Apple Silicon

updated toolchains for macOS on Apple Silicon
- arm64
- aarch64
- xpack-riscv
- xtensa-esp32
@github-actions github-actions bot added Area: Tooling Area: CI Size: M The size of the change in this PR is medium labels Nov 11, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 11, 2024

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements but is missing some key information. Here's a breakdown:

Strengths:

  • Clear Summary: The summary clearly states the "what" and "why" of the changes. Adding support for macOS Apple Silicon is a good justification.
  • Impact Assessment (Mostly): Most impact sections are addressed, although some lack detail.
  • Testing Confirmation: Confirmation of testing is present, and a link to the GitHub workflow is provided.

Weaknesses:

  • Missing Details in Impact: While most impact sections are marked "NO," it's important to consider potential impacts more thoroughly, even if they are negative. For example:
    • Impact on hardware: Even if no existing architectures/boards/drivers change, explicitly stating which ones were tested with the new toolchains would be helpful. Did you test any physical boards?
    • Impact on documentation: Does the documentation need updating to reflect the new supported toolchains for macOS Apple Silicon? Even if not, explicitly stating this would be beneficial.
  • Limited Testing Information: While the GitHub Actions link is good, it doesn't replace showing actual logs. Include snippets of relevant logs demonstrating successful builds with each of the mentioned toolchains (arm64, aarch64, xpack-riscv, xtensa-esp32). "Tested only on GitHub workflow" isn't sufficient. Provide more specifics about the configurations tested within the workflow.
  • Missing "How it Works" in Summary: The summary doesn't explain how the toolchain updates were implemented. Briefly describe the changes made to the build system or configuration files.
  • Lack of "Before" Logs: Even if the behavior hasn't changed functionally, providing "before" logs would create a baseline for comparison and prove that the existing functionality remains unaffected. This is especially important when changing build tools.

Recommendation:

To fully meet the NuttX requirements, the PR should be revised to include:

  1. More detailed impact assessment: Even if the answer is "NO," briefly explain why. If "YES," provide specifics. For hardware impact, list the tested architectures/boards. For documentation, explicitly state whether changes are needed.
  2. More comprehensive testing information: Include relevant snippets of logs demonstrating successful builds with each toolchain (before and after). Specify the configurations (board, architecture) used in the tests, both locally and in the GitHub workflow.
  3. Explanation of implementation in the summary: Briefly describe the changes made to support the new toolchains.
  4. "Before" logs: Include logs demonstrating the previous behavior before the change, even if no functional changes are expected.

By addressing these points, the PR will be much stronger and more likely to be accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit 2464b32 into apache:master Nov 11, 2024
lupyuen added a commit to lupyuen/nuttx-build-farm that referenced this pull request Nov 12, 2024
@simbit18 simbit18 deleted the simbit18-macos14 branch November 12, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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