From c38c6b6a15de0eb2f45192c18ceb7620e9172810 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 23 Jun 2023 15:19:23 -0400 Subject: [PATCH 1/4] build(c): force C++11 for drivers for R's sake Fixes #815. Fixes #842. --- c/cmake_modules/AdbcDefines.cmake | 42 ++++++++++++++++++++++------- c/cmake_modules/BuildUtils.cmake | 13 +++++++-- c/cmake_modules/DefineOptions.cmake | 3 +++ c/driver/common/CMakeLists.txt | 1 + c/driver/postgresql/connection.cc | 6 ++--- c/validation/CMakeLists.txt | 1 + 6 files changed, 52 insertions(+), 14 deletions(-) diff --git a/c/cmake_modules/AdbcDefines.cmake b/c/cmake_modules/AdbcDefines.cmake index 25dfcbac8e..34166788b2 100644 --- a/c/cmake_modules/AdbcDefines.cmake +++ b/c/cmake_modules/AdbcDefines.cmake @@ -60,16 +60,40 @@ if(CXX_LINKER_SUPPORTS_VERSION_SCRIPT) endif() # Set common build options -macro(adbc_configure_target TARGET) - if(CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - target_compile_options(${TARGET} - PRIVATE -Wall - -Werror - -Wextra - -Wpedantic - -Wno-unused-parameter - -Wunused-result) +if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "") + if("${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE") + set(ADBC_BUILD_WARNING_LEVEL "PRODUCTION") + else() + set(ADBC_BUILD_WARNING_LEVEL "CHECKIN") + endif() +endif() + +set(ADBC_C_CXX_FLAGS) +if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN") + if(MSVC) + set(ADBC_C_CXX_FLAGS /Wall /Werror) + elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + set(ADBC_C_CXX_FLAGS -Wall -Werror) + else() + message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") endif() +elseif("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "PRODUCTION") + if(MSVC) + set(CMAKE_C_FLAGS /Wall) + set(ADBC_C_CXX_FLAGS /Wall) + elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + set(ADBC_C_CXX_FLAGS -Wall) + else() + message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") + endif() +endif() + +macro(adbc_configure_target TARGET) + target_compile_options(${TARGET} PRIVATE ${ADBC_C_CXX_FLAGS}) endmacro() # Common testing setup diff --git a/c/cmake_modules/BuildUtils.cmake b/c/cmake_modules/BuildUtils.cmake index df2590a905..6d6438d336 100644 --- a/c/cmake_modules/BuildUtils.cmake +++ b/c/cmake_modules/BuildUtils.cmake @@ -194,6 +194,10 @@ function(ADD_ARROW_LIB LIB_NAME) target_link_libraries(${LIB_NAME}_objlib PRIVATE ${ARG_SHARED_LINK_LIBS} ${ARG_SHARED_PRIVATE_LINK_LIBS} ${ARG_STATIC_LINK_LIBS}) + adbc_configure_target(${LIB_NAME}_objlib) + # https://github.com/apache/arrow-adbc/issues/81 + target_compile_features(${LIB_NAME}_objlib PRIVATE cxx_std_11) + set_property(TARGET ${LIB_NAME}_objlib PROPERTY CXX_STANDARD 11) else() # Prepare arguments for separate compilation of static and shared libs below # TODO: add PCH directives @@ -255,6 +259,9 @@ function(ADD_ARROW_LIB LIB_NAME) VERSION "${ADBC_FULL_SO_VERSION}" SOVERSION "${ADBC_SO_VERSION}") + # https://github.com/apache/arrow-adbc/issues/81 + target_compile_features(${LIB_NAME}_shared PRIVATE cxx_std_11) + target_link_libraries(${LIB_NAME}_shared LINK_PUBLIC "$" @@ -342,6 +349,9 @@ function(ADD_ARROW_LIB LIB_NAME) PROPERTIES LIBRARY_OUTPUT_DIRECTORY "${OUTPUT_PATH}" OUTPUT_NAME ${LIB_NAME_STATIC}) + # https://github.com/apache/arrow-adbc/issues/81 + target_compile_features(${LIB_NAME}_static PRIVATE cxx_std_11) + if(ARG_STATIC_INSTALL_INTERFACE_LIBS) target_link_libraries(${LIB_NAME}_static LINK_PUBLIC "$") @@ -584,6 +594,7 @@ function(ADD_TEST_CASE REL_TEST_NAME) set(TEST_PATH "${EXECUTABLE_OUTPUT_PATH}/${TEST_NAME}") add_executable(${TEST_NAME} ${SOURCES}) + adbc_configure_target(${TEST_NAME}) # With OSX and conda, we need to set the correct RPATH so that dependencies # are found. The installed libraries with conda have an RPATH that matches @@ -637,8 +648,6 @@ function(ADD_TEST_CASE REL_TEST_NAME) add_test(${TEST_NAME} ${TEST_PATH} ${ARG_TEST_ARGUMENTS}) endif() - adbc_configure_target(${TEST_NAME}) - # Add test as dependency of relevant targets add_dependencies(all-tests ${TEST_NAME}) foreach(TARGET ${ARG_LABELS}) diff --git a/c/cmake_modules/DefineOptions.cmake b/c/cmake_modules/DefineOptions.cmake index 42b8f4f51f..b6dd1079d2 100644 --- a/c/cmake_modules/DefineOptions.cmake +++ b/c/cmake_modules/DefineOptions.cmake @@ -86,6 +86,9 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}") #---------------------------------------------------------------------- set_option_category("Compile and link") + define_option_string(ADBC_BUILD_WARNING_LEVEL + "CHECKIN to enable Werror, PRODUCTION otherwise" "") + define_option_string(ADBC_CXXFLAGS "Compiler flags to append when compiling ADBC C++ libraries" "") define_option_string(ADBC_GO_BUILD_TAGS diff --git a/c/driver/common/CMakeLists.txt b/c/driver/common/CMakeLists.txt index 33dd1c8d0a..0da24bb782 100644 --- a/c/driver/common/CMakeLists.txt +++ b/c/driver/common/CMakeLists.txt @@ -16,6 +16,7 @@ # under the License. add_library(adbc_driver_common STATIC utils.c) +adbc_configure_target(adbc_driver_common) set_target_properties(adbc_driver_common PROPERTIES POSITION_INDEPENDENT_CODE ON) target_include_directories(adbc_driver_common PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/vendor") diff --git a/c/driver/postgresql/connection.cc b/c/driver/postgresql/connection.cc index 611cd51325..0e79f63052 100644 --- a/c/driver/postgresql/connection.cc +++ b/c/driver/postgresql/connection.cc @@ -107,7 +107,7 @@ class PqResultHelper { AdbcStatusCode Execute() { std::vector param_c_strs; - for (auto index = 0; index < param_values_.size(); index++) { + for (size_t index = 0; index < param_values_.size(); index++) { param_c_strs.push_back(param_values_[index].c_str()); } @@ -375,8 +375,8 @@ class PqGetObjectsHelper { const char** table_types = table_types_; while (*table_types != NULL) { auto table_type_str = std::string(*table_types); - if (auto search = kPgTableTypes.find(table_type_str); - search != kPgTableTypes.end()) { + auto search = kPgTableTypes.find(table_type_str); + if (search != kPgTableTypes.end()) { table_type_filter.push_back(search->second); } table_types++; diff --git a/c/validation/CMakeLists.txt b/c/validation/CMakeLists.txt index 3c83f95c34..2f6549b5e7 100644 --- a/c/validation/CMakeLists.txt +++ b/c/validation/CMakeLists.txt @@ -16,6 +16,7 @@ # under the License. add_library(adbc_validation OBJECT adbc_validation.cc adbc_validation_util.cc) +adbc_configure_target(adbc_validation) target_compile_features(adbc_validation PRIVATE cxx_std_17) target_include_directories(adbc_validation SYSTEM PRIVATE "${REPOSITORY_ROOT}" "${REPOSITORY_ROOT}/c/driver/" From 4b0d1c58101c251dbbbdc0569acc58627357fb78 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 23 Jun 2023 16:53:46 -0400 Subject: [PATCH 2/4] Fix Windows --- c/cmake_modules/AdbcDefines.cmake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c/cmake_modules/AdbcDefines.cmake b/c/cmake_modules/AdbcDefines.cmake index 34166788b2..7c69f279b6 100644 --- a/c/cmake_modules/AdbcDefines.cmake +++ b/c/cmake_modules/AdbcDefines.cmake @@ -71,7 +71,7 @@ endif() set(ADBC_C_CXX_FLAGS) if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN") if(MSVC) - set(ADBC_C_CXX_FLAGS /Wall /Werror) + set(ADBC_C_CXX_FLAGS /Wall /WX) elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") From 17d3416a293cd37c79b33d2fb07b2d23dd5eae40 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 23 Jun 2023 17:00:15 -0400 Subject: [PATCH 3/4] Fix warning --- c/validation/adbc_validation_util.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/c/validation/adbc_validation_util.h b/c/validation/adbc_validation_util.h index a239e769f7..fec5e7589d 100644 --- a/c/validation/adbc_validation_util.h +++ b/c/validation/adbc_validation_util.h @@ -200,7 +200,7 @@ struct StreamReader { /// \brief Read an AdbcGetInfoData struct with RAII safety struct GetObjectsReader { - explicit GetObjectsReader(struct ArrowArrayView* array_view) : array_view_(array_view) { + explicit GetObjectsReader(struct ArrowArrayView* array_view) { // TODO: this swallows any construction errors get_objects_data_ = AdbcGetObjectsDataInit(array_view); } @@ -214,7 +214,6 @@ struct GetObjectsReader { } private: - struct ArrowArrayView* array_view_; struct AdbcGetObjectsData* get_objects_data_; }; From 90f8d3812478c827109d405f40c49a3e94384542 Mon Sep 17 00:00:00 2001 From: David Li Date: Fri, 23 Jun 2023 18:25:06 -0400 Subject: [PATCH 4/4] Suggestions --- c/cmake_modules/AdbcDefines.cmake | 40 ++++++++++++------------------- c/cmake_modules/BuildUtils.cmake | 6 ++--- 2 files changed, 17 insertions(+), 29 deletions(-) diff --git a/c/cmake_modules/AdbcDefines.cmake b/c/cmake_modules/AdbcDefines.cmake index 7c69f279b6..466c90d8e3 100644 --- a/c/cmake_modules/AdbcDefines.cmake +++ b/c/cmake_modules/AdbcDefines.cmake @@ -60,40 +60,30 @@ if(CXX_LINKER_SUPPORTS_VERSION_SCRIPT) endif() # Set common build options -if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "") - if("${CMAKE_BUILD_TYPE}" STREQUAL "RELEASE") +if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "") + string(TOLOWER "${CMAKE_BUILD_TYPE}" _lower_build_type) + if("${_lower_build_type}" STREQUAL "release") set(ADBC_BUILD_WARNING_LEVEL "PRODUCTION") else() set(ADBC_BUILD_WARNING_LEVEL "CHECKIN") endif() endif() -set(ADBC_C_CXX_FLAGS) -if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN") - if(MSVC) - set(ADBC_C_CXX_FLAGS /Wall /WX) - elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" - OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" - OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - set(ADBC_C_CXX_FLAGS -Wall -Werror) - else() - message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") - endif() -elseif("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "PRODUCTION") - if(MSVC) - set(CMAKE_C_FLAGS /Wall) - set(ADBC_C_CXX_FLAGS /Wall) - elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" - OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" - OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") - set(ADBC_C_CXX_FLAGS -Wall) - else() - message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") - endif() +if(MSVC) + set(ADBC_C_CXX_FLAGS_CHECKIN /Wall /WX) + set(ADBC_C_CXX_FLAGS_PRODUCTION /Wall) +elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "Clang" + OR CMAKE_CXX_COMPILER_ID STREQUAL "GNU") + set(ADBC_C_CXX_FLAGS_CHECKIN -Wall -Werror) + set(ADBC_C_CXX_FLAGS_PRODUCTION -Wall) +else() + message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") endif() macro(adbc_configure_target TARGET) - target_compile_options(${TARGET} PRIVATE ${ADBC_C_CXX_FLAGS}) + target_compile_options(${TARGET} + PRIVATE ${ADBC_C_CXX_FLAGS_${ADBC_BUILD_WARNING_LEVEL}}) endmacro() # Common testing setup diff --git a/c/cmake_modules/BuildUtils.cmake b/c/cmake_modules/BuildUtils.cmake index 6d6438d336..de1a7b2a8d 100644 --- a/c/cmake_modules/BuildUtils.cmake +++ b/c/cmake_modules/BuildUtils.cmake @@ -166,7 +166,6 @@ function(ADD_ARROW_LIB LIB_NAME) add_library(${LIB_NAME}_objlib OBJECT ${ARG_SOURCES}) # Necessary to make static linking into other shared libraries work properly set_property(TARGET ${LIB_NAME}_objlib PROPERTY POSITION_INDEPENDENT_CODE 1) - set_property(TARGET ${LIB_NAME}_objlib PROPERTY CXX_STANDARD 17) set_property(TARGET ${LIB_NAME}_objlib PROPERTY CXX_STANDARD_REQUIRED ON) if(ARG_DEPENDENCIES) add_dependencies(${LIB_NAME}_objlib ${ARG_DEPENDENCIES}) @@ -197,7 +196,6 @@ function(ADD_ARROW_LIB LIB_NAME) adbc_configure_target(${LIB_NAME}_objlib) # https://github.com/apache/arrow-adbc/issues/81 target_compile_features(${LIB_NAME}_objlib PRIVATE cxx_std_11) - set_property(TARGET ${LIB_NAME}_objlib PROPERTY CXX_STANDARD 11) else() # Prepare arguments for separate compilation of static and shared libs below # TODO: add PCH directives @@ -213,7 +211,7 @@ function(ADD_ARROW_LIB LIB_NAME) if(BUILD_SHARED) add_library(${LIB_NAME}_shared SHARED ${LIB_DEPS}) - set_property(TARGET ${LIB_NAME}_shared PROPERTY CXX_STANDARD 17) + target_compile_features(${LIB_NAME}_shared PRIVATE cxx_std_11) set_property(TARGET ${LIB_NAME}_shared PROPERTY CXX_STANDARD_REQUIRED ON) adbc_configure_target(${LIB_NAME}_shared) if(EXTRA_DEPS) @@ -311,7 +309,7 @@ function(ADD_ARROW_LIB LIB_NAME) if(BUILD_STATIC) add_library(${LIB_NAME}_static STATIC ${LIB_DEPS}) - set_property(TARGET ${LIB_NAME}_shared PROPERTY CXX_STANDARD 17) + target_compile_features(${LIB_NAME}_static PRIVATE cxx_std_11) set_property(TARGET ${LIB_NAME}_shared PROPERTY CXX_STANDARD_REQUIRED ON) adbc_configure_target(${LIB_NAME}_static) if(EXTRA_DEPS)