Skip to content

Conversation

@simbit18
Copy link
Contributor

@simbit18 simbit18 commented Dec 11, 2024

Summary

added LLVM toolchain for Arm on Apple Intel
release-17.0.1

added LLVM toolchain for Arm on Apple Silicon
release-19.1.1

Impact

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

Testing

github [macOS-14, macOS-13 (arm-08)]
https://github.com/simbit18/nuttx_test_pr/actions/runs/12277626847/job/34257527992

====================================================================================
Configuration/Tool: nucleo-f207zg/nsh,CONFIG_ARM_TOOLCHAIN_CLANG
2024-12-11 14:35:07
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Disabling CONFIG_ARM_TOOLCHAIN_GNU_EABI
  Enabling CONFIG_ARM_TOOLCHAIN_CLANG
  Building NuttX...
chip/stm32_gpio.c:43:11: note: '#pragma message: CONFIG_STM32_USE_LEGACY_PINMAP will be deprecated migrate board.h see tools/stm32_pinmap_tool.py'
   43 | #  pragma message "CONFIG_STM32_USE_LEGACY_PINMAP will be deprecated migrate board.h see tools/stm32_pinmap_tool.py"
      |           ^~~~~~~
  Normalize nucleo-f207zg/nsh
====================================================================================

added LLVM toolchain for Arm on Apple Intel
release-17.0.1

added LLVM toolchain for Arm on Apple Silicon
release-19.1.1

ì
@github-actions github-actions bot added Area: Tooling Area: CI Size: S The size of the change in this PR is small labels Dec 11, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 11, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, though it could be improved.

Strengths:

  • It addresses a clear need: Adding LLVM toolchain support for Arm on Apple platforms.
  • It outlines the impact (albeit minimally).
  • It provides testing verification and logs, demonstrating functionality.

Weaknesses & Suggestions for Improvement:

  • Summary: The summary is too brief. It should explain why LLVM toolchain support is needed. Is this for performance, compatibility, or some other reason? It should also explicitly state what changed (e.g., "Added configuration options and build scripts to support...").

  • Impact: While the "no impact" statements are fine if true, it's important to be thorough. Consider the following:

    • Impact on hardware: Does this impact any hardware, even indirectly? For example, are there any specific Arm architectures or boards that are now supported or unsupported because of this change? Even if the answer is "no," explicitly stating it demonstrates that you considered the question.
    • Impact on documentation: If LLVM toolchain usage is now possible, the documentation almost certainly needs updating to guide users on how to use it. Even a small documentation update warrants a "YES" here with a description of the changes.
    • Impact on compatibility: Does this change introduce any compatibility issues with existing code or tools? Even if the answer is no, explicitly stating so is beneficial.
  • Testing: The provided logs only show a successful build. More comprehensive testing is needed to demonstrate functionality. Consider including:

    • Tests of basic functionality on the target hardware (if applicable).
    • Tests of any specific features related to the LLVM toolchain.
    • Performance benchmarks comparing LLVM to GCC (if relevant).
  • Clarity: Specify the exact versions of LLVM used (e.g., llvm-16.0.6, not just release-17.0.1). This level of detail is crucial for reproducibility.

By addressing these weaknesses, the PR would be significantly stronger and more likely to be accepted.

@acassis acassis merged commit 1dcada4 into apache:master Dec 11, 2024
27 checks passed
@simbit18 simbit18 deleted the simbit18-macos-dmg branch December 11, 2024 17:12
@lupyuen
Copy link
Member

lupyuen commented Dec 13, 2024

Sorry @simbit18 I need to disable these sudoline for the macOS Build Farm, since sudo will hang the Build Job, waiting for password:

sudo hdiutil attach ${basefile}.dmg
sudo cp -R /Volumes/${basefile}/${basefile} "${NUTTXTOOLS}"/${basefile}
sudo mv ${basefile} clang-arm-none-eabi

Here's how I patch it for macOS Build Farm. Thanks!

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

simbit18 commented Dec 13, 2024

Hi @lupyuen have you tried to remove this patch

https://github.com/lupyuen/nuttx-build-farm/blob/a99c1f8abafe3ae5f3f3c00ed0e24ffd1f12f7bb/run-build-macos.sh#L123
(CONFIG_ARM_TOOLCHAIN_GNU_EABI)
to see if it is ok the build ?

@lupyuen
Copy link
Member

lupyuen commented Dec 15, 2024

@simbit18 Ah sorry I confused you :-) run-build-macos.sh is for the "build anything on macOS" script.

The script I patched is run-ci-macos.sh, which is for macOS Build Farm.

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