manage dependency by vcpkg & test with GitHub Actions

[liuzicheng1987]

I like the fact that you are adding in vcpkg support and I certainly like Github Actions, but I think vcpkg should be optional.

I want to be able to go:

mkdir build
cd build 
cmake ..
make -j4

Because that works for most C++ libraries and I almost expect it to work when I encounter a new library.

I also think that JSON support should be out-of-the-box and activated by default. It is the most important of all serialization formats and all of the examples are consistently written in JSON. So please keep yyjson.h and yyjson.c in the repository.

I think it is very important to make as easy as possible to give the library a try just to see that it does indeed work as promised. And when you have to deal with package management first and then install a couple of dependencies, it just adds unnecessary hurdles.

Other than that, I absolutely like where this is going. People have asked me about vcpkg and running the tests on Github Actions is certainly useful. So thank you for the initiative.

[liuzicheng1987]

Please don't get me wrong, I really like this...but the fact is that most people give a new library about 5-10 minutes. If they can't get it to run within that time, they just give up. So you need to give people a sense of achievement (you know, 成就感, the Chinese word is closer to what I mean) within a very short period of time. So I think you should be able to get at least JSON up and running without having to deal with dependency managers and other stuff.

[liuzicheng1987]

Another very minor point: I saw that you renamed the flexbuffers folder.

However, Rust's serde, which is one of the reference libraries we used for this project, refers to the format as flexbuffers:

https://docs.rs/serde/latest/serde/

I believe this to be more accurate, because we are indeed using the flexbuffers subset of flatbuffers.

[zerolfx]

@liuzicheng1987 I agree with you. I didn't notice the relationship between FlatBuffers and FlexBuffers, so the rename change has been reverted. Making JSON out-of-the-box is beneficial for users.

After numerous attempts, I gave up setting up Windows CI due to an unexpected compiler "internal error".

I removed -O2 from CMake since users can control the optimization level by using -DCMAKE_BUILD_TYPE=Release. Currently, if a test fails, the process won't exit abnormally, and the CI will still succeed. Perhaps we can consider switching to Google Test in the future.

[zerolfx]

The PR is ready for review. If you want to make any changes, you can also commit directly to my branch.

Additionally, I haven't cleaned up my commits, so please use squash and merge when you decide to merge.

[ChemiCalChems]

As @zerolfx says, it would be ideal to switch to Google Test or a similar framework, at the very least have our tests return true or false so we can automate testing for GitHub Actions or our own local workflows (I like to interactive rebase through all my commits and make sure tests pass on each of my commits I push so no commit that is made can individually break anything, for example, and it's currently way more difficult to do without greping the output of something).

In fact, as this PR stands, it runs the tests but doesn't check for output. This has to be remediated.

[liuzicheng1987]

Yeah, no, we're not going to grep the output. I think switching to Google Test or something similar is the most sensible thing to do. And it shouldn't be hard either. Most of the tests use the same function (test_read_and_write) for testing, so we just have to make one change.

[ChemiCalChems]

Then let's go for it! Improvements everywhere, little by little!

[liuzicheng1987]

As far as the vcpkg is concerned...I think there are some minor changes to the documentation, I would suggest.

1. In the README.md, it still says set(REFLECTCPP_FLATBUFFERS ON) # Optional, even though that should be REFLECTCPP_FLEXBUFFERS.
2. I had to install Ninja using sudo apt-get install ninja-build to build vcpkg. I think this should be documented as well.
3. I think this should be Option 4: Using vcpkg. In your current version, you have deleted Option 3 (even though it is still supported.)
4. I think the language "features other than the out-of-the-box JSON support" is a bit misleading. Instead, it should be "If you want serialization formats other than JSON, you can either install them manually or use vcpkg."

[liuzicheng1987]

Next thing...when I am developing locally, I usually just build the JSON tests. I don't have to compile the flexbuffers tests all the time. The different formats are pretty orthogonal and it would just be a waste of time.

I think it is great that we have Github actions that automate the testing pipeline, but if I want to compile only the JSON tests or only the XML tests (I'm working on that at the moment), how would I do that?

What I usually do is what used to be described in the README. But that doesn't work anymore. How would I do that in the future? If it is possible, could you document it as well? If not, could you re-enable this?

[liuzicheng1987]

Please don't be discouraged by my nitpicking here. I think this is a great initiative and I will certainly merge when these things are resolved.

[zerolfx]

@liuzicheng1987 I've updated the readme. Thanks for patiently highlighting all the issues.

You can still compile the json tests without vcpkg. Refer to the updated Compiling the tests section for details.

[liuzicheng1987]

Cool, I will try it out tonight!

[liuzicheng1987]

OK, I have tested this.

There were three minor things that came up:

1. Before you configure a new build, you have to delete the build directory using rm -rf build.
2. Whenever vcpkg was involved, I had to explicitly set the compilers using -DCMAKE_C_COMPILER=gcc -DCMAKE_CXX_COMPILER=g++ (or clang and clang++ respectively).
3. Whenever vcpkg was involved, I had to set the architecture using export VCPKG_FORCE_SYSTEM_BINARIES=arm. That is because my computer is one of those new Apple Silicon machines with a Ubuntu VM on top.

These three things should also be mentioned in the documentation. But these things are relatively small. I can even do that myself, that's not a big issue.

The one thing that is very weird is that on Windows/MSVC the tests compile in Debug Mode, not Release Mode, even though we explicitly pass -DCMAKE_BUILD_TYPE=Release. I can't really figure out why. I want to figure this out before I merge, but it is a bit late in the eveining. I will try to find the problem tomorrow.

Other than these things, I think this is absolutely great. I am sure we can resolve these very minor problems. All of the tests, including the tests for flexbuffers compile and run through on all three major compilers.

Thank you again for the initiative.

[zerolfx]

There are so many subtle issues with different architectures or different compilers. It's always good to consider more cases.

Unresolved or help wanted:

1. I've managed to reproduce the issue. It appears that removing the build directory (or build/CMakeCache.txt) is necessary if find_package changes. However, I'm unsure how and where to document this.
2. It should compile with the default system compiler (/usr/bin/cc and /usr/bin/cxx, presumably). On my machine, it compiles successfully with GCC without setting any environment variables. To switch from GCC to Clang, I need to set the CC and CXX environment variables, as I did in CI (setting CMAKE_C_COMPILER and CMAKE_CXX_COMPILER should also work).
3. I'm uncertain about the consequences of not setting the environment variable. (It seems export VCPKG_FORCE_SYSTEM_BINARIES=arm is equivalent to export VCPKG_FORCE_SYSTEM_BINARIES=1.)

Documented:

4. To build a release on Windows, it appears the best method is to add -config Release during the build. Refer to https://stackoverflow.com/questions/19024259/how-to-change-the-build-type-to-release-mode-in-cmake

@liuzicheng1987, if you can assist with documenting these aspects, you may commit directly to my branch before merging.

[liuzicheng1987]

@zerolfx , I can confirm that your proposed solution fixes the issue. I will merge the PR and then find ways to document the rest.

Thank you so much for your contribution! This is such a great thing to have.