keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

chore(core): update core to C++17 #11340

Closed srl295 closed 5 days ago

srl295 commented 2 weeks ago

Fixes: #8800

@keymanapp-test-bot skip

keymanapp-test-bot[bot] commented 2 weeks ago

User Test Results

Test specification and instructions

User tests are not required

Test Artifacts

mcdurdin commented 1 week ago
  • std::codecvt is now deprecated. Switch to ~ICU4C~ utfcodec.hpp

Good. std::codecvt was always weird

mcdurdin commented 1 week ago

Cascade of build issues with C++17 changes sadly -- Windows, Developer both failing to build, alongside Core:Windows.

Might need to update VC++ project setting for C++ version for various projects.

srl295 commented 1 week ago

@mcdurdin doesn't repro locally 😢

Build server has:

   icu-minimal| C++ compiler for the host machine: cl (msvc 19.28.29915 "Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x86")
18:09:17   icu-minimal| C++ linker for the host machine: link link 14.28.29915.0

My system has: 19.29.30146

https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170 says that these numbers indicate VS2019 versions 16.8-16.9 (failing) versus 16.10-16.11 (passing)

EDIT Well actually.. repro's locally in x86 but not x64 🤦

srl295 commented 1 week ago

@mcdurdin summary: works in x64, fails in x86. The project settings are updated properly. From long and .. painful experience, sounds like a compiler bug. Trying VS2022..

mcdurdin commented 1 week ago

We could consider eliminating json from C++ code altogether -- as far as I can remember, it's only debug info anyway.

srl295 commented 1 week ago

We could consider eliminating json from C++ code altogether -- as far as I can remember, it's only debug info anyway.

it's used for writing debug info and for reading the test data for the ldml runner.

srl295 commented 1 week ago

I upgraded to VS2022 (only as a test) and it got worse - now x64 breaks too 🤦

if i upgrade to VS2024 it will make Mac stop working too (kidding)

seriously it seems like it's a known compiler issue on the json side.

srl295 commented 1 week ago

I upgraded to VS2022 (only as a test) and it got worse - now x64 breaks too 🤦

if i upgrade to VS2024 it will make Mac stop working too (kidding)

seriously it seems like it's a known compiler issue on the json side.

Perhaps a Path Forward is to keep the codecvt stuff out, but build for C++11 for now – maybe have one builder 'try' c++17 on linux to keep it working?

mcdurdin commented 1 week ago

seriously it seems like it's a known compiler issue on the json side.

Any ref to the known issue?

Is there a #define you can use to block certain behaviours of the json lib? I haven't dug in deep but I have seen some examples

srl295 commented 5 days ago

@mcdurdin wow, the issue was not in nlohmann::json (that one was taken care of with the update done earlier), it was in jsonpp.hpp which is the non-namespaced json class.

it had some fancy templating to handle 'any' kind of string, but it ended up trying to instantiate a std::basic_string<const json & ...> (i.e. inappropriate template expansion). So rewrite to just be two specializations, one for char16 and one for char.

keyman-server commented 4 days ago

Changes in this pull request will be available for download in Keyman version 18.0.35-alpha