Bug 94096 - CMake: support platforms where -lrt is not needed or where -lsocket is needed
Summary: CMake: support platforms where -lrt is not needed or where -lsocket is needed
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Hardware: ARM other
: low normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
: 90687 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-02-11 17:30 UTC by Eric
Modified: 2016-08-18 13:00 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Don't link to rt when building for Android (1.33 KB, text/plain)
2016-02-11 17:30 UTC, Eric
Details
Do not link dbus-1 and dbus-internal against librt and libsocket if not present. (2.10 KB, patch)
2016-08-16 22:05 UTC, Ralf Habacker
Details | Splinter Review
Fix building with CMake for a Unix platform that does not have -lrt, such as Android. (1.81 KB, patch)
2016-08-17 16:52 UTC, Ralf Habacker
Details | Splinter Review
Fix building with CMake for a Unix platform where functions like recv() are in a separate -lsocket, like QNX. (1.34 KB, patch)
2016-08-17 16:52 UTC, Ralf Habacker
Details | Splinter Review

Description Eric 2016-02-11 17:30:30 UTC
Created attachment 121683 [details]
Don't link to rt when building for Android

Patch fixes linker issue when building for Android...


[ 36%] Linking C shared library ../lib/libdbus-1.so
F:/android/android-ndk-r10e/toolchains/arm-linux-androideabi-4.9/prebuilt/windows-x86_64/bin/../lib/gcc/arm-linux-androideabi/4.9/../../../../arm-linux-androideabi/bin/ld.exe: error: cannot find -lrt
collect2.exe: error: ld returned 1 exit status
make[2]: *** [lib/libdbus-1.so] Error 1
make[1]: *** [dbus/CMakeFiles/dbus-1.dir/all] Error 2
make: *** [all] Error 2
Comment 1 Simon McVittie 2016-02-11 20:11:33 UTC
The CMake build system is intended for builds that run on either Windows or GNU/Linux. I would recommend the Autotools build system for portable builds and/or cross-compilation.

