ericw-tools icon indicating copy to clipboard operation
ericw-tools copied to clipboard

Switch to Jsoncpp, set up vcpkg in CI

Open ericwa opened this issue 2 years ago • 1 comments

  • replace nlohmann json (via git submodule) with jsoncpp (via vcpkg)
    • As a result, bspinfo.cc build time reduced from 2.6s to 1.9s
    • I only really did this switch out of concern for build times.
    • Disadvantages: jsoncpp development is stalled. It's a bit flaky in places, e.g. I had to add this cast to get this line to compile on macOS: model["numbrushes"] = static_cast<Json::UInt64>(src_model.brushes.size());
    • I think moving away from header-only is the right move, but I'm not sure if changing worth it?
  • Set up vcpkg in our GH Actions config, also install lua (our binaries previously lacked lua)
    • Disadvantage, this adds another minute or so to CI
    • We probably do want our maputil release binaries to be fully functional, so if it needs lua, it needs lua..
  • Drop appveyor
    • It was considerably slower (12 min, vs 3-6min for GH Actions) and we don't need it anymore (all release binaries are built on GitHub actions)
    • Setting up vcpkg in GH Actions was enough work that I don't feel like dealing with another CI system - this will keep being a maintenance burden every time we touch CI build settings
    • As a negative, if we drop appveyor, getting access to our dev builds will require login to GitHub now
    • We could keep appveyor as well..
  • Fix an ODR violation reported by GCC
    • It was specifically complaining about struct texdef_valve_t and struct map_entity_t
    • I'm not sure why this only came up now, maybe jsoncpp is leaking some warning flags, but it was a valid problem

All these (except the ODR fix) are kind of subjective, what do you think @Paril ?

Alternatively we could just keep the ODR fix, vcpkg + lua, but keep nlohmann?

ericwa avatar Mar 18 '24 01:03 ericwa

The ODR change is probably because of a thing I did recently including map.hh in the old tools. I still need to move map.cc over to the new map types.

Paril avatar Mar 18 '24 17:03 Paril