Also link plugin to it, fix exports and make tests link to the shared lib.
Details
- Reviewers
kfunk - Group Reviewers
KDevelop - Commits
- R32:447cc0a8d3aa: Introduce KDevClangPrivate shared lib
test_problems is working now on Windows
Diff Detail
- Repository
- R32 KDevelop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
@mwolff: This is one way to make unit test work on Windows. We were violating the ODR-rule before (kdev-clang symbols compiled into the plugin (loaded at runtime), + kdev-clang symbols compiled into the test exucutable.)
I think the problem Gleb was facing was that qobject_cast failed on him in the test_problem executable.
I don't really like the change (we have to pay attention to export macros again, we install a lib + a plugin, etc.), but I currently can't think of any better solutions...
(Also worth a read: http://lists.qt-project.org/pipermail/interest/2013-August/008197.html)
Well, recent CMake have WINDOWS_EXPORT_ALL_SYMBOLS property, which should export everything from a DLL without def files or explicit dllexport annotations. Haven't used it yet, though.
Ugh, I really _hate_ this I have to say. I really don't want to install a separate lib, which brings back the ABI issues we had before. I'll have to think a bit about this. If we really go down this route, then we'll have to store the lib next to the plugin, i.e. in the versioned path. Maybe we'll also need to introduce a SOVERSION etc. pp. to prevent the issues from KDev 4...
What ABI problems are you talking about? Since both plugin and shared lib are built and installed together, i don't see how we can get a mismatch.
Other approaches:
- Mark the *plugin* (kdevclang) as SHARED library and link against the plugin in unit test. But I'm not entirely sure about the implications of this change across our supported platforms.
- This also forces us to export a lot of symbols in the plugin SO, which we'd only need for unit testing => bad for dynamic loader performance
We'd also need to force CMake into renaming the lib properly on Unix at least (no lib prefix, for instance), and choosing the *right* install prefix.
@arrowdoger: the ABI issues I mention were encountered in KDevelop 4:
Someone builds his own KDevelop from git master, and only configures his environment partially, such that plugins are found, but the libs these plugins depend on are picked from the system installation e.g. which may be (and often is) ABI incompatible to whatever someone builds from Git.
In kdev4 we said this setup is not supported. In KDev 5 I would really like to see this issue be resolved, and getting rid of these libs was one obvious way to do so.
Hum, I don't quite understand how it is possible to get into such state. The plugin and sharedlib targets are defined in the same CMakeLists.txt, they are built together and are installed together.
If you are talking about plugin linking stage, CMake should pick up just-built sharedlib.
Yes and no. It's not the linking stage that happens at make time, rather the dynamic linking stage at runtime is what can break apart. Maybe this is not an issue anymore in KF5 based KDE apps thanks to proper RPATH usage, but I'd like if someone could make sure that this really is the case.
I'm still reluctant to give a +1 for this patch...
languages/clang/CMakeLists.txt | ||
---|---|---|
24–117 | if you don't visit the subdirectories anymore, remove the CMakeLists.txt in them. | |
languages/clang/codecompletion/CMakeLists.txt | ||
0 | see below, why duplicate this path fragment? | |
languages/clang/util/CMakeLists.txt | ||
0 | duplicate path? this is inside util/ already, no? Why repeat it? |
So, the problem is that executables from build tree could pick up installed shared lib? This is the only scenario i imagine when ABI mismatch can be observed.
If you are concerned with one more file to install, we can build it as STATIC for Unix-like and SHARED for win, but @kfunk was against this idea.
I'd happily rework this however you want, i just don't see any other way.
It's not only the build dir, it could also happen when you have ABI incompatible installs in different prefixes and mess up your environment by misconfiguring LD_LIBRARY_PATH e.g.
I'm still thinking about this, sorry - my availability is currently limited. Is this holding you back, or is it OK for you to keep this change local to your setup for now until we find a good solution?
Also: If anyone wants to speed up the process: Please check how RPATH is set/used in KF5/KDevelop5. Maybe my above pain points are not valid anymore, at which point this patch would become much more acceptable to me.
Couldn't we mitigate *this* particular problem by installing those libraries with a proper SONAME? I.e. kdevclangshared.so.${PLUGINVERSION}, for instance kdevclangshared.so.24 (could be all wrapped in a kdevplatform_add_private_library(...) CMake macro.
I also vote for calling this library KDevClangPrivate.
Pretty complex scenario to think about it, IMO.
is it OK for you to keep this change local to your setup for now until we find a good solution?
Sure. I, personally, don't even need it, because right now only test suite on Windows is affected by ODR violations. If you don't care about running tests on Windows, we can abandon this diff right now.
Also: If anyone wants to speed up the process: Please check how RPATH is set/used in KF5/KDevelop5. Maybe my above pain points are not valid anymore, at which point this patch would become much more acceptable to me.
kdevplatform_add_plugin() doesn't do anything with RPATH and just calls add_library() internally. This means that CMake uses default RPATH settings, which, according to the documentation, are:
By default if you don't change any RPATH related settings, CMake will link the executables and shared libraries with full RPATH to all used libraries in the build tree. When installing, it will clear the RPATH of these targets so they are installed with an empty RPATH
If i understand this right, running stuff from build tree will always be OK. Now, to be OK when running installed apps, we can set INSTALL_RPATH_USE_LINK_PATH property for plugin target. I guess, this should eliminate problems you are talking about.
Useful links:
https://cmake.org/Wiki/CMake_RPATH_handling
https://cmake.org/cmake/help/v3.4/variable/CMAKE_INSTALL_RPATH_USE_LINK_PATH.html
https://cmake.org/cmake/help/v3.4/prop_tgt/INSTALL_RPATH_USE_LINK_PATH.html
Sorry for the delay. Using the RPATH + SOVERSION could really help. And no, I don't want you to abandon this effort, we do violate the ODR after all, even on Linux. So this must be fixed. So if we implement the two cmake related fixes and call the lib KDevClangPrivate like Kevin proposed then I'm OK with biting the bullet and accepting this.
Thanks again!
@arrowdodger: So I suggest adding a add_private_library macro to languages/clang/CMakeLists.txt which sets RPATH + SONAME as we like it, and use the macro: E.g. add_private_library(KDevClangPrivate ...)
We can then later move this macro to kdevplatform.
Rest LGTM
languages/clang/CMakeLists.txt | ||
---|---|---|
32 | Lowercase ${target}? See string(... TOLOWER ...) in CMake | |
35 | And this here should use the plugin version as (SO)VERSION. | |
39 | This library should probably go to the normal library install prefix. | |
128 | As discussed on IRC, IMO hard-coding RPATH in the installed lib is a no-go. Discouraged by distro packagers. @mwolff: Agreed? Just using a proper SOVERSION should solve your issue. Also see: https://wiki.debian.org/RpathIssue | |
languages/clang/clangsettings/clangsettingsmanager.h | ||
30 | As said above, lowercase + strip kdev prefix for consistency reasons with all the other export headers in kdevplatform/kdevelop. -> 'clangprivateexport.h' |
Could we maybe ask kde-buildsystem for input and guidance? I'm far from an expert in this area.