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.

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

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

Closed
wants to merge 2 commits into from

Conversation

wdconinc
Copy link
Member

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.

Copy link
Contributor

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

I've arrived to the same conclusion independently.

CMakeLists.txt Outdated Show resolved Hide resolved
@wdconinc
Copy link
Member Author

@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
Copy link
Collaborator

@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.

Co-authored-by: Dmitry Kalinkin <[email protected]>
@wdconinc
Copy link
Member Author

I would say always use target_link_libraries.

Changed.

@wdconinc
Copy link
Member Author

wdconinc commented Nov 20, 2023

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
Copy link
Collaborator

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

github-merge-queue bot pushed a commit to eic/EICrecon that referenced this pull request Nov 21, 2023
### Briefly, what does this PR introduce?
This fixes an issue in the linking of algorithms_test which popped up
today (and likely was always there...). Until
JeffersonLab/JANA2#255 is included, we install a
libJANA.so with undefined references to podio because it doesn't include
it in the NEEDED section. By passing `--no-as-needed` we don't strip the
podio calls from the algorithms_test executables where they are unused.

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

---------

Co-authored-by: Dmitry Kalinkin <[email protected]>
wdconinc added a commit to eic/EICrecon that referenced this pull request Nov 22, 2023
### Briefly, what does this PR introduce?
This fixes an issue in the linking of algorithms_test which popped up
today (and likely was always there...). Until
JeffersonLab/JANA2#255 is included, we install a
libJANA.so with undefined references to podio because it doesn't include
it in the NEEDED section. By passing `--no-as-needed` we don't strip the
podio calls from the algorithms_test executables where they are unused.

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

---------

Co-authored-by: Dmitry Kalinkin <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants