You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "WillAyd (via GitHub)" <gi...@apache.org> on 2023/04/20 20:48:46 UTC
[GitHub] [arrow-adbc] WillAyd opened a new pull request, #597: refactor(c): Cmake refactor
WillAyd opened a new pull request, #597:
URL: https://github.com/apache/arrow-adbc/pull/597
This is a redo of #314
--
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 #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1521548739
Alright, it's just the R jobs failing so I'll merge this too. Thanks again for tackling this!
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1518034244
Yea sorry meant to mark as a WIP. Agreed on all of your comments - looking to make green first then will come around to address those
--
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 #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597
--
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 #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1521551603
I filed #608 for the target_include_directories thing
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175877545
##########
c/driver/postgresql/CMakeLists.txt:
##########
@@ -53,14 +42,15 @@ add_arrow_lib(adbc_driver_postgresql
${ADBC_LINK_FLAGS}
SHARED_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
- nanoarrow
STATIC_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
nanoarrow
${LIBPQ_STATIC_LIBRARIES})
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${LIBPQ_INCLUDE_DIRS})
+include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
Review Comment:
Makes sense. I'll take a look upstream next
--
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 a diff in pull request #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1173154338
##########
c/driver/common/CMakeLists.txt:
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+add_library(adbc_driver_common STATIC utils.c)
+include_directories(SYSTEM ${REPOSITORY_ROOT})
Review Comment:
All(?) references to REPOSITORY_ROOT can be replaced with a proper CMake variable (CMAKE_SOURCE_DIR I think)
##########
c/CMakeLists.txt:
##########
@@ -0,0 +1,43 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+cmake_minimum_required(VERSION 3.18)
+get_filename_component(REPOSITORY_ROOT ".." ABSOLUTE)
+message(STATUS "${REPOSITORY_ROOT}")
+list(APPEND CMAKE_MODULE_PATH "${REPOSITORY_ROOT}/c/cmake_modules/")
+include(AdbcDefines)
+include(BuildUtils)
+
+project(adbc
+ VERSION "${ADBC_BASE_VERSION}"
+ LANGUAGES CXX)
+include(CTest)
+
+add_subdirectory(vendor/nanoarrow)
+add_subdirectory(driver/common)
+
+if(ADBC_BUILD_DRIVER_MANAGER)
Review Comment:
nit: arrow just uses `ARROW_COMPONENT`, can we use `ADBC_COMPONENT`?
##########
c/vendor/nanoarrow/CMakeLists.txt:
##########
@@ -16,11 +15,12 @@
# specific language governing permissions and limitations
# under the License.
-main() {
- local CMAKE_BINARY_DIR="$1"
- local TEST_OR_BENCHMARK="$2"
- local TEST_PATH="$3"
- exec "${CMAKE_BINARY_DIR}/${TEST_PATH}"
-}
+# Common definitions for the CMake projects in this repository.
+# Must define REPOSITORY_ROOT before including this.
Review Comment:
Copy-paste?
##########
c/driver/postgresql/CMakeLists.txt:
##########
@@ -53,14 +42,15 @@ add_arrow_lib(adbc_driver_postgresql
${ADBC_LINK_FLAGS}
SHARED_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
- nanoarrow
STATIC_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
nanoarrow
${LIBPQ_STATIC_LIBRARIES})
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${LIBPQ_INCLUDE_DIRS})
+include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
Review Comment:
Ideally we should use target_include_directories now that we have multiple targets
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175915222
##########
c/CMakeLists.txt:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+cmake_minimum_required(VERSION 3.18)
+get_filename_component(REPOSITORY_ROOT ".." ABSOLUTE)
+list(APPEND CMAKE_MODULE_PATH "${REPOSITORY_ROOT}/c/cmake_modules/")
+include(AdbcDefines)
+include(BuildUtils)
+project(adbc
+ VERSION "${ADBC_BASE_VERSION}"
+ LANGUAGES CXX)
Review Comment:
Nice catch - fixed
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1520963422
> Readmes and docs need to be updated too (though maybe you were waiting for a checkmark before doing all that slightly_smiling_face)
Simply forgot to do this but think I have it done now
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1520864745
Alright @lidavidm I think this is ready for review. The current CI failures I believe are flaky or related to an R-release issue noted on another thread
--
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 a diff in pull request #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175906285
##########
c/CMakeLists.txt:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+cmake_minimum_required(VERSION 3.18)
+get_filename_component(REPOSITORY_ROOT ".." ABSOLUTE)
+list(APPEND CMAKE_MODULE_PATH "${REPOSITORY_ROOT}/c/cmake_modules/")
+include(AdbcDefines)
+include(BuildUtils)
+project(adbc
+ VERSION "${ADBC_BASE_VERSION}"
+ LANGUAGES CXX)
Review Comment:
oh - this should probably be `C CXX` given the SQLite driver and 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] WillAyd commented on a diff in pull request #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175762238
##########
c/driver/common/CMakeLists.txt:
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+add_library(adbc_driver_common STATIC utils.c)
+include_directories(SYSTEM ${REPOSITORY_ROOT})
Review Comment:
Looking at this more closely the `REPOSITORY_ROOT` is still needed one way or another when including adbc.h, since that technically lives outside of the /c source directory.
Happy to do something here but I think better in a follow up - otherwise might make these changes confusing
--
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 #597: refactor(c): Cmake refactor
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175807047
##########
c/driver/postgresql/CMakeLists.txt:
##########
@@ -53,14 +42,15 @@ add_arrow_lib(adbc_driver_postgresql
${ADBC_LINK_FLAGS}
SHARED_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
- nanoarrow
STATIC_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
nanoarrow
${LIBPQ_STATIC_LIBRARIES})
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${LIBPQ_INCLUDE_DIRS})
+include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
Review Comment:
Gave this a shot but I wasn't able to get it to play nicely with `add_arrow_lib` - seems like it was just being ignored? Happy to look deeper in a follow up
--
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 a diff in pull request #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#discussion_r1175861623
##########
c/driver/postgresql/CMakeLists.txt:
##########
@@ -53,14 +42,15 @@ add_arrow_lib(adbc_driver_postgresql
${ADBC_LINK_FLAGS}
SHARED_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
- nanoarrow
STATIC_LINK_LIBS
${LIBPQ_LINK_LIBRARIES}
nanoarrow
${LIBPQ_STATIC_LIBRARIES})
include_directories(SYSTEM ${REPOSITORY_ROOT})
include_directories(SYSTEM ${REPOSITORY_ROOT}/c/)
include_directories(SYSTEM ${LIBPQ_INCLUDE_DIRS})
+include_directories(SYSTEM ${REPOSITORY_ROOT}/c/vendor)
Review Comment:
I think there were upstream changes to Arrow's CMake to enable that to work after we forked from there, now that I think about it. We can leave that for a follow up.
##########
c/CMakeLists.txt:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+cmake_minimum_required(VERSION 3.18)
+
+get_filename_component(REPOSITORY_ROOT ".." ABSOLUTE)
+message(STATUS "${REPOSITORY_ROOT}")
Review Comment:
Stray print? (Was this here before?)
##########
c/driver/common/CMakeLists.txt:
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+add_library(adbc_driver_common STATIC utils.c)
+include_directories(SYSTEM ${REPOSITORY_ROOT})
Review Comment:
Ah, ok - then let's not worry about it
##########
c/CMakeLists.txt:
##########
@@ -0,0 +1,48 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+cmake_minimum_required(VERSION 3.18)
+
+get_filename_component(REPOSITORY_ROOT ".." ABSOLUTE)
+message(STATUS "${REPOSITORY_ROOT}")
+list(APPEND CMAKE_MODULE_PATH "${REPOSITORY_ROOT}/c/cmake_modules/")
+include(AdbcDefines)
+include(BuildUtils)
+project(adbc
+ VERSION "${ADBC_BASE_VERSION}"
+ LANGUAGES CXX)
+
+include(CTest)
+
+add_subdirectory(vendor/nanoarrow)
+add_subdirectory(driver/common)
+
+if(ADBC_DRIVER_MANAGER)
+ add_subdirectory(driver_manager)
+endif()
+
+if(ADBC_DRIVER_SQLITE)
+ add_subdirectory(driver/sqlite)
+endif()
+
+if(ADBC_DRIVER_POSTGRESQL)
+ add_subdirectory(driver/postgresql)
+endif()
+
+if(ADBC_DRIVER_FLIGHTSQL)
+ add_subdirectory(driver/flightsql)
+endif()
Review Comment:
sorry to nit but can we sort these alphabetically 😅
--
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 #597: refactor(c): Cmake refactor
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on PR #597:
URL: https://github.com/apache/arrow-adbc/pull/597#issuecomment-1521014929
CC @zeroshade this will also impact your Go Snowflake driver a tiny bit
--
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