JeffersonLab / JANA2

Multi-threaded HENP Event Reconstruction
https://jeffersonlab.github.io/JANA2/
Other
6 stars 9 forks source link

fix: link_libraries against podio, podioRootIO in tld CMakeLists.txt #255

Closed wdconinc closed 9 months ago

wdconinc commented 10 months ago

Without linking, the shared library only 'happens to work' in older gcc versions because no NEEDED entries are included in the shared library:

$ readelf -d /opt/software/linux-ubuntu23.10-skylake/gcc-13.2.0/jana2-2.1.1-2vzfopkybqjsifmxc2bodpydpv3bthiv/lib/libJANA.so 

Dynamic section at offset 0x167618 contains 31 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libCore.so.6.28]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]
 0x000000000000000e (SONAME)             Library soname: [libJANA.so]

This causes linking errors in EICrecon (undefined references to ROOTFrameWriter functions).

After including podio in the linking, the necessary entries are there:

$ readelf -d /opt/software/linux-ubuntu23.10-skylake/gcc-13.2.0/jana2-master-7j2vwskbvoq45ibyuvnlh2qr6gfuvpce/lib/libJANA.so 

Dynamic section at offset 0x1685f8 contains 33 entries:
  Tag        Type                         Name/Value
 0x0000000000000001 (NEEDED)             Shared library: [libCore.so.6.28]
 0x0000000000000001 (NEEDED)             Shared library: [libpodioRootIO.so]
 0x0000000000000001 (NEEDED)             Shared library: [libpodio.so]
 0x0000000000000001 (NEEDED)             Shared library: [libstdc++.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libm.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [libgcc_s.so.1]
 0x0000000000000001 (NEEDED)             Shared library: [libc.so.6]
 0x0000000000000001 (NEEDED)             Shared library: [ld-linux-x86-64.so.2]
 0x000000000000000e (SONAME)             Library soname: [libJANA.so]

Note: It may make sense to revamp the CMakeLists.txt here to avoid this blanket link_libraries approach, and switch to a more granular target_link_libraries approach. See the note at https://cmake.org/cmake/help/latest/command/link_libraries.html.

wdconinc commented 10 months ago

@nathanwbrei Any preference on link_libraries or target_link_libraries? Anyone who's working on a CMake overhaul along with adding JANA2_USE_PODIO to the target property COMPILE_DEFINITIONS already, by chance?

nathanwbrei commented 10 months ago

@wdconinc:

  1. We are overhauling JANA's CMake targets right now so that all target_link_libraries and target_compile_definitions and target_include_directories are correctly exported as part of their respective targets.
  2. I would say always use target_link_libraries. See the "CMake Antipatterns" section of https://cliutils.gitlab.io/modern-cmake/chapters/intro/dodonot.html
  3. When I was creating test cases for JOmnifactories, I ran into a lot of linker errors due to CMake reordering target_link_libraries. Turns out it "proved" that podio::podio COULDN'T be a dependency of JANA::jana2_whatever_lib because there were places in EICrecon where we were linking JANA but weren't also linking podio "to the right" of it. This is a mess, and I hate it, and I'm feeling motivated to declare my target properties properly to avoid this in the future.
wdconinc commented 10 months ago

I would say always use target_link_libraries.

Changed.

wdconinc commented 9 months ago

Can we just merge this? It causes compilation failures in our environments, and it takes time to debug issues, in particular when the bug has been reported, a pull request has been provided, but somehow it's waiting for something.

nathanwbrei commented 9 months ago

I did some follow-on work for this issue in a different branch, now here: #264