Developer Ergonomics: Testing / Logging

[jmmk]

I was messing around with writing a module and there were a couple things I wish I had to improve the developer ergonomics:

• logs
• tests

It's possible there's a standard development workflow that I'm not aware of (e.g. attaching to a live process for debugging?) that gives more robust information, but logs are usually a simple go-to. Tests are also nice for verifying behavior programmatically.

Logs could also be potentially useful for debugging failures on the user side, though that's not my initial concern without knowing how the troubleshooting normally goes.

Is this something you would be interested in / accept a PR for? See below for some samples of logging and adding dependencies with cmake.

Edit: I guess there are two questions here:

• Which is the preferred method for managing dependencies: vendoring or using cmake?
• Is adding logging/testing support something that would be welcomed?

---

Re: Dependencies

I have never worked with C++ / CMake before, so I'm not familiar with how dependencies are usually managed in C+ projects. I noticed the cpp-lru-cache is committed in code and decided to figure out what the alternative would be.

Here's an example using cmake's ExternalProject_Add which brings in cpp-lru-cache using git: https://github.com/jmmk/slashdiablo-maphack/commit/90dcfb591d6e6555d5a7272103c3834fc67ea610

Technically this has no benefit over the current solution new since the code is already committed, but it's an example of a method that does not require all dependencies to be vendored.

---

Re: Logging

I'm not familiar with the C++ logging ecosystem, but https://github.com/gabime/spdlog seemed to be popular and well-supported, so I added it to the build (the compiled version, not the header-only version, though I don't fully understand the difference):

# spdlog
ExternalProject_Add(
        spdlog
        PREFIX ${THIRDPARTY_LIBS}
        GIT_REPOSITORY https://github.com/gabime/spdlog.git
        GIT_TAG 81b9149930f392d7797b54fe97a66ab3f2120671
        CMAKE_ARGS -DSPDLOG_BUILD_EXAMPLE=OFF -DCMAKE_INSTALL_PREFIX=${THIRDPARTY_LIBS}/spdlog
)
add_dependencies(BH spdlog)
target_link_libraries(BH PRIVATE ${THIRDPARTY_LIBS}/spdlog/lib/spdlog.lib)
target_include_directories(BH PRIVATE ${THIRDPARTY_LIBS}/spdlog/include)

It can be used like (more or less copied from the example in the docs, but tested in BH.cpp:

#include "spdlog/spdlog.h"
#include "spdlog/sinks/basic_file_sink.h"

// I'm not sure if this is the best place to put this, but it seemed a decent entry-point
void BH::Initialize() {
    // Set the default logger to file logger
    auto file_logger = spdlog::basic_logger_mt("basic_logger", "C:\\Program Files (x86)\\Diablo II\\BH.log");
    spdlog::set_default_logger(file_logger);
    // ...
}

// Inside my module
#include "spdlog/spdlog.h"
#include "spdlog/fmt/bin_to_hex.h"

void MyModule::someMethod() {
    spdlog::info("Some byte array in hex: {}", spdlog::to_hex(bytes));
}

---

Re: Tests

I didn't attempt to add tests yet, but this test framework looks pretty good: https://github.com/catchorg/Catch2

[jmmk]

Re: Vendoring

It's worth noting that you can sort of get the best of both worlds. You can have a subproject. included either as a git submodule or simply copied in (like cpp-lru-cache is today) and let CMake use that existing directory instead of doing the download itself

[youbetterdont]

Re the cpp-lru-cache inclusion:

I chose to use git subtree rather than submodule in this case. There are several reasons:

1. Not everyone is using CMake (as far as I know, I'm the only one), so the ExternalProject_Add isn't really an option.
2. Submodules sound good on the surface, but they are a pain in the ass to use in practice. It makes cloning the project a multiple step process that involves multiple google searches.

Although on the surface it looks like I've simply added a pile files into the repository, that's not quite how subtree works. I also imported the entire commit history of that project. This makes it possible to easily merge changes from that subtree. It's also possible to go the other way, but it's a bit more difficult.

One thing I probably should have done is squash the commit history. It's not terrible this way, but it might appear a little confusing at first.

Re logging:

I also looked into this a bit. It's a real shame there's no standard library mechanism to handle this. Not sure what the best solution is.

[jmmk]

@youbetterdont

It's been several years, but now that you mention it I remember using subtree in the past instead of submodules, probably for the reasons you mention

Are others using Visual Studio to build? I find the VS files harder to understand / modify (manually or through editor actions) than the CMake file, but I'll play with them

Regardless of the preferred dependency management strategy, I don't think it should be a barrier to adding useful libs as long as there is a repeatable process (e.g. git subtree add ..., add to VS file, add to CMake build)

[youbetterdont]

I believe most people are using VS to build. I agree that it doesn't play as nicely with git. I'm a big fan of CMake myself.

One nice thing about subtrees is that there isn't any extra work required to clone them.

[jmmk]

@youbetterdont Another thing I notice as I test out adding certain libraries: It seems the VS 2013 toolset/compiler (v120) is not fully compatible with C++11 (according to https://devblogs.microsoft.com/cppblog/c1114-core-language-features-in-vs-2013-and-the-nov-2013-ctp/. I'm not sure how to confirm).

Is there a reason not to upgrade to a later version?

---

I did see that one of the "features" listed on https://github.com/SergiusTheBest/plog is Doesn't require C++11, so it could be an option either way

I was able to use it by:

1. Adding the dependency

   git subtree add --prefix "Thirdparty/plog" --message "Add plog" "https://github.com/SergiusTheBest/plog.git" "f4c22b03d5d3aa753cca8e716636ac4eb29b0917" --squash

2. Adding the include paths

I added it to the BH project's properties under Configuration Properties > VC++ Directories > Include Directories. In this diff I did it for Release build only, but I see that cpp-lru-cache was added to many different configurations (not sure if they are all necessary)

diff --git a/BH/BH.vcxproj b/BH/BH.vcxproj
index a09b0be4..f39eb64c 100644
--- a/BH/BH.vcxproj
+++ b/BH/BH.vcxproj
@@ -54,7 +54,7 @@
   <PropertyGroup Label="UserMacros" />
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Release|Win32'">
     <TargetExt>.dll</TargetExt>
-    <IncludePath>$(INCLUDE);$(IncludePath);..\ThirdParty\cpp-lru-cache\include</IncludePath>
+    <IncludePath>$(INCLUDE);$(IncludePath);..\ThirdParty\cpp-lru-cache\include;..\ThirdParty\plog\include</IncludePath>
     <LibraryPath>..\ThirdParty\;$(LIBPATH);$(LibraryPath)</LibraryPath>
   </PropertyGroup>
   <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Packaging|Win32'">

3. Including the header and doing some logging

   diff --git a/BH/BH.cpp b/BH/BH.cpp
   index 6471661d..20b550f2 100644
   --- a/BH/BH.cpp
   +++ b/BH/BH.cpp
   @@ -10,6 +10,7 @@
   #include "MPQInit.h"
   #include "TableReader.h"
   #include "Task.h"
   +#include "plog/Log.h"
   
   string BH::path;
   HINSTANCE BH::instance;
   @@ -46,6 +47,8 @@ Patch* BH::oogDraw = new Patch(Call, D2WIN, { 0x18911, 0xEC61 }, (int)OOGDraw_In
   
   unsigned int index = 0;
   bool BH::Startup(HINSTANCE instance, VOID* reserved) {
   +       plog::init(plog::info, "c:\\Program Files (x86)\\Diablo II\\BH.log", 1000000, 5);
   +       PLOG_INFO << "starting BH";
   
       BH::instance = instance;
       if (reserved != NULL) {

[jmmk]

I did try out adding tests. Visual Studio has a built-in test framework, so I did something like the following:

• Create a new Test Project
• Add a dependency on the BH Project
• Write a test with an #include on an implementation file from BH

This "worked", but not completely:

• I got some weird errors I didn't know how to fix. It may be necessary to add other dependencies as described here: https://docs.microsoft.com/en-us/visualstudio/test/how-to-write-unit-tests-for-cpp-dlls?view=vs-2019#objectRef
• The code is very side-effectful, so is not always amenable to testing, at least not without more in-depth knowledge of how things are structured. It may require some reorganization to be worth it

---

Anyway, I'll close this for now. The above example should serve if logging is wanted/needed