Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. #6279

SiarheiFedartsou · 2022-07-09T19:31:25Z

Issue

This PR migrates OSRM to C++17. Also I had to update sol2 to 3.3.0, because our ancient 2.x version couldn't be built under C++17.

We could upgrade to even C++20, but it would require rework of CI.

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

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-07-09T20:08:25Z

src/extractor/scripting_environment_lua.cpp

@@ -1171,14 +1163,14 @@ void LuaScriptingContext::ProcessNode(const osmium::Node &node,
    {
    case 4:
    case 3:
-        node_function(profile_table, node, result, relations);


Otherwise it complained that Node has no copy constructor and it cannot copy it.

SiarheiFedartsou · 2022-07-13T18:35:04Z

src/extractor/scripting_environment_lua.cpp

+namespace
+{
+
+// string list can be defined either as a Set(see profiles/lua/set.lua) or as a Sequence (see


It turned out we had a bug with Set values: second in case of Set is always true. See:

osrm-backend/profiles/lib/set.lua

Line 17 in 06b1b98

set[v] = true

Nice catch!
I guess that means nobody has noticed the suffixes appearing in walking guidance (or nobody is using it).

SiarheiFedartsou · 2022-07-13T18:36:47Z

features/foot/restrictions.feature

@@ -29,7 +29,7 @@ Feature: Foot - Turn restrictions
        When I route I should get
            | from | to | route    |
            | s    | w  | sj,wj,wj |
-            | s    | n  | sj,nj,nj |
+            | s    | n  | sj,nj    |


This test uses foot.lua profile, which in its turn uses suffix_list defined as Set. That's why fix of the bug with Set values changed behaviour here.

osrm-backend/profiles/foot.lua

Line 67 in 06b1b98

suffix_list = Set {

Hmm, it's not immediately obvious why this would change due to the bug fix, unless it's something very subtle in the code.

SiarheiFedartsou · 2022-07-13T18:38:10Z

CHANGELOG.md

@@ -3,6 +3,7 @@
    - API:
      - FIXED: Fix inefficient osrm-routed connection handling [#6113](https://github.com/Project-OSRM/osrm-backend/pull/6113)
    - Build:
+      - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. [#6279](https://github.com/Project-OSRM/osrm-backend/pull/6279)


Should I maybe extract bug fix into separate PR?

I think it would be good to do so. It's not clear to me how https://github.com/Project-OSRM/osrm-backend/pull/6279/files#r920390451 is related to the bug, so would be good to separate it from the other changes to understand.

@mjjbell please see #6285

SiarheiFedartsou · 2022-07-13T19:00:44Z

@mjjbell @danpat
Please take a look when you have a chance. Please don't be afraid of the number of changes. Vast majority of them is in thirdparty/sol2:

mjjbell

👍

Zverik · 2022-08-09T13:56:38Z

Looks like this PR invalidated the bundled Dockerfile. #6302

v5.27.0 - Changes from 5.26.0 - API: - ADDED: Add Flatbuffers support to NodeJS bindings. [Project-OSRM#6338](Project-OSRM#6338) - CHANGED: Add `data_version` field to responses of all services. [Project-OSRM#5387](Project-OSRM#5387) - FIXED: Use Boost.Beast to parse HTTP request. [Project-OSRM#6294](Project-OSRM#6294) - FIXED: Fix inefficient osrm-routed connection handling [Project-OSRM#6113](https://gihub.com/Project-OSRM/osrm-backend/pull/6113) - FIXED: Fix HTTP compression precedence [Project-OSRM#6113](Project-OSRM#6113) - NodeJS: - FIXED: Support `skip_waypoints` in Node bindings [Project-OSRM#6060](Project-OSRM#6060) - Misc: - ADDED: conanbuildinfo.json for easy reading of dependencies [Project-OSRM#6388](Project-OSRM#6388) - CHANGED: Improve performance of JSON rendering. Fix undefined behaviour in JSON numbers formatting. [Project-OSRM#6380](Project-OSRM#6380) - ADDED: Add timestamps for logs. [Project-OSRM#6375](Project-OSRM#6375) - CHANGED: Improve performance of map matching via getPathDistance optimization. [Project-OSRM#6378](Project-OSRM#6378) - CHANGED: Optimize RestrictionParser performance. [Project-OSRM#6344](Project-OSRM#6344) - ADDED: Support floats for speed value in traffic updates CSV. [Project-OSRM#6327](Project-OSRM#6327) - CHANGED: Use Lua 5.4 in Docker image. [Project-OSRM#6346](Project-OSRM#6346) - CHANGED: Remove redundant nullptr check. [Project-OSRM#6326](Project-OSRM#6326) - CHANGED: missing files list is included in exception message. [Project-OSRM#5360](Project-OSRM#5360) - CHANGED: Do not use deprecated Callback::Call overload in Node bindings. [Project-OSRM#6318](Project-OSRM#6318) - FIXED: Fix distance calculation consistency. [Project-OSRM#6315](Project-OSRM#6315) - FIXED: Fix performance issue after migration to sol2 3.3.0. [Project-OSRM#6304](Project-OSRM#6304) - CHANGED: Pass osm_node_ids by reference in osrm::updater::Updater class. [Project-OSRM#6298](Project-OSRM#6298) - FIXED: Fix bug with reading Set values from Lua scripts. [Project-OSRM#6285](Project-OSRM#6285) - FIXED: Bug in bicycle profile that caused exceptions if there is a highway=bicycle in the data. [Project-OSRM#6296](Project-OSRM#6296) - FIXED: Internal refactoring of identifier types used in data facade [Project-OSRM#6044](Project-OSRM#6044) - CHANGED: Update docs to reflect recent build and dependency changes [Project-OSRM#6383](Project-OSRM#6383) - Build: - REMOVED: Get rid of Mason. [Project-OSRM#6387](Project-OSRM#6387) - CHANGED: Use clang-format from CI base image. [Project-OSRM#6391](Project-OSRM#6391) - ADDED: Build Node bindings on Windows. [Project-OSRM#6334](Project-OSRM#6334) - ADDED: Configure cross-compilation for Apple Silicon. [Project-OSRM#6360](Project-OSRM#6360) - CHANGED: Use apt-get to install Clang on CI. [Project-OSRM#6345](Project-OSRM#6345) - CHANGED: Fix TBB in case of Conan + NodeJS build. [Project-OSRM#6333](Project-OSRM#6333) - CHANGED: Migrate to modern TBB version. [Project-OSRM#6300](Project-OSRM#6300) - CHANGED: Enable performance-move-const-arg clang-tidy check. [Project-OSRM#6319](Project-OSRM#6319) - CHANGED: Use the latest node on CI. [Project-OSRM#6317](Project-OSRM#6317) - CHANGED: Migrate Windows CI to GitHub Actions. [Project-OSRM#6312](Project-OSRM#6312) - ADDED: Add smoke test for Docker image. [Project-OSRM#6313](Project-OSRM#6313) - CHANGED: Update libosmium to version 2.18.0. [Project-OSRM#6303](Project-OSRM#6303) - CHANGED: Remove EXACT from find_package if using Conan. [Project-OSRM#6299](Project-OSRM#6299) - CHANGED: Configure Undefined Behaviour Sanitizer. [Project-OSRM#6290](Project-OSRM#6290) - CHANGED: Use Conan instead of Mason to install code dependencies. [Project-OSRM#6284](Project-OSRM#6284) - CHANGED: Migrate to C++17. Update sol2 to 3.3.0. [Project-OSRM#6279](Project-OSRM#6279) - CHANGED: Update macOS CI image to macos-11. [Project-OSRM#6286](Project-OSRM#6286) - CHANGED: Enable even more clang-tidy checks. [Project-OSRM#6273](Project-OSRM#6273) - CHANGED: Configure CMake to not build flatbuffers tests and samples. [Project-OSRM#6274](Project-OSRM#6274) - CHANGED: Enable more clang-tidy checks. [Project-OSRM#6270](Project-OSRM#6270) - CHANGED: Configure clang-tidy job on CI. [Project-OSRM#6261](Project-OSRM#6261) - CHANGED: Use Github Actions for building container images [Project-OSRM#6138](Project-OSRM#6138) - CHANGED: Upgrade Boost dependency to 1.70 [Project-OSRM#6113](Project-OSRM#6113) - CHANGED: Upgrade Ubuntu CI builds to 20.04 [Project-OSRM#6119](Project-OSRM#6119) - CHANGED: Make building osrm-routed optional [Project-OSRM#6144](Project-OSRM#6144) - FIXED: Run all unit tests in CI [Project-OSRM#5248](Project-OSRM#5248) - FIXED: Fix installation of Mason CMake and 32 bit CI build [Project-OSRM#6170](Project-OSRM#6170) - FIXED: Fixed Node docs generation check in CI. [Project-OSRM#6058](Project-OSRM#6058) - CHANGED: Docker build, enabled arm64 build layer [Project-OSRM#6172](Project-OSRM#6172) - CHANGED: Docker build, enabled apt-get update/install caching in separate layer for build phase [Project-OSRM#6175](Project-OSRM#6175) - FIXED: Bump CI complete meta job to ubuntu-20.04 [Project-OSRM#6323](Project-OSRM#6323) - CHANGED: Node packages are now scoped by @Project-OSRM [Project-OSRM#6386](Project-OSRM#6386) - Routing: - CHANGED: Lazily generate optional route path data [Project-OSRM#6045](Project-OSRM#6045) - FIXED: Completed support for no_entry and no_exit turn restrictions. [Project-OSRM#5988](Project-OSRM#5988) - ADDED: Add support for non-round-trips with a single fixed endpoint. [Project-OSRM#6050](Project-OSRM#6050) - FIXED: Improvements to maneuver override processing [Project-OSRM#6125](Project-OSRM#6125) - ADDED: Support snapping to multiple ways at an input location. [Project-OSRM#5953](Project-OSRM#5953) - FIXED: Fix snapping target locations to ways used in turn restrictions. [Project-OSRM#6339](Project-OSRM#6339) - ADDED: Support OSM traffic signal directions. [Project-OSRM#6153](Project-OSRM#6153) - FIXED: Ensure u-turn exists in intersection view. [Project-OSRM#6376](Project-OSRM#6376) - FIXED: Gracefully handle no-turn intersections in guidance processing. [Project-OSRM#6382](Project-OSRM#6382) - Profile: - CHANGED: Bicycle surface speeds [Project-OSRM#6212](Project-OSRM#6212) - Tools: - CHANGED: Do not generate intermediate .osrm file in osrm-extract. [Project-OSRM#6354](Project-OSRM#6354)

* Migrate to C++17. Update sol2 to 3.3.0.

Migrate to C++17. Update sol2 to 3.3.0.

59b531f

SiarheiFedartsou force-pushed the sf-cpp-17 branch from 7be8e4d to 59b531f Compare July 9, 2022 19:32

SiarheiFedartsou added 3 commits July 9, 2022 21:33

Merge branch 'master' into sf-cpp-17

5cd1f85

Migrate to C++17. Update sol2 to 3.3.0.

8dc0214

Migrate to C++17. Update sol2 to 3.3.0.

ef66c0a

SiarheiFedartsou commented Jul 9, 2022

View reviewed changes

SiarheiFedartsou marked this pull request as ready for review July 9, 2022 20:11

SiarheiFedartsou added 6 commits July 13, 2022 20:27

Migrate to C++17. Update sol2 to 3.3.0.

b358000

Migrate to C++17. Update sol2 to 3.3.0.

f9ccc0c

Migrate to C++17. Update sol2 to 3.3.0.

55a0b74

Migrate to C++17. Update sol2 to 3.3.0.

ddd8fe6

Migrate to C++17. Update sol2 to 3.3.0.

e53a5cf

Migrate to C++17. Update sol2 to 3.3.0.

3ccd349

SiarheiFedartsou commented Jul 13, 2022

View reviewed changes

SiarheiFedartsou changed the title ~~Migrate to C++17. Update sol2 to 3.3.0.~~ Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. Jul 13, 2022

SiarheiFedartsou mentioned this pull request Jul 23, 2022

Fix bug with reading Set values from Lua scripts. #6285

Merged

6 tasks

Merge branch 'master' into sf-cpp-17

ef5c461

mjjbell approved these changes Jul 30, 2022

View reviewed changes

mjjbell merged commit 4e8ee28 into Project-OSRM:master Jul 30, 2022

Zverik mentioned this pull request Aug 9, 2022

OSRM build fails on GCC 10.2.1 #6302

Closed

SiarheiFedartsou mentioned this pull request Aug 9, 2022

Fix performance issues after migration to sol2 3.3.0 #6304

Merged

6 tasks

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

Migrate to C++17. Update sol2 to 3.3.0. (Project-OSRM#6279)

1b9cece

* Migrate to C++17. Update sol2 to 3.3.0.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. #6279

Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. #6279

SiarheiFedartsou commented Jul 9, 2022 •

edited

Loading

SiarheiFedartsou Jul 9, 2022

SiarheiFedartsou Jul 13, 2022 •

edited

Loading

mjjbell Jul 20, 2022

SiarheiFedartsou Jul 13, 2022 •

edited

Loading

mjjbell Jul 20, 2022

SiarheiFedartsou Jul 13, 2022

mjjbell Jul 20, 2022

SiarheiFedartsou Jul 23, 2022

SiarheiFedartsou commented Jul 13, 2022

mjjbell left a comment

Zverik commented Aug 9, 2022

Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. #6279

Migrate to C++17. Update sol2 to 3.3.0. Fix bug with reading Set values from Lua scripts. #6279

Conversation

SiarheiFedartsou commented Jul 9, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou Jul 9, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

mjjbell Jul 20, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

mjjbell Jul 20, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 13, 2022

Choose a reason for hiding this comment

mjjbell Jul 20, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Jul 23, 2022

Choose a reason for hiding this comment

SiarheiFedartsou commented Jul 13, 2022

mjjbell left a comment

Choose a reason for hiding this comment

Zverik commented Aug 9, 2022

SiarheiFedartsou commented Jul 9, 2022 •

edited

Loading

SiarheiFedartsou Jul 13, 2022 •

edited

Loading

SiarheiFedartsou Jul 13, 2022 •

edited

Loading