Building on a Windows build system, cross-compiling for an Android host system, using the CMake build system is way outside anything we've ever actively supported.
Comment 2 Simon McVittie 2016-02-12 13:41:09 UTC
(In reply to Simon McVittie from comment #1)
> The CMake build system is intended for builds that run on either Windows or
> GNU/Linux. I would recommend the Autotools build system for portable builds
> and/or cross-compilation.

See also Bug #90687, which is targeting QNX rather than Android but the principles are the same. In particular, <https://bugs.freedesktop.org/show_bug.cgi?id=90687#c5> and <https://bugs.freedesktop.org/show_bug.cgi?id=90687#c9> describe the approach we should take for -lrt (and, for QNX, -lsocket).

https://bugs.freedesktop.org/show_bug.cgi?id=90687#c11 suggests that something like this might work:

    # for clock_getres() on e.g. GNU/Linux (but not Android)
    find_library(LIBRT rt)
    if(!LIBRT)
      target_link_libraries(dbus-1 ${LIBRT})
    endif()

    # for socket() on QNX
    find_library(LIBSOCKET socket)
    if(LIBSOCKET)
      target_link_libraries(dbus-1 ${LIBSOCKET})
    endif()

I know you don't need the socket part yourself, but if you add this code for -lsocket at the same time as -lrt, then normal GNU/Linux builds will test the "we found this library" code path for -lrt, and the "we didn't find this library" code path for -lsocket. Code that isn't tested doesn't work, except by mistake, so regularly testing both is valuable.

(Your patch also doesn't link to ${CMAKE_THREAD_LIBS_INIT} on Android - but that variable is never set anyway, so it should just be deleted altogether, and not used on GNU/Linux either.)
Comment 3 Eric 2016-02-12 14:31:17 UTC
Hi Simon, thank you for responding so quickly to my reports. Yes, I am cross compiling for Android from Windows using Google's toolchain (https://code.google.com/archive/p/android-cmake/).  We appreciate Google's work here as it saves us from writing new build system code just for Android.  DBus is a fantastic project that we would like to use without having to patch the source locally before building.  Your help in this regard is much appreciated.

What could help is if we could not try to link against rt unless the package is found, like:

find_library(LIBRT rt)
if(LIBRT)
  target_link_libraries(dbus-1 ${LIBRT})
endif()

Is the "if(!LIBRT)" a typo?
Comment 4 Simon McVittie 2016-02-12 18:57:26 UTC
(In reply to Eric from comment #3)
> Is the "if(!LIBRT)" a typo?

Er, yes, I meant if(LIBRT), similar to the LIBSOCKET one.

Please try that (or if it doesn't work, adjust it until it does) and see whether that solves this for you.
Comment 5 Ralf Habacker 2016-02-13 18:36:39 UTC
(In reply to Simon McVittie from comment #2)
> 
>     # for clock_getres() on e.g. GNU/Linux (but not Android)
>     find_library(LIBRT rt)
>     if(!LIBRT)

According to https://cmake.org/cmake/help/v3.0/command/find_package.html?highlight=_found this need to be

     if(LIBRT_FOUND)


>     find_library(LIBSOCKET socket)
>     if(LIBSOCKET)

if(LIBSOCKET_FOUND)
Comment 6 Eric 2016-03-07 15:20:22 UTC
Yes, this solves this for me and is a better solution - thank you.
Comment 7 Simon McVittie 2016-03-08 14:40:09 UTC
(In reply to Eric from comment #6)
> this solves this for me and is a better solution

What is the "this" that solves it for you?

If you have a revised patch (perhaps based on Comment #2, Comment #3, Comment #5), please propose it here - I'd like to merge a change that does the right thing for -lrt and -lsocket.

If I have to guess at the precise details of how you adapted my pseudocode into a specific patch, sorry but it isn't going to get merged.
Comment 8 Ralf Habacker 2016-08-16 21:42:29 UTC
(In reply to Ralf Habacker from comment #5)
>      if(LIBRT_FOUND)
> if(LIBSOCKET_FOUND)

Sorry, I was wrong and Simon was correct. My comment is valid for find_package.
Comment 9 Ralf Habacker 2016-08-16 22:05:43 UTC
Created attachment 125831 [details] [review]
Do not link dbus-1 and dbus-internal against librt and libsocket if not present.

This fixes a cross link issue on windows for android.

Bug:
Comment 10 Ralf Habacker 2016-08-16 22:06:07 UTC
Comment on attachment 121683 [details]
Don't link to rt when building for Android

superseeded
Comment 11 Simon McVittie 2016-08-17 08:02:55 UTC
Comment on attachment 125831 [details] [review]
Do not link dbus-1 and dbus-internal against librt and libsocket if not present.

Review of attachment 125831 [details] [review]:
-----------------------------------------------------------------

The code changes are fine, the commit message could do with some improvement.

> Do not link dbus-1 and dbus-internal against librt and libsocket if not present

That's not actually what this patch does. It is making two changes:

* Previously, we linked to librt unconditionally. Now, only link it if present.
* Previously, we never linked to libsocket. Now, link it if present.

I'd prefer those to be two separate commits. The first is necessary when building with CMake for a Unix platform that does not have -lrt, such as Android. The second is necessary when building with CMake for a Unix platform where functions like recv() are in a separate -lsocket, like QNX.

> This fixes a cross link issue on windows for android.

For the librt change: "This lets us build using CMake for an Android host, for instance on Windows" or something. Whether the build platform is Windows is actually not relevant.

For the addition of socket, similarly: "This lets us build using CMake for a QNX host".
Comment 12 Ralf Habacker 2016-08-17 16:52:50 UTC
Created attachment 125851 [details] [review]
Fix building with CMake for a Unix platform that does not have -lrt, such as Android.

Bug:
Comment 13 Ralf Habacker 2016-08-17 16:52:54 UTC
Created attachment 125852 [details] [review]
Fix building with CMake for a Unix platform where functions like recv() are in a separate -lsocket, like QNX.

Bug:
Comment 14 Ralf Habacker 2016-08-17 16:53:31 UTC
Comment on attachment 125831 [details] [review]
Do not link dbus-1 and dbus-internal against librt and libsocket if not present.

superseeded
Comment 15 Simon McVittie 2016-08-17 17:05:09 UTC
Separate patches look good, thanks.
Comment 16 Simon McVittie 2016-08-17 17:08:40 UTC
*** Bug 90687 has been marked as a duplicate of this bug. ***
Comment 17 Ralf Habacker 2016-08-18 13:00:12 UTC
Comment on attachment 125852 [details] [review]
Fix building with CMake for a Unix platform where functions like recv() are in a separate -lsocket, like QNX.

submitted to master
Comment 18 Ralf Habacker 2016-08-18 13:00:22 UTC
Comment on attachment 125851 [details] [review]
Fix building with CMake for a Unix platform that does not have -lrt, such as Android.

submitted to master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.