Link libosrm_guidance correctly and work around circular dependencies #6955
Conversation
… between guidance and extract, fixes Project-OSRM#6954
|
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 |
There was a problem hiding this comment.
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?
|
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? |
|
This still doesn't work—the build succeeds, but |
|
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. |
|
If we did want to go the monolithic library route, I tried that on my |
I have no particular insight off the top of my head. |
|
FWIW, with the monolithic library, |
|
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. |
|
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 |
|
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. |
|
I would be very interested in the Julia library, is there any chance this can be pushed over the finish line? |
|
@jrklasen thanks for your interest! It is mostly complete and several of my students are already using it. There is an OSRM binary distributed with it; it's not in Julia General yet but after adding the additional repository installing it is just ]add OSRM. See documentation here: https://github.com/mattwigway/OSRM.jl There are a few things that remain for a final release:
I'd be happy to collaborate if you're interested! @jeremiahpslewis had also expressed interest (and did a lot of the work on the binary packaging). |
|
The Julia library looks super interesting. Thanks for sharing. |
|
@mattwigway that's really great! I will definitely check it out, having it in the general registry would be needed, though, to use it in production. My C++ skills are somewhat limited, so I'm not sure, I can be of any help on this end. I could try to spare some time to write tests. I fear it will take a view weeks until I manage to do so, however. |
|
Thanks! The main reason I haven't put it into General yet is that I wanted
to get the two JLLs and the Julia package all stable to avoid having a
bunch of different versions of them in General, but I think that's pretty
close to done (though having a new tagged release of OSRM would be nice).
If you want to move this off the GitHub thread feel free to reach out over
email: ***@***.***
…On Fri, Nov 22, 2024, 6:13 AM Jonas Klasen ***@***.***> wrote:
@mattwigway <https://github.com/mattwigway> that's really great! I will
definitely check it out, having it in the general registry would be needed,
though, to use it in production. My C++ skills are somewhat limited, so I'm
not sure, I can be of any help on this end. I could try to spare some time
to write tests. I fear it will take a view weeks until I manage to do so,
however.
—
Reply to this email directly, view it on GitHub
<#6955 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEKNLRROWRQKH6VGXHDRPT2B4GULAVCNFSM6AAAAABJOYJ7JSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJTGUYDSNJQG4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
GitHub mangled the email address, it's mwbc at unc dot edu |
|
Depending on your plans, we could discuss moving the plugin repo under the OSRM org. |
|
I would be fine with that.
…On Fri, Nov 22, 2024, 5:01 PM Dennis Luxen ***@***.***> wrote:
Depending on your plans, we could discuss moving the plugin repo under the
OSRM org.
—
Reply to this email directly, view it on GitHub
<#6955 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEKNLVC6ZQ2SPFY2YIUNTL2B6SVNAVCNFSM6AAAAABJOYJ7JSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOJUHE2DAMBQGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Fixes #6954
Issue
Building shared libraries on macOS fails, because there is no
target_link_librariescall 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 thetarget_link_librariescall.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