You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "paleolimbot (via GitHub)" <gi...@apache.org> on 2024/08/05 14:25:57 UTC
[PR] refactor: Consolidate per-target actions in CMakeLists.txt [arrow-nanoarrow]
paleolimbot opened a new pull request, #573:
URL: https://github.com/apache/arrow-nanoarrow/pull/573
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 https://github.com/apache/arrow-nanoarrow/pull/555#discussion_r1702103767 where there was a DCHECK referencing a variable that didn't exist that made it through CI 😬
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: Consolidate per-target actions in CMakeLists.txt [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #573:
URL: https://github.com/apache/arrow-nanoarrow/pull/573#discussion_r1705564440
##########
CMakeLists.txt:
##########
@@ -145,41 +147,17 @@ target_include_directories(nanoarrow
PUBLIC $<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>
$<INSTALL_INTERFACE:include>)
-target_compile_definitions(nanoarrow PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
-
-# Ensure targets/headers can be installed
-install(TARGETS nanoarrow
- DESTINATION lib
- EXPORT nanoarrow-exports)
-
install(FILES ${NANOARROW_INSTALL_HEADERS} DESTINATION include/nanoarrow)
-if(CMAKE_BUILD_TYPE STREQUAL "Debug")
- if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wno-type-limits
- -Wmaybe-uninitialized
- -Wunused-result
- -Wconversion
- -Wno-sign-conversion)
- elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID STREQUAL
- "Clang")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wdocumentation
- -Wconversion
- -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
Review 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.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: Consolidate per-target actions in CMakeLists.txt [arrow-nanoarrow]
Posted by "bkietz (via GitHub)" <gi...@apache.org>.
bkietz commented on code in PR #573:
URL: https://github.com/apache/arrow-nanoarrow/pull/573#discussion_r1704571334
##########
CMakeLists.txt:
##########
@@ -145,41 +147,17 @@ target_include_directories(nanoarrow
PUBLIC $<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>
$<INSTALL_INTERFACE:include>)
-target_compile_definitions(nanoarrow PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
-
-# Ensure targets/headers can be installed
-install(TARGETS nanoarrow
- DESTINATION lib
- EXPORT nanoarrow-exports)
-
install(FILES ${NANOARROW_INSTALL_HEADERS} DESTINATION include/nanoarrow)
-if(CMAKE_BUILD_TYPE STREQUAL "Debug")
- if(CMAKE_C_COMPILER_ID STREQUAL "GNU")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wno-type-limits
- -Wmaybe-uninitialized
- -Wunused-result
- -Wconversion
- -Wno-sign-conversion)
- elseif(CMAKE_C_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_C_COMPILER_ID STREQUAL
- "Clang")
- target_compile_options(nanoarrow
- PRIVATE -Wall
- -Werror
- -Wextra
- -Wpedantic
- -Wdocumentation
- -Wconversion
- -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
Review Comment:
If `alignas` and `static_assert` are the only non-c99 features used by flatcc_generated.h, it should be possible to define
```c
#if __STDC_VERSION__ < 201112L
#define alignas(...)
#define static_assert(...)
#endif
```
( [macro reference](https://en.cppreference.com/w/c/preprocessor/replace#Predefined_macros) )
##########
CMakeLists.txt:
##########
@@ -346,9 +339,50 @@ if(NANOARROW_BUILD_TESTS OR NANOARROW_BUILD_INTEGRATION_TESTS)
PUBLIC $<BUILD_INTERFACE:${NANOARROW_BUILD_INCLUDE_DIR}>
$<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}/src>
$<INSTALL_INTERFACE:include>)
- target_link_libraries(nanoarrow_c_data_integration PRIVATE nanoarrow nanoarrow_testing)
+ target_link_libraries(nanoarrow_c_data_integration PRIVATE nanoarrow_testing)
endif()
+# Common configuration for all targets
+foreach(target
+ nanoarrow
+ nanoarrow_ipc
+ nanoarrow_device
+ nanoarrow_testing
+ nanoarrow_c_data_integration)
+ if(TARGET ${target})
+ target_compile_definitions(${target} PUBLIC "$<$<CONFIG:Debug>:NANOARROW_DEBUG>")
+
+ install(TARGETS ${target}
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+
+ if(CMAKE_BUILD_TYPE STREQUAL "Debug")
Review 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 ...>")
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: Consolidate per-target actions in CMakeLists.txt [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #573:
URL: https://github.com/apache/arrow-nanoarrow/pull/573
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org