build: Allow building Cling using the Clang shared library. #15563
Conversation
|
Doesn't root-project/cling#430 require a simpler fix by adding libSerialization to the list of linked libraries? |
Test Results 12 files 12 suites 2d 16h 35m 44s ⏱️ Results for commit 094176e. ♻️ This comment has been updated with latest results. |
|
Note: We are intentionally building |
For Guix purposes, I'm building a |
That wouldn't be sufficient at least for linking to a shared library LLVM.so. When LLVM is built this way ( |
|
libclangSerialization is not part of llvm but clang and won’t be part of llvm.so |
For other purposes :) we should continue to make sure we can also build LLVM and CLANG in static mode. |
The shared library mode is more rigorous of detecting missing link-time dependencies. If we enumerate them all that won't hurt and won't change the way we build ROOT/cling if we want to build it is static mode. |
You're right, I answered too quickly. But rereading my original report I had managed to add link directives and get cling to build, but it wouldn't run, so this PR is still necessary/useful to build Cling using shared libraries. |
I agree this change shouldn't break the current way of linking cling statically. The CI suggests this change didn't impact that. |
Are you building cling standalone or you are building root? |
interpreter/cling/CMakeLists.txt
Outdated
| @@ -444,7 +447,11 @@ macro(add_cling_library name) | |||
| endif() | |||
|
|
|||
| if(TARGET ${name}) | |||
| target_link_libraries(${name} INTERFACE ${LLVM_COMMON_LIBS}) | |||
| if(LLVM_LIB_IS_SHARED) | |||
| target_link_libraries(${name} PUBLIC LLVM) | |||
There was a problem hiding this comment.
When you build in shared library ON mode that gets automatically linked. These changes are not needed.
There was a problem hiding this comment.
I don't understand; what does "shared library ON" mode means? Do you mean -DBUILD_SHARED_LIBS=ON ? If so, note that per https://llvm.org/docs/CMake.html,
BUILD_SHARED_LIBS is only recommended for use by LLVM developers. If you want to build LLVM as a shared library, you should use the LLVM_BUILD_LLVM_DYLIB option.
There was a problem hiding this comment.
Both options should give you a libLLVM.so, right?
|
I am not sure what is the use-case here but generally building something that has an execution engine and llvm in it as a shared library is a bad idea. This is because the symbols of llvm get incorporated in the binary/library and if that binary loads something like libMessa which contains a copy of llvm becomes a huge mess, unless you provide some sort of symbol versioning. Please think twice about your use-case before building as a shared library. You can get cling as a shared library through the CppInterOp project. As part of this PR we can probably accept changes in terms of missing dependencies. That is, adding to the list of dependent libraries (such as your change about libclangSerialization). |
Standalone. |
If I may elaborate, this is what the Guix package definitions for Cling look like: (define llvm-cling
;; To determine which version of LLVM a given release of Cling should use,
;; consult the
;; https://raw.githubusercontent.com/root-project/cling/master/LastKnownGoodLLVMSVNRevision.txt
;; file.
(let ((base llvm-15)) ;for a DYLIB build
(package/inherit base
(name "llvm-cling")
(version "13-20240318-01")
(source
(origin
(inherit (package-source base))
(method git-fetch)
(uri (git-reference
(url "https://github.com/root-project/llvm-project")
(commit (string-append "cling-llvm" version))))
(file-name (git-file-name "llvm-cling" version))
(sha256
(base32
"1zh6yp8px9hla7v9i67a6anbph140f8ixxbsz65aj7fizksjs1h3"))
(patches (search-patches "clang-cling-13-libc-search-path.patch")))))))
(define clang-cling-runtime
(let ((base clang-runtime-13))
(package/inherit base
(name "clang-cling-runtime")
(version (package-version llvm-cling))
(source (package-source llvm-cling))
(arguments
(substitute-keyword-arguments (package-arguments base)
((#:phases phases '%standard-phases)
#~(modify-phases #$phases
(add-after 'unpack 'change-directory
(lambda _
(chdir "compiler-rt")))
(add-after 'install 'delete-static-libraries
;; This reduces the size from 22 MiB to 4 MiB.
(lambda _
(for-each delete-file (find-files #$output "\\.a$"))))))))
(inputs (modify-inputs (package-inputs base)
(replace "llvm" llvm-cling))))))
(define clang-cling
(let ((base clang-13))
(package/inherit base
(name "clang-cling")
(version (package-version llvm-cling))
(source (package-source llvm-cling))
(arguments
(substitute-keyword-arguments (package-arguments base)
((#:phases phases '%standard-phases)
#~(modify-phases #$phases
(add-after 'unpack 'change-directory
(lambda _
(chdir "clang")))
(add-after 'install 'delete-static-libraries
;; This reduces the size by half, from 220 MiB to 112 MiB.
(lambda _
(for-each delete-file (find-files #$output "\\.a$"))))))))
(propagated-inputs
(modify-inputs (package-propagated-inputs base)
(replace "llvm" llvm-cling)
(replace "clang-runtime" clang-cling-runtime))))))
(define-public cling
(package
(name "cling")
(version "1.0")
(source (origin
(method git-fetch)
(uri (git-reference
(url "https://github.com/root-project/cling")
(commit (string-append "v" version))))
(file-name (git-file-name name version))
(sha256
(base32
"17n66wf5yg1xjc94d6yb8g2gydjz0b8cj4a2pn6xrygdvhh09vv1"))
;; Patch submitted upstream here:
;; https://github.com/root-project/cling/pull/433.
(patches (search-patches "cling-use-shared-library.patch"))))
(build-system cmake-build-system)
(arguments
(list
#:build-type "Release" ;keep the build as lean as possible
#:tests? #f ;FIXME: 78 tests fail (out of ~200)
#:test-target "check-cling"
#:configure-flags
#~(list (string-append "-DCLING_CXX_PATH="
(search-input-file %build-inputs "bin/g++"))
;; XXX: The AddLLVM.cmake module expects LLVM_EXTERNAL_LIT to
;; be a Python script, not a shell executable.
(string-append "-DLLVM_EXTERNAL_LIT="
(search-input-file %build-inputs "bin/.lit-real")))
#:phases
#~(modify-phases %standard-phases
(add-after 'unpack 'set-version
(lambda _
(make-file-writable "VERSION")
(call-with-output-file "VERSION"
(lambda (port)
(format port "~a~%" #$version)))))
(add-after 'unpack 'patch-paths
(lambda* (#:key inputs #:allow-other-keys)
(substitute* "lib/Interpreter/CIFactory.cpp"
(("\\bsed\\b")
(which "sed"))
;; This ensures that the default C++ library used by Cling is
;; that of the compiler that was used to build it, rather
;; than that of whatever g++ happens to be on PATH.
(("ReadCompilerIncludePaths\\(CLING_CXX_RLTV")
(format #f "ReadCompilerIncludePaths(~s"
(search-input-file inputs "bin/g++")))
;; Cling uses libclang's CompilerInvocation::GetResourcesPath
;; to resolve Clang's library prefix, but this fails on Guix
;; because it is relative to the output of cling rather than
;; clang (see:
;; https://github.com/root-project/cling/issues/434). Fully
;; shortcut the logic in this method to return the correct
;; static location.
(("static std::string getResourceDir.*" all)
(string-append all
" return std::string(\""
#$(this-package-input "clang-cling")
"/lib/clang/"
#$(first
(take (string-split
(package-version clang-cling) #\-)
1)) ".0.0" ;e.g. 13.0.0
"\");")))
;; Check for the 'lit' command for the tests, not 'lit.py'
;; (see: https://github.com/root-project/cling/issues/432).
(substitute* "CMakeLists.txt"
(("lit.py")
"lit"))))
(add-after 'unpack 'adjust-lit.cfg
;; See: https://github.com/root-project/cling/issues/435.
(lambda _
(substitute* "test/lit.cfg"
(("config.llvm_tools_dir \\+ '")
"config.cling_obj_root + '/bin"))))
(add-after 'install 'delete-static-libraries
;; This reduces the size from 17 MiB to 5.4 MiB.
(lambda _
(for-each delete-file (find-files #$output "\\.a$")))))))
(native-inputs (list python python-lit))
(inputs (list clang-cling llvm-cling))
(home-page "https://root.cern/cling/")
(synopsis "Interactive C++ interpreter")
(description "Cling is an interactive C++17 standard compliant
interpreter, built on top of LLVM and Clang. Cling can be used as a
read-eval-print loop (REPL) to assist with rapid application development.
Here's how to print @samp{\"Hello World!\"} using @command{cling}:
@example
cling '#include <stdio.h>' 'printf(\"Hello World!\\n\");'
@end example")
(license license:lgpl2.1+))) ;for the combined workAs a distribution (Guix), building as a shared library makes sense to 1. reduce disk size and 2. inherit the base llvm/clang package definitions mostly as-is (which should reduce maintenance). While linking to different LLVM could be a problem, Guix provides the tools to manipulate the package graph, should someone want to link libcling with Mesa or some other projects that links to the stock LLVM, so that it'd instead link with |
Is CppInterOp the future of Cling (that is, will it eventually obsolete it?). I remember reading about some effort integrating Cling or some clang-repl into the LLVM project itself, which would be simplest for users/distributors down the line. As for my use-case, I tried expounding on the rationale here: #15563 (comment) |
|
We have had the same discussions for the conda package. You are trading disk space for correctness and that’s something we should not do. |
|
You can take a look for more details for example here https://github.com/Axel-Naumann/llvm-mesa-reproducer CppInterOp is a library that enables incremental compilation for third party clients. It won’t replace cling per se but eventually it will provide everything that clings provides using the clang-repl backend. |
Do you have a link to that past issue/discussion? It seems to me that this change under review is adding more flexibility without taking anything away. We can also think that at some point in the future, the LLVM project would integrate all the Cling patches, and a shared LLVM would make sense even on classic FHS distributions. |
conda-forge/cppyy-cling-feedstock#56 (comment)
Indeed, if we are adding only the missing dependencies. The current PR does more than that and I am not sure if that is strictly needed.
Yes, that project is called clang-repl. Unfortunately, I do not anticipate all patches to land in llvm because some hardly would make sense there. |
|
@Apteryks, I think I am lost. Are you building cling as a standalone project and you rely on the llvm-config rather than integrating it properly as an external project to llvm? |
Cling is built as a standalone project, in an environment that contains the patched LLVM and Clang built using the |
|
I think llvm discourages that setup generally. Can you use |
Discourage what setup? If you mean I haven't tried |
No, relying on the llvm-config binary.
In that case we should use find_package instead. You can see how that is done in the clad project https://github.com/vgvassilev/clad Unfortunately, I do not understand how the current changes help our project. I can claim they make our cmake more complicated and unreadable. I can see how this helps your package manager but this is the first time I hear about it. To move forward here, I propose to replace the llvm-config part with find_package for clang and llvm. Most of these changes you inserted will be handled automatically for you. |
Just to make sure, do you mean wholly replacing the llvm-config part (which already exists), or just the newly added |
Replacing should be fine. |
* CMakeLists.txt: Remove llvm-config related code, instead using modern 'find_package' constructs. Fixes: <root-project/cling#430>
Apteryks
force-pushed
the
allow-building-against-shared-llvm-library
branch
from
7f57ae2
to
094176e
Compare
I've pushed a new version which implements the change suggested. I was able to use this patch to build cling against a shared library LLVM fine, so it seems to work as intended! Thanks for the suggestion, that's indeed cleaner. |
|
That looks pretty good. Can you confirm you can build and package cling standalone this way? |
Yes. I built the |
|
Thanks for your patience! |
The officially supported way to build LLVM/Clang as a shared library is via the LLVM_BUILD_LLVM_DYLIB and LLVM_LINK_LLVM_DYLIB CMake options (see: https://llvm.org/docs/BuildingADistribution.html). When built this way, the whole of Clang API is exposed as a shared library (libclang-cpp.so).
[LLVM_LIB_IS_SHARED] <target_link_libraries>: Use the PUBLIC interface of the LLVM shared library.
Fixes: root-project/cling#430
-> Yes, using the Guix packages for building cling.
This PR fixes root-project/cling#430