You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "lidavidm (via GitHub)" <gi...@apache.org> on 2023/06/23 19:19:58 UTC
[GitHub] [arrow-adbc] lidavidm opened a new pull request, #844: build(c): force C++11 for drivers for R's sake
lidavidm opened a new pull request, #844:
URL: https://github.com/apache/arrow-adbc/pull/844
Fixes #815.
Fixes #842.
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605061002
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.
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605621662
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.
--
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
[GitHub] [arrow-adbc] paleolimbot commented on a diff in pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#discussion_r1240563977
##########
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 /WX)
Review Comment:
I fixed a number of them here: https://github.com/apache/arrow-nanoarrow/pull/191 (it looks like vendored nanoarrow predates that?)
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1604765084
The build isn't -Wextra clean right now, sadly.
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605622015
I filed https://github.com/apache/arrow-adbc/issues/848
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1604957316
Ugh, some of the utility code needs to be fixed, moving to draft
--
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
[GitHub] [arrow-adbc] kou commented on a diff in pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#discussion_r1240405645
##########
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 "")
Review Comment:
Can we simplify this?
```suggestion
if("${ADBC_BUILD_WARNING_LEVEL}" STREQUAL "")
```
##########
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)
Review Comment:
This may be redundant. (I think that we only need `target_compile_features()`.)
##########
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 /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)
Review Comment:
Do we need this?
##########
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 /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()
+endif()
Review Comment:
We may want to use the following style:
```cmake
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}})
```
--
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
[GitHub] [arrow-adbc] lidavidm merged pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844
--
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
[GitHub] [arrow-adbc] WillAyd commented on a diff in pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#discussion_r1240442781
##########
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 /WX)
Review 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
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605057675
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.
--
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
[GitHub] [arrow-adbc] WillAyd commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605060558
Yea makes sense. If this PR isn't a rush I don't think it's unreasonable to get Wall to work for MSVC
--
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
[GitHub] [arrow-adbc] WillAyd commented on a diff in pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#discussion_r1240435868
##########
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 /WX)
Review 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?
--
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
[GitHub] [arrow-adbc] WillAyd commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605004614
Guess all the utils stuff is complaining about NanoArrow's `GetInt` method returning a 64 bit integer, while some of the fields we are assigning to are 16/32. I can add some more safety checks there in a separate PR if it helps
--
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
[GitHub] [arrow-adbc] lidavidm commented on pull request #844: build(c): force C++11 for drivers for R's sake
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #844:
URL: https://github.com/apache/arrow-adbc/pull/844#issuecomment-1605088135
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)
--
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