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

Fix building with external LLVM #13420

Merged
merged 2 commits into from
Aug 10, 2023

Conversation

stephanlachnit
Copy link
Contributor

This Pull request:

Changes or fixes:

Ensure that LLVM is always linked statically. While doing so this fixes #12156 and #12152. Tested with both LLVM 13 and LLVM 16.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

This PR fixes #12156 #12152

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac11/noimt, mac12arm/cxx20, windows10/default
How to customize builds

@stephanlachnit
Copy link
Contributor Author

@Axel-Naumann is a backport of #12153 and this PR possible for a 6.28.xx release?

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Test Results

         9 files           9 suites   1d 19h 23m 30s ⏱️
  2 475 tests   2 470 ✔️ 0 💤 5
21 262 runs  21 257 ✔️ 0 💤 5

For more details on these failures, see this check.

Results for commit 66f562d.

@phsft-bot
Copy link
Collaborator

Build failed on mac11/noimt.
Running on macphsft23.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@bellenot bellenot self-assigned this Aug 9, 2023
Copy link
Member

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

That looks like the fix I imagined. LGTM! Thanks!

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

Also LGTM and CI builds are happy, so let's merge.

@hahnjo hahnjo merged commit 92807f3 into root-project:master Aug 10, 2023
10 of 13 checks passed
@hahnjo
Copy link
Member

hahnjo commented Aug 10, 2023

@Axel-Naumann is a backport of #12153 and this PR possible for a 6.28.xx release?

I think that's a good idea. Can you prepare a backport PR against v6-28-00-patches? If not I can also do it. Then up to Axel if we include it already in the next patch release that is expected ~now or later.

@stephanlachnit
Copy link
Contributor Author

@Axel-Naumann is a backport of #12153 and this PR possible for a 6.28.xx release?

I think that's a good idea. Can you prepare a backport PR against v6-28-00-patches? If not I can also do it. Then up to Axel if we include it already in the next patch release that is expected ~now or later.

I was hoping I would be able to do so before I leave, but unfortunately I had to move and don't have access to my computer for a while... So I wouldn't mind if you give it a go, but it's also not super high priority.

@hahnjo
Copy link
Member

hahnjo commented Aug 14, 2023

So I wouldn't mind if you give it a go, but it's also not super high priority.

no worries, here we go: #13443
Happy moving 🚀

@hahnjo
Copy link
Member

hahnjo commented Aug 14, 2023

Hm, it appears the second commit breaks check-cling from ROOT with builtin_llvm=ON because it doesn't include LLVMConfig.cmake anymore, so TARGET_TRIPLE is not available. I think we can fix this in interpreter/CMakeLists.txt by forwarding the respective variable, let me investigate...

edit: #13446

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.

Building fails with builtin_llvm=OFF: CommandLine Error: Option 'W' registered more than once!
5 participants