refactor: Consolidate per-target actions in CMakeLists.txt #573
Conversation
paleolimbot
force-pushed
the
cmake-consolidate-per-target-items
branch
from
e660a4c
to
7210c42
Compare
| -Wno-sign-conversion) | ||
| if(NANOARROW_IPC) | ||
| # flatcc requires C11 for alignas() and static_assert() in flatcc_generated.h | ||
| # It may be possible to use C99 mode to build the runtime and/or generated header |
There was a problem hiding this comment.
If alignas and static_assert are the only non-c99 features used by flatcc_generated.h, it should be possible to define
#if __STDC_VERSION__ < 201112L
#define alignas(...)
#define static_assert(...)
#endif( macro reference )
There was a problem hiding this comment.
I am going to punt on C99 for now but I think flatcc provides some of these workarounds if this is something we really need. Off the top of my head, I think C11 might not be available on MSVC (although I checked my local Windows environment and it seems to build without problem). From flatcc --help:
The generated source assumes C11 functionality for alignment, compile
time assertions and inline functions but an optional set of portability
headers can be included to work with most any compiler. The portability
layer is not throughly tested so a platform specific test is required
before production use. Upstream patches are welcome.
CMakeLists.txt
Outdated
| DESTINATION lib | ||
| EXPORT nanoarrow-exports) | ||
|
|
||
| if(CMAKE_BUILD_TYPE STREQUAL "Debug") |
There was a problem hiding this comment.
It's possibly not a concern for nanoarrow, but CMake doesn't define this variable in all generators. Specifically it's defined for single configuration generators like Ninja or makefiles, but on multi configuration generators like Visual Studio or XCode we have instead https://cmake.org/cmake/help/latest/variable/CMAKE_CONFIGURATION_TYPES.html
I think the most correct way to do this is by wrapping these options in a generator expression:
target_compile_options(${target} PRIVATE "$<$<CONFIG:Debug>:-Wall -Werror -Wextra ...>")
In previous PRs we consolidated the extensions into the main CMakeLists.txt; however, there were some things happening for certain targets (like installing them or setting NANOARROW_DEBUG) but not others.
Noticed in #555 (comment) where there was a DCHECK referencing a variable that didn't exist that made it through CI 😬