Skip to content

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

Merged
hahnjo merged 5 commits into
root-project:masterfrom
hahnjo:clang-config
Jan 23, 2025
Merged

[interpreter] Revert patch to move ClangConfig.cmake#17354
hahnjo merged 5 commits into
root-project:masterfrom
hahnjo:clang-config

Conversation

@hahnjo

@hahnjo hahnjo commented Jan 6, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

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 Bot commented Jan 6, 2025 •
edited
Loading

Copy link
Copy Markdown

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

hahnjo commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

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

@vgvassilev vgvassilev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@hahnjo

hahnjo commented Jan 21, 2025

Copy link
Copy Markdown
Member Author

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
@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