Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build(c): force C++11 for drivers for R's sake #844

Merged
merged 4 commits into from
Jun 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions c/cmake_modules/AdbcDefines.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,30 @@ 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("${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()

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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to use the following style:

if(MSVC)
  set(ADBC_C_CXX_FLAGS_CHECKIN "/Wall /WX")
  set(ADBC_C_CXX_FLAGS_PRODUCTION "/Wall /WX")
elseif(CMAKE_CXX_COMPILER_ID STREQUAL "AppleClang" ...)
  set(ADBC_C_CXX_FLAGS_CHECKIN "...")
  set(ADBC_C_CXX_FLAGS_PRODUCTION "...")
else()
  ...
endif()

target_compile_options(${TARGET} PRIVATE ${ADBC_C_CXX_FLAGS_${ADBC_BUILD_WARNING_LEVEL}})


macro(adbc_configure_target TARGET)
target_compile_options(${TARGET}
PRIVATE ${ADBC_C_CXX_FLAGS_${ADBC_BUILD_WARNING_LEVEL}})
endmacro()

# Common testing setup
Expand Down
17 changes: 12 additions & 5 deletions c/cmake_modules/BuildUtils.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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})
Expand Down Expand Up @@ -194,6 +193,9 @@ 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)
else()
# Prepare arguments for separate compilation of static and shared libs below
# TODO: add PCH directives
Expand All @@ -209,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)
Expand Down Expand Up @@ -255,6 +257,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
"$<BUILD_INTERFACE:${ARG_SHARED_LINK_LIBS}>"
Expand Down Expand Up @@ -304,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)
Expand Down Expand Up @@ -342,6 +347,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
"$<INSTALL_INTERFACE:${ARG_STATIC_INSTALL_INTERFACE_LIBS}>")
Expand Down Expand Up @@ -584,6 +592,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
Expand Down Expand Up @@ -637,8 +646,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})
Expand Down
3 changes: 3 additions & 0 deletions c/cmake_modules/DefineOptions.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions c/driver/common/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
6 changes: 3 additions & 3 deletions c/driver/postgresql/connection.cc
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ class PqResultHelper {
AdbcStatusCode Execute() {
std::vector<const char*> 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());
}

Expand Down Expand Up @@ -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++;
Expand Down
1 change: 1 addition & 0 deletions c/validation/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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/"
Expand Down
3 changes: 1 addition & 2 deletions c/validation/adbc_validation_util.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -214,7 +214,6 @@ struct GetObjectsReader {
}

private:
struct ArrowArrayView* array_view_;
struct AdbcGetObjectsData* get_objects_data_;
};

Expand Down