Turn all clang related static libs into a single shared library.
ClosedPublic

Authored by arrowd on Jan 9 2016, 8:48 PM.

Details

Summary

Also link plugin to it, fix exports and make tests link to the shared lib.

Test Plan

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.
arrowd updated this revision to Diff 1819.Jan 9 2016, 8:48 PM
arrowd retitled this revision from to Turn all clang related static libs into a single shared library..
arrowd updated this object.
arrowd edited the test plan for this revision. (Show Details)
arrowd added a reviewer: KDevelop.
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJan 9 2016, 8:48 PM

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

arrowd added a comment.Jan 9 2016, 9:28 PM
In D772#14684, @kfunk wrote:

we have to pay attention to export macros again

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.

kfunk added a comment.Jan 10 2016, 1:26 PM

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.

arrowd updated this revision to Diff 1863.Jan 11 2016, 12:32 PM

Actually install shared library along with plugin.

In D772#14865, @mwolff wrote:

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.

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?

arrowd updated this revision to Diff 1900.Jan 12 2016, 3:00 PM

Remove CMakeLists.txt in subdirs.

arrowd marked an inline comment as done.Jan 12 2016, 3:27 PM

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.

In D772#15394, @mwolff wrote:

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.

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.

In D772#15394, @mwolff wrote:

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.

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!

kfunk added a comment.EditedJan 19 2016, 12:03 PM

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

arrowd updated this revision to Diff 2057.Jan 21 2016, 5:10 PM

Add add_private_library() macro.

kfunk added a comment.Jan 21 2016, 5:35 PM

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'

arrowd added inline comments.Jan 21 2016, 6:36 PM
languages/clang/CMakeLists.txt
128

I read that. RPATH would break if the installed app is moved, yes, but it wouldn't harm or break anything. @mwolff wanted it as additional safety belt, if i understood him correctly.

Could we maybe ask kde-buildsystem for input and guidance? I'm far from an expert in this area.

kfunk requested changes to this revision.Feb 2 2016, 10:21 PM
kfunk added a reviewer: kfunk.

@arrowdodger: Could you perform the requested changes?

This revision now requires changes to proceed.Feb 2 2016, 10:21 PM
arrowd updated this revision to Diff 2185.Feb 4 2016, 9:39 AM
arrowd edited edge metadata.

Address comments.

arrowd marked 4 inline comments as done.Feb 4 2016, 9:40 AM
This revision was automatically updated to reflect the committed changes.