libosrm breaking change in master after v5.22.0

[jcoupey]

Hi there. I'm using libosrm with a simple consumer code that comes down to something like:

osrm::EngineConfig config;
osrm::OSRM osrm(config);

osrm::TableParameters params;
// Fill params with coordinates

osrm::json::Object result;
osrm::Status status = osrm.Table(params, result);

This works perfectly against tagged versions of OSRM up to v5.22.0, but fails when building libosrm from current master with something like:

error: no matching function for call to ‘osrm::OSRM::Table(osrm::engine::api::TableParameters&, osrm::util::json::Object&) const’
   osrm::Status status = _osrm.Table(params, result);
                                                   ^
/usr/local/include/osrm/osrm.hpp:96:12: note: candidate: osrm::engine::Status osrm::OSRM::Table(const osrm::engine::api::TableParameters&, osrm::engine::api::ResultT&) const
     Status Table(const TableParameters &parameters, osrm::engine::api::ResultT &result) const;
            ^~~~~

Indeed, the Table interface has been changed in this commit.

I didn't dig enough to know whether switching from osrm::json::Object to osrm::engine::api::ResultT would be straithforward, but this looks like a breaking change anyway. All builds relying on previous libosrm interface will fail in the same way if this change makes it into a release.

FWIW, original problem spotted at VROOM-Project/vroom#344.

[akashihi]

Yes, it is caused by providing more effective serialization with osrm. in your case you can continue using json by doing something like:

engine::api::ResultT json_result = json::Object();
osrm::Status status = osrm.Table(params, result);
auto &result = json_result.get<json::Object>();

[jcoupey]

Thanks @akashihi for your feedback on this. I'm not questioning the internal changes and motivation here, my point is that having a breaking API change is a problem as such.

OSRM explicitly uses semantic versioning. It's advertised in CONTRIBUTING.md and it has always been a key consideration for the core team not to break the API in the past few years. Thus all downstream libosrm users will be affected by problems they would not expect without a major version change.

Are there maybe ways to circumvent the problem, i.e. provide those serialization changes by extending the API in a retro-compatible way?

[akashihi]

As it was mentioned earlier [https://github.com//issues/5548#issuecomment-531247180]" projects that are afraid to change something in their API's, usually lose their relevance and disappear."

This API change is more extension than really a break and it is not yet officially released, so not semver rules are broken yet. I agree that such a big change deserves to be released under 6.x version :)

[jcoupey]

This is not about being "afraid to change something" in the API, but rather about how the changes are handled. One may argue that projects that break their API lose the trust of users. ;-)

This API change is more extension than really a break

I don't see it this way because previously working code that uses libosrm simply does not compile any more due to a signature change when calling the plugins. In this commit, the example client code for libosrm was adjusted to the new API, which would not have been required without a breaking change.

it is not yet officially released, so not semver rules are broken yet

Totally, the point of this issue is to raise awareness on a potential breaking change for future releases.

[akashihi]

This API change is more extension than really a break

I don't see it this way because previously working code that uses libosrm simply does not compile any more due to a signature change when calling the plugins.

Right! What i meant - the old API is hidden below the new API, thus pretty trivial change is required, not the whole rewrite of the client code from scratch. And even more - introducing that ResultT wrapper will allow future API extensions without breaking it again.

it is not yet officially released, so not semver rules are broken yet

Totally, the point of this issue is to raise awareness on a potential breaking change for future releases.

I completely agree with you. When (if?) a next release will be made, that breaking API change needs to be stated loudly.

[jcoupey]

It would be useful to have the views of some of the (remaining) core devs here. Similarly to other issues suffering from #5209, the problem is that now that Mapbox has dropped its efforts into OSRM, a clear road-map and decision process for this kind of change is missing.

[danpat]

I generally agree that this is a change that breaks the API sufficiently to indicate a major version bump - old code should always recompile without change against libosrm that's within a SemVer compatible range.

The right way to implement the FlatBuffer interface and maintain compatibility would be to move detection of whether the user requested JSON or FlatBuffeer format up into the HTTP layer, then provide a new overload of the various methods that took a different return parameter type.

The big hint in the PR that a major API change was made was that example/example.cpp needed to be changed to work with the new interface. That should only be necessary if the example was demonstrating some new feature, or if the major version bumped.

The HTTP interface changes made in osrm-routed are fine - there is no SemVer major change there - a new parameter was added, which is fine. But the libosrm C++ API has changed in a major way.

If someone has time, it's should be possible to unbreak the C++ interface and still support the new feature - a few things need to be moved around.

[akashihi]

As original author of FlatBuffer interface i can do that unbreak, but could you please provide more information, on how do you think it needs to be done?

PS @danpat Could you please take a look at my other PRs? :)