Made compatible with new osrm::engine::api::ResultT

daniel-j-h / libosrmc

Pure C bindings for libosrm

MIT License

18 stars 8 forks source link

Made compatible with new osrm::engine::api::ResultT #17

Open akashihi opened 4 years ago

akashihi commented 4 years ago

Closes #11

Closes #5548

daniel-j-h commented 4 years ago

If we merge these changes in we fix compilation against libosrm master but break compilation against stable releases and past libosrm releases. I the following steps moving forward:

the osrm folks make a proper release
we conditionally compile for their old / new api
libosrmc can then be kept api and abi compatible

akashihi commented 4 years ago

Ok, let's wait for OSRM release

akashihi commented 3 years ago

OSRM new version is released, should this one be merged?

daniel-j-h commented 3 years ago

@akashihi I'm no longer involved with osrm and/or this library. If you want, I can give you commit access to this repo. so you are not blocked by me not being responsive here.

If we can keep the API/ABI stable here, that would be my number one goal. The implementation is free to change. But then we should conditionally compile the now code. What I mean is

if the user has the libosrm C++ headers for 5.23+ we need to compile with the new ResultT changes
if the user has the libosrm C++ headers for <5.23+ we need to compile with the old interface

or we say we only support 5.23+ from now on, but this breaks out understanding of ABI/API compatibility across OSRM versions.

akashihi commented 3 years ago

Not that i'm really working on OSRM, but i think i'm obliged to fix that incompatibility, cause i caused it. I'll check if i can maintain compatibility by using some metaprogramming, but in any case plan B will be to use simple preprocessor for that.

PS Yes, write access will definitely help.

daniel-j-h commented 3 years ago

Coolio! I've added you as a collaborator to this repository :bow:

akashihi commented 3 years ago

Thank you! I'll try to find sometime for that issue in December

mster429 commented 3 years ago

Hi, can we get this pull request merged ? @daniel-j-h

daniel-j-h commented 3 years ago

Please read the conversation, we can not just merge it, it would break compilation against previous releases.

The osrm folks have changed their api and we need to conditionally compile against the old or new api now.

Until this is done, we can not merge this pull request.

mster429 commented 3 years ago

Got it. Thanks for your quick response @daniel-j-h

hogan-chu commented 2 years ago

Hello is there any improvement un this commt? I want to process this repository for recent osrm version For not use https Kind regards