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

Link libosrm_guidance correctly and work around circular dependencies #6955

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mattwigway
Copy link
Contributor

@mattwigway mattwigway commented Jun 17, 2024

Fixes #6954

Issue

Building shared libraries on macOS fails, because there is no target_link_libraries call for osrm_guidance to tell it what external and internal libraries to link against. This works on Linux because the linker assumes undefined symbols will be defined at runtime, but macOS checks that the symbols are defined at link time. This PR adds the target_link_libraries call.

This creates another problem in that there is a circular dependency between osrm_guidance and osrm_extract. osrm_guidance calls a lot of functions from osrm_extract, and osrm_extract only calls two from osrm_guidance. So I fix this by specifying linker options to tell the linker to resolve those two functions at runtime. I've only tested this with clang on macOS, but I think it should work on other platforms as well. It looks like the CI currently doesn't try to build shared libraries on any platform, perhaps we should change that?

Tasklist

Requirements / Relations

None

@mattwigway
Copy link
Contributor Author

Test failures are because lua.org is down right now.

# We don't need to do this for osrm_guidance, as osrm_extract will already be built when that gets
# built and symbols can be looked up in osrm_extract directly.
# https://stackoverflow.com/questions/25421479/
target_link_libraries(osrm_extract
Copy link
Member

@SiarheiFedartsou SiarheiFedartsou Jun 18, 2024

Choose a reason for hiding this comment

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

Hey!
Any chance it is possible to resolve this somehow differently (in a less hacky way)? May be we could easily extract those 2 functions to separate library?

@mattwigway
Copy link
Contributor Author

mattwigway commented Jun 18, 2024 via email

@SiarheiFedartsou
Copy link
Member

I'll take a look to see if we can move them to osrm_extract, I'm not sure if they depend on other things in osrm_guidance. Or a more drastic change would be too put osrm_guidance and osrm_extract together as a single library, that would definitely work. Alternately, could we just build a single monolithic libosrm? I don't know the history of why there's multiple libraries. I guess it saves some memory, maybe that's relevant on mobile devices?

On Tue, Jun 18, 2024, 7:26 AM Siarhei Fedartsou @.> wrote: @.* commented on this pull request. ------------------------------ In CMakeLists.txt <#6955 (comment)> : > @@ -566,7 +566,17 @@ set(UTIL_LIBRARIES target_link_libraries(osrm ${ENGINE_LIBRARIES}) target_link_libraries(osrm_update ${UPDATER_LIBRARIES}) target_link_libraries(osrm_contract ${CONTRACTOR_LIBRARIES} osrm_update osrm_store) -target_link_libraries(osrm_extract osrm_guidance ${EXTRACTOR_LIBRARIES}) + +# There is a circular dependency between osrm_extract and osrm_guidance. Tell the linker to trust us +# that the two osrm::guidance functions used in osrm_extract will be available at runtime. +# We don't need to do this for osrm_guidance, as osrm_extract will already be built when that gets +# built and symbols can be looked up in osrm_extract directly. +# https://stackoverflow.com/questions/25421479/ +target_link_libraries(osrm_extract Hm, any chance it is possible to resolve this somehow differently (in a less hacky way)? May be we could easily extract those 2 functions to separate library? — Reply to this email directly, view it on GitHub <#6955 (review)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEKNLQARMKPA57GAI5Q7NDZIAKQFAVCNFSM6AAAAABJOYJ7JSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMRVGIZTSOJYGA . You are receiving this because you authored the thread.Message ID: @.***>

@DennisOSRM may be you know?

@mattwigway mattwigway marked this pull request as draft June 18, 2024 12:27
@mattwigway
Copy link
Contributor Author

This still doesn't work—the build succeeds, but osrm-extract is not loading osrm_guidance.dylib:

dyld[69643]: symbol not found in flat namespace '__ZN4osrm8guidance13annotateTurnsERKNS_4util12DynamicGraphINS1_17NodeBasedEdgeDataEEERKNS_9extractor6detail30EdgeBasedNodeDataContainerImplILNS_7storage9OwnershipE0EEERKNSt3__16vectorINS1_10CoordinateENSF_9allocatorISH_EEEERKNS7_23CompressedEdgeContainerERKNSF_13unordered_setIjNSF_4hashIjEENSF_8equal_toIjEENSI_IjEEEERKNS7_18NodeRestrictionMapINS7_17UnconditionalOnlyEEERKNS7_17WayRestrictionMapERKNS8_13NameTableImplILSB_0EEERKNS7_11SuffixTableERKNSF_5tupleIJNSG_IjSV_EENSG_ItNSI_ItEEEEEEERNS1_15ConcurrentIDMapIS1H_tNSR_IS1H_EEEERNS1L_INS1_8guidance15LaneTupleIdPairEtNSR_IS1Q_EEEERNS0_6detail21TurnDataContainerImplILSB_2EEERS1F_RNS1L_INS1P_12BearingClassEjNSR_IS1Z_EEEERNS1L_INS1P_10EntryClassEtNSR_IS23_EEEERj'
Abort trap: 6

@mattwigway
Copy link
Contributor Author

Okay, the binaries work now. But I agree with @SiarheiFedartsou that this feels hacky. The guidance functions used in extract do call a number of other guidance functions, so moving them to extract is a bit of work, but I could do that if desired. Alternately, guidance already depends on extract in a bunch of places, so it could make sense to just combine them. Since you need both libraries to do anything with either this doesn't seem like it would be much of an issue.

mattwigway added a commit to mattwigway/osrm-backend that referenced this pull request Jun 18, 2024
@mattwigway
Copy link
Contributor Author

If we did want to go the monolithic library route, I tried that on my monolithic-library branch and it seems to work fine.

@DennisOSRM
Copy link
Collaborator

@DennisOSRM may be you know?

I have no particular insight off the top of my head.

@mattwigway
Copy link
Contributor Author

FWIW, with the monolithic library, libosrm.dylib is 7.7mb when built for ARM64 Mac, so not ridiculously large even for a mobile app.

@mattwigway
Copy link
Contributor Author

It looks like the multiple library setup was created by #1883, but I don't see any discussion there of the merits of multiple libraries vs. a single monolithic library.

@DennisOSRM
Copy link
Collaborator

DennisOSRM commented Jun 19, 2024

It's too long ago to fully remember. It may have had benefits wrt build time or incremental builds, or max memory usage during builds at the time. That's an educated guess only, tho

@DennisOSRM
Copy link
Collaborator

I was able to find an old note that may shed sone light on the history of the change. Please take it with a grain of salt.

Back then some of the compile units were built and linked into several binaries, eg executables and test binaries. CMake didn't support object libraries back then and for some time there was the issue that the same code was compiled and linked more than once. The libraries helped get rid of the the multiple compilation issue. I think I remember compile time being an annoyance.

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.

Can't build shared libraries on macOS
3 participants