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

CI: Disable the failed macOS Builds #14772

Closed
wants to merge 1 commit into from
Closed

Conversation

lupyuen
Copy link
Member

@lupyuen lupyuen commented Nov 14, 2024

Summary

This PR disables the failed macOS Builds from CI Checks sim-02 and sim-03, so that the macOS CI Checks will complete successfully in the NuttX Mirror Repo:

  • sim:libcxxtest: macOS doesn't support cxxabi.h

  • sim:lua: macOS doesn't support pipe2()

  • sim:note: macOS doesn't support ld --wrap

  • sim:quickjs: macOS fails to patch quickjs-libc.c

  • sim:rpproxy_virtio: macOS fails at uintmax_t unsigned long long vs unsigned long

  • sim:rpserver_virtio: macOS fails at uintmax_t unsigned long long vs unsigned long

Impact

With this PR, macOS CI Checks will complete successfully in the NuttX Mirror Repo.

Testing

The macOS CI Checks completed successfully in our Test Repo:
https://github.com/lupyuen5/label-nuttx/actions/runs/11828738112

- `sim:libcxxtest`: macOS doesn't support cxxabi.h

- `sim:lua`: macOS doesn't support pipe2()

- `sim:note`: macOS doesn't support `ld --wrap`

- `sim:quickjs`: macOS fails to patch quickjs-libc.c

- `sim:rpproxy_virtio`: macOS fails at uintmax_t `unsigned long long` vs `unsigned long`

- `sim:rpserver_virtio`: macOS fails at uintmax_t `unsigned long long` vs `unsigned long`
@github-actions github-actions bot added Area: Tooling Area: CI Size: S The size of the change in this PR is small labels Nov 14, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 14, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, though some sections could be more explicitly filled out. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why" (failed macOS builds), "what" (disabling specific tests), and "how" (by excluding them from the CI).
  • Impact is Addressed: The impact on the user, build process, hardware, documentation, security, and compatibility is adequately addressed, albeit briefly. The main impact is a positive one: fixing the broken CI.
  • Testing Provided: A link to a successful test run is provided, which demonstrates the effectiveness of the change.

Areas for Improvement (While Minor, Worth Considering):

  • Explicitly State "No Impact" Where Applicable: Instead of just implying no impact, explicitly state "NO" for each impact category where there's no change. For example: "Impact on user: NO". This improves readability and clarity.
  • More Detailed Testing Logs (If Feasible): While the link to the successful run is good, including snippets of relevant log output directly in the PR would further strengthen it. Show a "before" (failing builds) and "after" (successful builds) example, even if brief.
  • Mention Specific CI Config Files Changed: If possible, mention the specific CI configuration files that were modified to disable the tests. This adds transparency.
  • Consider Upstream Solutions (Long-Term): While disabling the tests is a practical short-term fix, mention (if applicable) whether upstream fixes for these macOS issues are being explored. This shows that the disabled tests are not intended to be a permanent solution.

Despite the minor suggestions, the PR provides sufficient information to understand the change, its impact, and the verification process. It generally meets the requirements.

-Darwin,sim:rpproxy_virtio

# macOS fails at uintmax_t: 'unsigned long long' vs 'unsigned long'
-Darwin,sim:rpserver_virtio
Copy link
Contributor

Choose a reason for hiding this comment

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

should we fix this problem instead disabling it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm can anyone help? Our macOS builds have been failing in our NuttX Mirror for a couple of weeks, kinda distressing to see it failing the NuttX Mirror Build every day 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

could you attach the warning message?

Copy link
Member Author

Choose a reason for hiding this comment

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

-Darwin,sim:libcxxtest

# macOS doesn't support pipe2()
-Darwin,sim:lua
Copy link
Contributor

Choose a reason for hiding this comment

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

but pipe2 should work in Darwin

Copy link
Member Author

Choose a reason for hiding this comment

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

Something special about the Lua build I think, since the other calls to pipe2 seem OK?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -33,6 +33,16 @@
# macOS matter compilation is not currently supported
-Darwin,sim:matter

# macOS doesn't support cxxabi.h
-Darwin,sim:libcxxtest
Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/apache/nuttx/tree/master/libs/libxx/libcxxabi could provide cxxabi.h, can we switch to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry not quite sure how to switch it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll post the above as NuttX Issues, since I'm not able to resolve them. Thanks!

Copy link
Member Author

@lupyuen lupyuen Nov 14, 2024

Choose a reason for hiding this comment

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

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.

3 participants