Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Link atomic to ublox_gps #164

Open
wants to merge 1 commit into
base: ros2
Choose a base branch
from
Open

Link atomic to ublox_gps #164

wants to merge 1 commit into from

Conversation

kenji-miyake
Copy link

Currently, if I build this repository with Clang, it causes errors of undefined reference to some __atomic_* functions.

$ CXX=clang++ colcon build
...
/usr/bin/ld: libublox_gps.so: undefined reference to `__atomic_load'
/usr/bin/ld: libublox_gps.so: undefined reference to `__atomic_store'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
gmake[2]: *** [CMakeFiles/ublox_gps_node.dir/build.make:286: ublox_gps_node] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:167: CMakeFiles/ublox_gps_node.dir/all] Error 2

According to the Clang documentation, we have to link it manually.

Clang does not currently automatically link against libatomic when using libgcc_s. You may need to manually add -latomic to support this configuration when using non-native atomic operations (if you see link errors referring to _atomic* functions).

I know that Clang isn't the main target, but as I guess it doesn't affect GCC, I think it's good to link it.

Link atomic to ublox_gps
49c76e8 
Signed-off-by: Kenji Miyake <[email protected]>
clalancette
clalancette requested changes Sep 7, 2022
View reviewed changes
Copy link
Collaborator

@clalancette clalancette left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to adding the link to atomic here, but I'm guessing doing it this way is going to cause problems. There's a couple of issues that I see:

  1. We have no package.xml dependency to bring in atomic, so I'm not sure this is going to be available on all platforms. In particular, the buildfarm uses a heavily restricted environment, and only installs things which are explicitly mentioned in the package.xml.
  2. Similarly, we have nothing in the CMakeLists.txt that does a find_package to get this library. So this may not exist.
  3. Finally, we heavily discourage the use of these kinds of open variables. The problem is that they cause hard-coded paths in the output, which isn't a problem when you are building from source, but is a problem when we are generating binary packages for distribution.

Overall, I'd say to get this in, we need some kind of package that provides it (listed in the package.xml), some kind of find_package call to find the library, and then a modern CMake target (atomic::atomic) to link against. All of these could be optional one way or another depending on the platform, but this will need looking into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kenji-miyake @clalancette