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

Run test on macOS 14 with upstream clang #613

Merged
merged 5 commits into from
Sep 17, 2024

Conversation

burnpanck
Copy link
Contributor

macOS 14 is the only platform that tests on arm64 on GitHub Actions (and is available for public repos). We already test on macOS 14, but so far only with Apple Clang. Apple Clang isn't a widely known compiler in terms of standards compliance, so test with upstream clang should be valuable. In particular, Apply Clang doesn't seem to warn about prevision loss from long to long double, but upstream clang does. Thus, with this PR, we actually detect the causes of issue #611. #611 is already fixed on master, so the (fast-forward) merge should fix the CI issues that will appear.

mpusz
mpusz previously approved these changes Sep 16, 2024
Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Thanks! I have no working experience with Apple platforms, so your help here is greatly appreciated.

@mpusz
Copy link
Owner

mpusz commented Sep 16, 2024

It seems we have some runtime issues with precision on a new platform for some unit tests. Could you please fix it on your side as I have no way to repo this in my environment?

@burnpanck
Copy link
Contributor Author

Yeah, I left those issues in on purpose, to highlight that these tests do in fact add value :-). I'll merge the current master, where the recently merged #612 (fix to #611) should resolve this issue.

@burnpanck
Copy link
Contributor Author

Ah, here is now that small precision loss in the trigonometric functions due to long double being the same as double in this environment. I cherry-picked that fix from #615, increasing the threshold to two epsilon for those particular tests. Hopefully, now all is green again.

Copy link
Owner

@mpusz mpusz left a comment

Choose a reason for hiding this comment

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

Thanks!

@mpusz mpusz merged commit 8a68221 into mpusz:master Sep 17, 2024
347 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants