[vpdq] Modernize CMake Usage #1600
Conversation
| run: | | ||
| rm -rf cpp/build | ||
| mkdir cpp/build | ||
| cd cpp/build | ||
| cmake .. -DCMAKE_CXX_FLAGS="-O2 -fPIC -Wall -Wextra -Werror -Wno-unused-function -Wno-deprecated-declarations -fsanitize=thread,undefined" -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,leak,undefined" |
There was a problem hiding this comment.
The linker flags in this were wrong. It should be thread not address Not entirely sure how it was working.
There was a problem hiding this comment.
I think I learned a little but of cmake to review the original authors' work here, but at this point my head is empty.
| secondsPerHash: int, | ||
| downsampleFrameDimension: int, | ||
| ): | ||
| Path.mkdir(OUTPUT, exist_ok=True) |
There was a problem hiding this comment.
Need this directory or the hashes won't be output. This avoids having to make it manually.
| set(CMAKE_SHARED_LINKER_FLAGS_TSAN | ||
| "${CMAKE_SHARED_LINKER_FLAGS_DEBUG} -fsanitize=thread,undefined" CACHE STRING | ||
| "Linker lags to be used to create shared libraries for Tsan build type." FORCE) | ||
| endif() |
There was a problem hiding this comment.
Not going to pretend that I fully understand this block, but it makes sense at a high level and it does work.
There was a problem hiding this comment.
This one is out of my area of expertise. The best guidance I have for you is that if still produces the right output on regtest, and if asan and tsan still work, we are good.
| set(CMAKE_SHARED_LINKER_FLAGS_TSAN | ||
| "${CMAKE_SHARED_LINKER_FLAGS_DEBUG} -fsanitize=thread,undefined" CACHE STRING | ||
| "Linker lags to be used to create shared libraries for Tsan build type." FORCE) | ||
| endif() |
There was a problem hiding this comment.
This one is out of my area of expertise. The best guidance I have for you is that if still produces the right output on regtest, and if asan and tsan still work, we are good.
| run: | | ||
| rm -rf cpp/build | ||
| mkdir cpp/build | ||
| cd cpp/build | ||
| cmake .. -DCMAKE_CXX_FLAGS="-O2 -fPIC -Wall -Wextra -Werror -Wno-unused-function -Wno-deprecated-declarations -fsanitize=thread,undefined" -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,leak,undefined" |
There was a problem hiding this comment.
I think I learned a little but of cmake to review the original authors' work here, but at this point my head is empty.
| @@ -20,12 +20,13 @@ | |||
| CPP_BUILD_DIR = CPP_DIR / "build" | |||
There was a problem hiding this comment.
Haven't not looked at this for a while, it seems setup.py have mostly fallen out of vogue vs pyproject.toml, but that can be a problem for later, ref: https://packaging.python.org/en/latest/overview/
Summary
Changes are splitting CMake files to their respective directories. This is the preferred way to structure modern CMake projects.
This allows easier modifications to some parts of the project without breaking another.
Update examples and CI to use modern CLI commands.
cmake -S . -B buildreplacesmkdir build && cmake ...cmake --build build -jreplacesmakeMove CLI programs to new directory, apps
Add CMake build types for sanitizers
Link Python binding with pdq since it is now a separately built library
Add another stage in the CPP CI for a no sanitizer regtest
Fix benchmark.py
I've been learning proper CMake at work and wanted to apply it to a more complex project. This should significantly simplify the work needed for future changes to the project, such as adding unit tests or bindings.
And hopefully it can serve as a reference to others since there is a lack of proper CMake projects out there, especially with Cython.
Test Plan
Tested with container, CI and Arch Linux the Python binding and cpp through regtest.
Tested the Python package output sdist build to ensure it works when downloaded from PyPi.
I building on an M2 Pro, but there are some other unrelated issues in the implementation that causes compilation to fail. I will address these issues in a separate PR. After fixing those problems this CMake setup works on macos + Clang.