Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to Jsoncpp, set up vcpkg in CI #414

Open
wants to merge 10 commits into
base: brushbsp
Choose a base branch
from
Open

Switch to Jsoncpp, set up vcpkg in CI #414

wants to merge 10 commits into from

Conversation

ericwa
Copy link
Owner

@ericwa ericwa commented Mar 18, 2024

  • 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?

@Paril
Copy link
Collaborator

Paril commented Mar 18, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants