build(c): force C++11 for drivers for R's sake #844
Conversation
|
The build isn't -Wextra clean right now, sadly. |
|
Ugh, some of the utility code needs to be fixed, moving to draft |
c/cmake_modules/BuildUtils.cmake
Outdated
| 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) |
There was a problem hiding this comment.
This may be redundant. (I think that we only need target_compile_features().)
c/cmake_modules/AdbcDefines.cmake
Outdated
| -Wpedantic | ||
| -Wno-unused-parameter | ||
| -Wunused-result) | ||
| if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "") |
There was a problem hiding this comment.
Can we simplify this?
| if(NOT ADBC_BUILD_WARNING_LEVEL OR ADBC_BUILD_WARNING_LEVEL STREQUAL "") | |
| if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "") |
c/cmake_modules/AdbcDefines.cmake
Outdated
| endif() | ||
| elseif("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "PRODUCTION") | ||
| if(MSVC) | ||
| set(CMAKE_C_FLAGS /Wall) |
| else() | ||
| message(WARNING "Unknown compiler: ${CMAKE_CXX_COMPILER_ID}") | ||
| endif() | ||
| endif() |
There was a problem hiding this comment.
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}})
|
Guess all the utils stuff is complaining about NanoArrow's |
c/cmake_modules/AdbcDefines.cmake
Outdated
| set(ADBC_C_CXX_FLAGS) | ||
| if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "CHECKIN") | ||
| if(MSVC) | ||
| set(ADBC_C_CXX_FLAGS /Wall /WX) |
There was a problem hiding this comment.
Not a Windows expert by any means but wondering if we should start with a lower warning level like W2, W3, or W4 instead of Wall?
There was a problem hiding this comment.
Most of the warnings are coming from the vendored nanoarrow which doesn't build with warnings on MSVC. A lot of the case statement warnings are easy enough to fix but will be hard to warn on MSVC downstream if also not set up in nanoarrow
There was a problem hiding this comment.
I fixed a number of them here: apache/arrow-nanoarrow#191 (it looks like vendored nanoarrow predates that?)
|
Thanks Kou! Will: I'd like to have /Wall if possible, but given the flood of warnings from Nanoarrow, that may not be possible...and as I recall, MSVC didn't support the equivalent of -isystem until very recently. |
|
Yea makes sense. If this PR isn't a rush I don't think it's unreasonable to get Wall to work for MSVC |
|
I'm somewhat inclined to skip MSVC for now just so that we enforce C++11 for R, and get MSVC fixed separately since it looks like it'll be a giant pain. |
|
Hmm, that appears to have fixed things in CI (what I expect is that the next person to try to do development on MSVC is going to get a giant wall of warnings) |
There was a problem hiding this comment.
I can't speak to the CMake...the R packages don't use CMake or MSVC and it seems like that might be forcing some complexity that could be avoided
Perhaps instead of forcing C++11 in CMake we could just add a line - {os: windows-latest, r: '3.6', pkg: 'adbcpostgresql'} right here: https://github.com/apache/arrow-adbc/blob/main/.github/workflows/native-unix.yml#L596C11-L596C72 . I think checking that it builds is sufficient for CI...I can run tests from R with a Docker setup locally before submitting to CRAN. I'm happy to make that CI change in another PR.
For Centos7 I ran:
# docker run --rm -it -v $(pwd):/arrow-adbc ghcr.io/apache/arrow-nanoarrow:centos7-arm64
cd /arrow-adbc
yum install postgresql-devel
R -e 'install.packages("nanoarrow", repos = "https://cloud.r-project.org")'
R CMD INSTALL r/adbcdrivermanager --preclean
R CMD INSTALL r/adbcpostgresql --preclean...with no problems.
I did run into a slight problem on R 3.6/Windows because NODATA is apparently not defined. Defining it to 148 in postgres_copy_reader.h if not previously defined seemed to do the trick. (Again, I'm happy to do that in another PR).
|
An R 3.6 pipeline would be useful in combination with this, just to verify that it actually works. But I'd like to have the CMake config so that we get immediate feedback for local development. |
|
I filed #848 |
It seems like an older version may be causing problems with MSVC warnings (#847, #844 (comment) ).
Fixes #815.
Fixes #842.