Skip to content

[interpreter] Revert patch to move ClangConfig.cmake #17354

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jan 23, 2025
Merged

[interpreter] Revert patch to move ClangConfig.cmake #17354

merged 5 commits into from
Jan 23, 2025

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Jan 6, 2025 •
edited
Loading

Commit 8a45c09 already modified the setup of ClingConfig.cmake, this reverts the second part of commit 2b283cc and removes one downstream patch from our Clang sources.

TODO:

  • More testing with builtin_llvm=OFF / builtin_clang=OFF, also for the individual commits.
  • Create new tag and update interpreter/llvm-project.tag (to be sequenced with other changes)

@hahnjo hahnjo added in:Cling clean build Ask CI to do non-incremental build on PR labels Jan 6, 2025
@hahnjo hahnjo requested a review from vgvassilev January 6, 2025 10:12
@hahnjo hahnjo self-assigned this Jan 6, 2025
@hahnjo hahnjo mentioned this pull request Jan 6, 2025
Closed
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 6, 2025 •
edited
Loading

Test Results

    18 files      18 suites   3d 17h 10m 56s ⏱️
 2 686 tests  2 663 ✅ 23 💤 0 ❌
46 622 runs  46 622 ✅  0 💤 0 ❌

Results for commit 27c18ce.

♻️ This comment has been updated with latest results.

@hahnjo hahnjo mentioned this pull request Jan 20, 2025
Merged
5 tasks
@hahnjo hahnjo marked this pull request as ready for review January 21, 2025 07:38
@hahnjo hahnjo requested a review from bellenot as a code owner January 21, 2025 07:38
@hahnjo hahnjo requested a review from devajithvs January 21, 2025 07:38
@hahnjo
Copy link
Member Author

hahnjo commented Jan 21, 2025

I tested with various configurations, and it appears builtin_clang=OFF is broken since the upgrade to LLVM 16 (#17461). I checked with the diff pasted in the issue that this PR doesn't make things worse, and I will fix builtin_clang=OFF in a subsequent PR. @vgvassilev if the changes look ok to you, I will prepare a tag and update interpreter/llvm-project.tag so we can get rid of another of our downstream patches (and clean up our CMake code in the process)

vgvassilev
vgvassilev approved these changes Jan 21, 2025
View reviewed changes
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.

LGTM!

@hahnjo
Copy link
Member Author

hahnjo commented Jan 21, 2025

I changed CLANG_CONFIG_CMAKE_DIR to CLANG_CMAKE_DIR (that's the variable that is actually set by ClangConfig.cmake) and also created the tag. Waiting for another CI run before merging...

hahnjo added 4 commits January 21, 2025 17:11
@hahnjo
[interpreter/cling] Consistently set and use CLANG_CMAKE_DIR
da1a710 
For standard CMake, Clang_DIR should include the lib/cmake/clang
subdirectory suffix. This currently works only because Clad defines
HINTS itself.
@hahnjo
[interpreter] Rename variable as CLANG_BINARY_DIR
2e03918 
This is the name used in the Clang build system.
@hahnjo
[cling] Use CMAKE_CURRENT_{SOURCE,BINARY}_DIR
eccee95 
This is clearer and should also help in case ROOT is built as a
subdirectory itself.
@hahnjo
[interpreter] Revert patch to move ClangConfig.cmake
27c18ce 
Commit 8a45c09 already modified the setup of ClingConfig.cmake,
this reverts the second part of commit 2b283cc and removes one
downstream patch from our Clang sources.
@hahnjo hahnjo marked this pull request as draft January 21, 2025 16:55
@hahnjo hahnjo marked this pull request as ready for review January 22, 2025 09:44
@hahnjo hahnjo merged commit 65779a4 into root-project:master Jan 23, 2025
22 checks passed
@hahnjo hahnjo deleted the clang-config branch January 23, 2025 11:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vgvassilev vgvassilev vgvassilev approved these changes

@devajithvs devajithvs devajithvs approved these changes

@bellenot bellenot Awaiting requested review from bellenot bellenot is a code owner

Assignees

@hahnjo hahnjo

Labels
clean build Ask CI to do non-incremental build on PR in:Cling
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hahnjo @vgvassilev @devajithvs