Closed CihanSari closed 3 years ago
Hey! I was expecting this to come up actually. The gist of it is that I'm not familiar with cmake. The encoder itself doesn't use any magic that I expect to fail with other compilers/platforms. And the decoder is vanilla C++20 If I didn't mess up. Two thoughts:
So it's down to me not knowing cmake well enough to attempt this, especially with the external libs involved. I don't see fundamental problems however. If you or someone else wants to tackle this, that would be very welcome. In fact that would be pretty cool.
Hi Sebastian,
1- There were a few changes required, especially on includes and relatively new (c++20 / experimental) stl features but nothing major. I could get the code to compile and vast majority of tests to pass on Ubuntu.
2- All dependencies can be compiled using vcpkg. Instructions that I used for them are provided within the PR.
You can find my PR here: https://github.com/s9w/binary_bakery/pull/3
Hey wow, thanks for that - a lot! Two things:
1) What's with the changes from the ui8 literals to static_cast<uint8_t>(0)
etc? Style, compiler differences or something else?
2) I'm going to need to get some familiarity with cmake. Some things are pretty wild. I did manage to compile it with your instructions though. Out of curiosity, I tried loading it with Visual Studios cmake support. That did not work out, log:
1> [CMake] CMake Error at C:/inc/vcpkg-master/scripts/buildsystems/vcpkg.cmake:788 (_find_package):
1> [CMake] Could not find a package configuration file provided by "lz4" with any of
1> [CMake] the following names:
1> [CMake]
1> [CMake] lz4Config.cmake
1> [CMake] lz4-config.cmake
1> [CMake]
1> [CMake] Add the installation prefix of "lz4" to CMAKE_PREFIX_PATH or set "lz4_DIR"
1> [CMake] to a directory containing one of the above files. If "lz4" provides a
1> [CMake] separate development package or SDK, be sure it has been installed.
1> [CMake] Call Stack (most recent call first):
1> [CMake] binary_bakery_lib/CMakeLists.txt:23 (find_package)
1> [CMake] -- Configuring incomplete, errors occurred!
Not sure what to make of it or if that's relevant.
static_cast<uint8_t>(0)
is to support gnu and similar. 0ui8
is only valid in visual studio.
it sounds like lz4 is not installed on vcpkg on the triplet. Did you set vcpkg to the correct triplet on install step and the corresponding cmake instructions?
.\build\vcpkg\vcpkg install zstd lz4 tomlplusplus fmt stb doctest --triplet=x64-windows-static
<= here the triplet is x64-windows-static so it should install the libraries as such under the said triplet's folder (e.g. vcpkg\installed\x64-windows-static
). Then on cmake we set the toolchain file and target triplet: -DCMAKE_TOOLCHAIN_FILE=%vcpkg%/scripts/buildsystems/vcpkg.cmake -DVCPKG_TARGET_TRIPLET=x64-windows-static
If you already configured the triplet prior, then remove the CMakeCache.txt file from the build directory and re-configure with the given arguments.
I didn't read your message thoroughly.
It sounds like lz4 is not installed on vcpkg on the triplet visual studio is looking for. We need to set two parameters:
VCPKG_TARGET_TRIPLET=x64-windows-static
and CMAKE_TOOLCHAIN_FILE=%vcpkg%/scripts/buildsystems/vcpkg.cmake
I think by default visual studio might be looking for x64-windows or x86-windows and couldn't find the libraries there.
0ui8
is only valid in visual studio.
Damn I was not aware of that 😮
Yeah it's doing something weird. I'm on the road tomorrow but would like to have another look at this before I merge this. I hope that's ok for you. I'd like to have at least some basic familiarity before that. But I'm sure this will work out. Will be back on Thursday.
Of course mate, take your time :).
I would like to add cmake scripts to consume this library inside cmake and generate/compress resource files associated with the project and generate the header files. Add these into a cmake target (optionally as dynamic or static) and provide the content to the projects which depend on it to fully automate resource file generation.
I am not sure when I can look into this yet or how this plays into your plans of the library.
This is an exciting discussion. I too am interested in trying this out on Unix with CMake. If it would be useful to keep track of whether this project builds cross-platform, I've had some success using Github Actions to compile the code for multiple platforms with CMake. Ask me if you want some pointers on how that might be done (building and testing on CI is easier when your dependencies are trivial, I must admit).
I think testing on unix & windows should not be too hindered with the PR's build instructions (using vcpkg for the dependencies cross platform).
I would wait until my PR matures and gets merged or work on my fork to automate the tests.
Regarding this, we should add tests to cmake as tests (currently they are executable) and allow it to be run using CTest for an easier time.
Would you be interested in working on it @saxbophone ?
Regarding this, we should add tests to cmake as tests (currently they are executable) and allow it to be run using CTest for an easier time. Would you be interested in working on it @saxbophone ?
I'm not in a position to write any unit tests but I am happy to share the workflow I use for cross-platform unit-testing. To be honest, I was thinking more about basic testing that the project compiles cross-platform when I wrote my comments, but if you have unit tests that can be integrated, that would be great.
Unit tests are already in place on tests
target. The target is added as an executable rather than CTest. Executing it (https://github.com/CihanSari/binary_bakery/blob/main/building.md#configure--build) then checking the exit code should be more than sufficient.
@CihanSari With CMake, you can choose to add an executable target as a test case. I think it's the add_test()
function. This integrates with CTest so your current process of using an executable for the tests can be maintained, just enable_testing()
and some calls to add_test()
.
For the Github-Actions stuff, I have some samples here for using GHA to run unit tests on every PR and commit merged into master, and also for making release builds for every published release: https://github.com/saxbophone/CPP20-Cross-Platform-Template/tree/master/.github/workflows
In my sample, I build and test with g++ and clang++ on Linux and macOS and with MSVC on Windows. For the release builds, just one compiler per OS is used (g++ on Linux, clang++ on macOS and MSVC on Windows).
Hopefully a lot of the steps in those build files will be applicable to this project
I'm back and tried getting into this a bit more. I can see how you would have to specify the vcpkg directory at command line for cmake. But I was a bit confused about the target_triplet thing. That feels like something that should reside in the CMakeLists.txt
file, at least the static
part of the triplet. But I didn't even manage setting the whole triplet inside that file so apparently setting this via command line is the way to do things.
Anyway I'll merge things later as you clearly know your way around and it works for me.
Also I love the activity (hi @saxbophone) - that cmake and CI stuff is just something I tried to avoid in my life so you guys will be on your own there for a while. But go ham!
We could set triplet etc. on CMake but enforcing vcpkg on CMakeLists is very intrusive, as CMake will also locate locally installed libraries (sometimes with some command line magic).
Great idea! I didn't come across something very similar -- although Qt is cross platform, it is definitely extremely heavy.
I have a few questions 1- Your project seems to be based on visual studio, why not port it to CMake? 2- Is the project (binary file) windows dependent or do you only serve the executable for windows and ask others to compile it for unix/macos? 3- Would you be interested in extending this to make it integrated via CMake to allow resource files to be updated when they change on pre-built steps?