search

ChimeraTK / ControlSystemAdapter

An adapter layer which allows to use control applications with different control system software environments.
GNU Lesser General Public License v3.0
3 stars 2 forks source link

Require libatomic on Linux only. #14

Closed smarsching closed 5 years ago

smarsching commented 5 years ago

This library is neither available nor required on some other platforms (e.g. macOS), so we must only add this dependency when we actually need it.

@mhier Could please review this? I think this change is pretty straight forward, but the comment about "some platforms" needing libatomic confused me, so maybe you know whethere there are some other platforms (in addition to Linux) where we need it.

mhier commented 5 years ago

Can you please clarify why this change is required? Does the library not exist, or does it hurt linking against it on macOS? If it simply doesn't exist, wouldn't it be better to still search for it but ignore if it is not there? Since it is not set to REQUIRED, shouldn't this already be the case? This would avoid issues on non-Linux platforms which need it as well as on Linux flavours which might not need it (e.g. a non-GNU Linux with a pure clang toolchain).

smarsching commented 5 years ago

The library simply does not exist on macOS. You would think that this is not a problem because the library is not marked as required, but with the find_library line present (and unconditional), I get the following error message from CMake:

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
atomic-library
    linked by target "ChimeraTK-ControlSystemAdapter" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testUnidirectionalProcessArray" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testTypeChangingDecorator" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testControlSystemSynchronizationUtility" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testReferenceTestCoreWithTread" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testPersistentDataStorage" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testPerformance" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testBidirectionalProcessArray" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testDeviceSynchronizationUtility" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "stresstest" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testAsyncRead" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "testPVManager" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
    linked by target "fullStubTest" in directory /Users/termi/Documents/workspace/ChimeraTK-ControlSystemAdapter
mhier commented 5 years ago

Would it work to set ${atomic-library} to an empty string if it is not found? Or maybe we could try something like this: https://reviews.llvm.org/D10453

The decision has btw. nothing to do with Linux. The library is required on some platforms, but even on Linux it is not required on amd64. If it is present, it simply doesn't hurt to link against it even if it is not required. Maybe clang always has the library built into its default libraries.

smarsching commented 5 years ago

That's a good idea. I changed this branch to simply set atomic-library when it is undefined. Do you like this better now?

mhier commented 5 years ago

Looks good!