-
Notifications
You must be signed in to change notification settings - Fork 35
[wip] Add llvm 21 support #672
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #672 +/- ##
=======================================
Coverage 79.04% 79.04%
=======================================
Files 9 9
Lines 3879 3879
=======================================
Hits 3066 3066
Misses 813 813
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
|
clang-tidy review says "All clean, LGTM! 👍" |
086ee33 to
9f09f8a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
fbd1215 to
410db40
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
3 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Current progress with adding compatibility for llvm 21. Can build on all platforms. Can pass all tests for native Windows, native Linux and Emscripten platform. MacOS errors for the following test on both arm and x86 1: [ RUN ] InterpreterTest.IncludePaths |
c04a271 to
015f4f0
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
015f4f0 to
60df3d7
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
For the MacOS failures we apear to be triggering this line CppInterOp/lib/CppInterOp/Paths.cpp Line 161 in 82f08c6
|
60df3d7 to
2a280d1
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Something has happened to the llvm 21 release branch in the last 2 weeks to break the ScopeReflectionTest.GetNumBases test in CppInterOp. Also the Windows build of CppInterOp has broken somehow. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hey @mcbarton , Are you working on this ? I think I saw quite some errors errors while trying to build cppinterop against latest llvm 21 few days back. |
|
clang-tidy review says "All clean, LGTM! 👍" |
@anutosh491 With the exception of the Windows native build you can build CppInterOp against llvm 21 with the changes in this PR (Windows fails building llvm). The tests do not pass however. Given the changes in this PR are minimal, then the source of these errors are likely on the llvm side. I will update this PR soon to get rid of the Windows Emscripten cross compile patch since it is no longer needed, and to update the documentation. Not sure I have any spare time at the moment though to work out the source of the failing tests. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hey @mcbarton does the patch not sit well with llvm 21 ? If that's the case feel free to update it accordingly if that's possible. I shall revamp my PR on this (now that we have emscripten based tests) and try getting this upstream in a couple of days. |
|
hey @mcbarton here's the updated pr on llvm for the patch : llvm/llvm-project#132670 Hope this helps if we need to keep it for some more time on llvm 21 |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
The Emscripten builds will be fixed either by updating the existing llvm 20 exception handling patch for llvm 21, or @anutosh491 patch llvm/llvm-project#132670 being merged upstream, and cherry picked for the llvm 21 release branch. The only major issue left to resolve is why the apple jobs are failing the tests. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@mcbarton can you rebase and fix the size threshold? I’d like to get that in and make a release. We should fix the emscripten size in a separate effort to avoid blocking the support of llvm21. |
@vgvassilev I have rebased the PR, and removed the check for the size of the Emscripten shared library size, which I fix after release. The only thing that should be left to get the ci to go green would be to fix the native osx jobs which fail with the following error https://github.com/compiler-research/CppInterOp/actions/runs/19036208336/job/54361154631?pr=672#step:12:578 . I tried fixing it about 2 weeks ago without success. I have enabled it so this PR can be edited by maintainers. If anyone knows how to fix, then they can feel free to commit the fix to the PR. |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
d906430 to
125daf8
Compare
This reverts commit 125daf8.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
Description
Please include a summary of changes, motivation and context for this PR.
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist