Upgrade libsol2 v2.20.6

[dburnsii]

Issue

This PR targets issue #5858 and updates Sol2, allowing OSRM to be built with GCC10

Tasklist

• CHANGELOG.md entry (How to write a changelog entry)
• update relevant Wiki pages
• add tests (see testing documentation
• review
• adjust for comments
• cherry pick to release branch

Requirements / Relations

Related: #5859
Related: #5884

[dburnsii]

Builds with GCC10 and almost passes tests, just have to account for the table being empty, potentially coming back as nil if no elements are added?

[akashihi]

Tests are failing on gcc7 and clang 5 and both are released like 3 years ago, so may be it worth dropping them?

As for nil, i haven't looked deeply yet, but seems to be a reasonable solution.

[dburnsii]

So I wouldn't want to drop support for those compilers, but what's interesting is that it's able to build just fine, but it's failing with the coverage builds. I'll look into why that's the case, the logs suggest issues with nil vs an empty table maybe.

[mloskot]

allowing OSRM to be built with GCC10

I think, more precisely is to allow sol2 to be built as C++17. Currently, it's not possible:

third_party/sol2/sol2/sol.hpp: error C2039: 'object_type': is not a member of...

[dburnsii]

Sol2 added C++17 support back in version [v2.18.0]*(https://github.com/ThePhD/sol2/releases/tag/v2.18.0). I think it's possible, just a few changes need to be made. So far, the build is working and with my latest commit, all tests pass except one.

[mloskot]

As far as I can tell, I tried today, the third_party/sol2/sol2/sol.hpp from v5.23 does not compile with /std:c++17 using the latest VS 2019.

[dburnsii]

So that's fair, I'd say it's worth investigating that as well. For now, my use case requires that OSRM can be built with GCC10, regardless of the C++ standard in use (my assumption would be C++17). What I've found is that the newer version of Sol2 seems not to like passing nil to properties that should have actual values. For instance, setting Way.name to nil fails, and that's tested through

osrm-backend/features/testbot/nil.feature

Line 3 in a3f1c2a

Scenario: Assign nil values to all way strings

.

I'm looking through the source to see if there's a way to fix this, to again allow strings to be set to nil, but I feel like an empty string is more appropriate here, considering when we clear a way, we're calling std::string::clear, which in turn sets the value to an empty string rather than nullptr.

My proposition here is that we just pull the ability to set the strings to nil (in a .lua profile) and instead set to an empty string if we want to clear the value. I've changed the test to reflect this functionality. If that's not an acceptable solution, I'm open to changing it with some guidance on where that change might need to happen.

[orivej]

C++17 build of the old sol can be fixed by this patch: https://gist.github.com/orivej/b7783dfd3d9b9269ee65152365c5d810

[dburnsii]

So I'm very close at this point, but I'm having a hard time replicating the nodejs test failure I'm getting. Output looks like this:

$ npm run nodejs-tests

> osrm@5.24.0-unreleased nodejs-tests /home/travis/build/Project-OSRM/osrm-backend

> make -C test/data && ./lib/binding/osrm-datastore test/data/ch/monaco.osrm && node test/nodejs/index.js | faucet

make: Entering directory `/home/travis/build/Project-OSRM/osrm-backend/test/data'

Verifiyng data file integrity...

../../scripts/md5sum.js -c data.md5sum

monaco.osm.pbf: OK

monaco.poly: OK

Running osrm-extract...

../../scripts/timer.js "osrm-extract\tmonaco.osrm" /home/travis/build/Project-OSRM/osrm-backend/build-osrm/osrm-extract monaco.osm.pbf -p ../../profiles/car.lua

[info] Parsed 0 location-dependent features with 0 GeoJSON polygons

[info] Using script ../../profiles/car.lua

[info] Input file: monaco.osm.pbf

[info] Profile: car.lua

[info] Threads: 4

[info] Parsing in progress..

[info] input file generated by osmconvert 0.7T

[info] timestamp: 2016-03-05T00:26:02Z

[info] Using profile api version 4

osrm-extract: /home/travis/build/Project-OSRM/osrm-backend/third_party/sol2/sol/optional_implementation.hpp:604: auto sol::optional<nullptr_t>::operator*()::(anonymous class)::operator()() const [T = nullptr_t]: Assertion `!"initialized()"' failed.

Aborted (core dumped)

But I can't seem to find the options for the compiler to get it to fail like this, and as such I'm having a hard time tracking down what's going wrong. This is really the last issue I see before this is ready to merge in.

[akashihi]

@dburnsii This looks like the biggest change i've ever seen

[akashihi]

Looks fine at a first glance. But do we need to put the whole sol into repo? I think it's a header only library, so sol2/single/sol.hpp (+license) should be enough

[dburnsii]

So the changes to the codebase are relatively minimal, but I did change one of the tests. The cast majority of changes here are because I switched to the Sol2 Git repo itself instead of just the released file. I figure that makes it pretty easy to upgrade in the future, and could help get the scripts/update_dependencies script working again. Using the full repo also matches the other third party libraries a little better.

I can instead just pull the consolidated file to reduce the changes here, if you're more comfortable with that.

[akashihi]

Oh, now i see reasons behind putting the whole repo, so i'm fine with it. As change is very intrusive, let's wait for a second opinion.
@mloskot @danpat

[OgreTransporter]

This merge break PR #5783

[OgreTransporter]

Version 2.20.6 is dated 28 Nov 2018, with support for Lua 5.4 introduced on 9 Sep 2019. The next release after this date is version 3.2.2.