build: Add compatibility with LLVM 21#2030
build: Add compatibility with LLVM 21#2030lgritz merged 5 commits intoAcademySoftwareFoundation:mainfrom
Conversation
|
Thanks for this! It's been on my list of things to do for a while, it's a huge help for somebody else to take care of it. What's here looks good. I assume you have tested against llvm 21 on your end? The easiest way to incorporate a test into CI is to change ci.yml so that one of the Mac test cases that currently says |
|
@christian-heusel Are you still pursuing this PR? |
|
Hey thanks for checking, I have been busy with other things but can work on it later today 😊 |
a43b98e to
ffd2181
Compare
|
I have pushed another revision, could you approve the CI @lgritz? 😊 |
|
I believe the title of this PR is incorrect -- we already support LLVM 20, and you are adding compatibility with LLVM 21, right? Can you edit the title, please? |
|
Thanks for coming back to this. The code looks good (aside from failing CI entirely :-) and I think we are very close, probably just one more thing to fix. |
ffd2181 to
e58fdaf
Compare
1c1457a to
91a7032
Compare
|
Lets see if this revision makes a few more happy noises, could you approve again @lgritz ? 😊 Edit: Also are you fine with the change being in one commit or should I split it a bit? 😊 |
Neither -- I will merge it in the way that automatically squashes it to a single commit! |
This adds LLVM 21 compatibility by adding the relevant code changes guarded by `#ifdev`-statements for backwards compatibility. Additionally also add CI checks for this. Signed-off-by: Christian Heusel <christian@heusel.eu>
91a7032 to
2858176
Compare
|
I have now also fixed the |
|
And don't forget to mark it as "ready for review" |
|
So close! Now it's only failing for the two tests where you have (thank you) modified the CI to actually test against LLVM 21. And those failures appear to be warnings. What I think is happening is that when you were working on your own, you built in a mode where warnings were allowed. But in CI, we make sure that warnings are flagged as errors, so they don't get overlooked. And it's fairly clear from the MacOS case that these warnings in particular appear to be places where LLVM is telling us that we are using deprecated functionality that will disappear in a future release. So I think we need to fix those spots to use their modern API (only for LLVM 21+). On the Linux case, it also appears that by using clang 21, there are additional warnings happening in other parts of our code, unrelated to your work here. Why don't you concentrate on the Mac one, try to find fixes to the LLVM functions that are marked as deprecated, and simultaneously, I will take a look at the Linux one and see if I can fix quick fixes in the rest of the code to address those other warnings? |
|
Sounds good, I'll have a look later 😊 |
|
@christian-heusel Any progress on this? |
|
If at all possible, I would love to get this wrapped up in time for the scheduled patch release on Nov 1, a week from today, so we can finally say that we work with LLVM 21. |
|
So far I have been busy with other things but I'll get back to it tomorrow 😁👍 |
Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
I took it upon myself to take a quick look, and all of the remaining problems were related to just two deprecated LLVM functions, and it was not hard to find the fix. I took the liberty of pushing that fix to your branch to amend this PR. I can get a correct build on my Mac with LLVM 21 this way. Let's see if the CI passes or if there is anything left to do. |
|
Alright, we have three CI failures now:
So no remaining failures are because of this patch, I think it's all working and I intend to merge it tomorrow unless somebody objects (including any concerns you may have about the amendments I pushed to your patch). |
lgritz
left a comment
There was a problem hiding this comment.
LGTM. Thanks so much for helping us get to LLVM 21 compatibility!
We were using the wrong env variable name. Signed-off-by: Larry Gritz <lg@larrygritz.com>
|
Thanks for adding the missing compatibility fixes 😊 |
|
Look at that, now the Windows tests pass, without our doing anything. Must have been a spurious failure on GitHub after all. But we are still failing the "bleeding edge" test. Now we are farther along in the compilation, no longer having simple compilation errors based on errors using the LLVM APIs. Instead, we are failing at the link stage, with this message: I fear that this means that the LLVM 21.1.3 we are retrieving using the KyleMayes/install-llvm-action is somehow not correct for this Linux flavor? I'm going to try 21.1.4 and see if that helps at all. |
|
I've tried KyleMayes/install-llvm-action on every 21.1.x version, they all fail. I'm not sure what's up with that, I think the pre-built llvm it's downloading is somehow not right for the way we're using it or something about the GH runner or its distro? I'm not sure I have the time or patience to track it down at the moment. I'm convinced that the rest of this patch gets us to the point where we build correctly against LLVM 21 in general, and it is confirmed by the (working) Mac CI. So my current inclination is to
|
Signed-off-by: Larry Gritz <lg@larrygritz.com>
Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
left a comment
There was a problem hiding this comment.
LGTM, merging.
Only compromise: the only CI job that tests LLVM 21 is the MacOS one. I would like the "bleeding edge" Linux job to also do so, but we haven't been able to configure any of the Linux runners to make use of the pre-built LLVM builds we have easy access to.
This adds LLVM 21 compatibility by adding the relevant code changes guarded by `#ifdev`-statements for backwards compatibility. Avoid deprecated LLVM calls for 21+ Additionally also add CI checks for this. --------- Signed-off-by: Christian Heusel <christian@heusel.eu> Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: Larry Gritz <lg@larrygritz.com>
This adds LLVM 21 compatibility by adding the relevant code changes guarded by `#ifdev`-statements for backwards compatibility. Avoid deprecated LLVM calls for 21+ Additionally also add CI checks for this. --------- Signed-off-by: Christian Heusel <christian@heusel.eu> Signed-off-by: Larry Gritz <lg@larrygritz.com> Co-authored-by: Larry Gritz <lg@larrygritz.com> Signed-off-by: Alexandru Biscoveanu <alex.biscoveanu.gfx@gmail.com>
Description
This adds LLVM 21 compatibility, so far just as a first iteration without the
needed
#ifdefsbut I wanted to get a first review on it before completing it.The motivation behind the change is the upgrade of LLVM to 21 in the Arch Linux repositories:
https://archlinux.org/todo/llvm-21/
Tests
Checklist:
already run clang-format v17 before submitting, I definitely will look at
the CI test that runs clang-format and fix anything that it highlights as
being nonconforming.
cc @svenstaro