-
Notifications
You must be signed in to change notification settings - Fork 9
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
Conversation
There was a problem hiding this 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.
@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? |
|
Co-authored-by: Dmitry Kalinkin <[email protected]>
Changed. |
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. |
I did some follow-on work for this issue in a different branch, now here: #264 |
### 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]>
### 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]>
Without linking, the shared library only 'happens to work' in older gcc versions because no NEEDED entries are included in the shared library:
This causes linking errors in EICrecon (undefined references to ROOTFrameWriter functions).
After including podio in the linking, the necessary entries are there:
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.