CMake: explicitly link the atomic library

[bulwahn]

For arm & mips architecture, the linker must explicitly be asked to link the atomic library (with -latomic).

Otherwise, the linking fails with:

| devel/lib/libmavros.so: undefined reference to `__atomic_load_8'
| devel/lib/libmavros.so: undefined reference to `__atomic_store_8'
| collect2: error: ld returned 1 exit status

Linking atomic unconditionally as library is strictly needed only for arm & mips, but it seems not to imply any further differences with other architectures. Hence, this commit simply adds atomic unconditionally for a uniform handling of all machine architectures.

This is an alternative solution to the proposed solution in #790.

The issue was discovered cross-compiling mavros in meta-ros, the OpenEmbedded layer for ROS. Some further pointers are available at:

ros/meta-ros#525

[vooon]

I think that you should link atomic to libmavconn, it's first usage and that automatically adds lib to other processes.
Also perhaps better to use conditional flags, maybe cmake module?

[TSC21]

I think that you should link atomic to libmavconn, it's first usage and that automatically adds lib to other processes.
Also perhaps better to use conditional flags, maybe cmake module?

@vooon I tried to address this on #790. Usage of a cmake module I don't think is required. We can check for the TARGET arch and if it is arm or mips or related, it just adds -latomic.

[TSC21]

@bulwahn can you please follow @vooon 's suggestion and add the link to libmavconn CMakeLists.txt instead? If after you test it it goes well, we can merge it already. Thanks

[TSC21]

I've closed #790 as the complexity to find the TARGET arch on CMake wouldn't add anything beneficial to the building process. If linking the library in this general fashion would break the build, then we would have to have a condition based on the target, but that's not the case.

[bulwahn]

Okay, so this solution is preferred to #790. We will not try to make an architecture-specific solution, as @TSC21 has pointed out the difficulties in #797. I will check if linking in libmavconn works to resolve the issue as well, and then come back here with my findings.

[bulwahn]

I patched libmavconn and re-ran the cross-compile build tool chain on arm with this patch applied:

---
 libmavconn/CMakeLists.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/libmavconn/CMakeLists.txt b/libmavconn/CMakeLists.txt
index 9f9bbfe..2250348 100644
--- a/libmavconn/CMakeLists.txt
+++ b/libmavconn/CMakeLists.txt
@@ -49,6 +49,7 @@ add_library(mavconn
   src/tcp.cpp
 )
 target_link_libraries(mavconn
+  atomic
   ${Boost_LIBRARIES}
   ${console_bridge_LIBRARIES}
 )
-- 
1.9.1

With this patch applied, the build of mavros (although it depends on libmavconn) continues to fail with the linking error mentioned above. Maybe, I misunderstood your suggestion to simply add the target in mavconn, but this patch does not work. libmavconn does not need atomic when it is built, but only when mavros links everything together atomic is needed for linking. I don't know CMake well enough (yet) to know a good solution to that problem.
As libmavconn is not used heavily in other applications other than mavros, possibly the initially suggested solution is light-weight and simple, although not a general solution to all applications that depend on libmavconn.

[TSC21]

Yeah I also though that when the linking happened, there was not going to be an issue when building the mavros package. Maybe @vooon has something to suggest.

[vooon]

Seems that atomic 32 and 64 bit are implemented (mavconn uses atomic size_t and bool).
And failed at UAS ap type etc. 8-bit variables.

So we may have same problem with mavconn but not on arm's. So lgtm.

[oceanusxiv]

@vooon I would just like to mention here that explicitly linking atomic actually breaks the build on Mac, as Mac doesn't have an explicit libatomic to link against. I know that ROS isn't officially supported on Mac and mavros isn't either, but for the sake of general cross platform compatility it may be prudent to add a platform check before explicitly linking atomic.

[TSC21]

@eric1221bday that was our first approach but we ended up seeing that would be an overkill in terms of development - #790 (comment). If you have the bandwidth to develop a general approach to this, great. A PR is welcomed.