You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "vyasr (via GitHub)" <gi...@apache.org> on 2024/03/19 00:46:46 UTC
[PR] Add an example that systematically tests various build scenarios [arrow-nanoarrow]
vyasr opened a new pull request, #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406
Resolves #400
Still a draft, I need to do some cleanup as well as add testing of the C++ API as well.
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1534327908
##########
examples/cmake-scenarios/CMakeLists.txt:
##########
@@ -0,0 +1,73 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+cmake_minimum_required(VERSION 3.12)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
Review Comment:
`std::optional` requires C++17, so that's probably what you're hitting. I don't recall using any other C++17 features offhand. I could check and see, but in any case we're stripping down the example so not a big deal.
I assumed C++17 was OK because [that's what arrow requires](https://arrow.apache.org/docs/developers/cpp/building.html#system-setup), but it's valuable for nanoarrow to support older systems.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1531449200
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
That's what the [NAMESPACE](https://cmake.org/cmake/help/latest/command/install.html#export) is for, it is considered good practice to use namespaced targets like `nanoarrow::coverage_config` even within projects (which requires creating an ALIAS) but def for exported targets. We should use that here as well.
##########
CMakeLists.txt:
##########
Review Comment:
I don't quite understand the approach you are taking with this two step process. IIRC that shouldn't be necessary but I'll have to come back to this after going through the documentation and test some things.
##########
cmake/config.cmake.in:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+
+@PACKAGE_INIT@
+
+cmake_minimum_required(VERSION @CMAKE_MINIMUM_REQUIRED_VERSION@)
Review Comment:
That's probably in the fetch content version picking up 3.25 from the example cml due to
> The <min> version of CMake given to the most recent call to the [cmake_minimum_required(VERSION)](https://cmake.org/cmake/help/latest/command/cmake_minimum_required.html#command:cmake_minimum_required) command in the current variable scope or any parent variable scope.
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
Review Comment:
This seems unnecessarily high? 3.16 should be fine or am I missing anything that requires 3.25? (It could confuse potential users too)
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
I would add some minor bit of example code to both files. (no idea what thought ^^) This would make for a better example (e.g. custom namespace usage) and properly exercise linking.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532422413
##########
cmake/config.cmake.in:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+
+@PACKAGE_INIT@
+
+cmake_minimum_required(VERSION @CMAKE_MINIMUM_REQUIRED_VERSION@)
Review Comment:
Yeah no specific reason. I'm usually running a very recent version of CMake so I set this, but we can definitely use earlier. I bumped up from 3.11 to make my job easier is all.
It looks like the examples CI just uses the default on the ubuntu-latest. Do we have any testing that validates that we can run on a specific version of CMake? Ideally CI would be configured to run on the oldest supported version of CMake so that we have confirmation that we're not using any newer features.
Also, I'll note that when we get around to similar improvements for the device extension we may want to require a newer CMake. There have been a ton of improvements to CMake/CUDA integration. At minimum I would say we should require 3.17 so that we can use FindCUDAToolkit instead of FindCUDA.
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
Review Comment:
Yup I can drop to a lower version.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532497774
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
install(DIRECTORY src/
DESTINATION include
FILES_MATCHING
- PATTERN "*.h")
+ PATTERN "*.h*")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/generated/nanoarrow_config.h
DESTINATION include/nanoarrow)
+
+ # Set up build tree as a CMake package.
+ include(CMakePackageConfigHelpers)
+ write_basic_package_version_file(
+ "${PROJECT_BINARY_DIR}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
+ COMPATIBILITY AnyNewerVersion)
+ configure_package_config_file("${CMAKE_CURRENT_LIST_DIR}/cmake/config.cmake.in"
+ "${PROJECT_BINARY_DIR}/nanoarrow-config.cmake"
+ INSTALL_DESTINATION "${PROJECT_BINARY_DIR}")
+ export(EXPORT nanoarrow-exports FILE "${PROJECT_BINARY_DIR}/nanoarrow-targets.cmake")
+
+ # Set up install tree as a CMake package.
+ include(GNUInstallDirs)
+ set(install_location "${CMAKE_INSTALL_LIBDIR}/cmake/nanoarrow")
+ set(build_location "${CMAKE_BINARY_DIR}/${install_location}")
+ write_basic_package_version_file(
+ "${build_location}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
Review Comment:
That sounds perfect!
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532634669
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
I've added real examples now so I'm not sure that the version check is relevant anymore, but let me know what you're thinking.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1531445501
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
+option(FIND_NANOARROW "Find an existing nanoarrow" ON)
+
+# When adding nanoarrow's CMake directory to a CMake project that contains a library
+# intended for use by others, set NANOARROW_NAMESPACE to rename symbols in the
+# nanoarrow library such that they do not collide with other libraries that may also
+# link to their own copy of nanoarrow. You may wish to include the namespace only
+# on release builds, since the namespace implementation obscures inline help
+# available in many text editors.
+set(NANOARROW_NAMESPACE "ExampleCmakeMinimal")
+
+if(FIND_NANOARROW)
+ find_package(nanoarrow 0.5.0 REQUIRED)
Review Comment:
Depending on the find-module/config this usually means a minimum version, so it doesn't necessarily need to be updated. If you know the code used has a required minimum version it's good practice to add it in the find call to avoid confusing compile errors.
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1534426874
##########
examples/cmake-scenarios/src/library.cpp:
##########
@@ -0,0 +1,39 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
+#include "nanoarrow/nanoarrow.hpp"
+
+#include "library.hpp"
+
+std::optional<std::pair<std::unique_ptr<ArrowArray>, std::unique_ptr<ArrowSchema>>>
+make_simple_array() {
+ nanoarrow::UniqueArray tmp_array;
+ nanoarrow::UniqueSchema tmp_schema;
+ auto result = make_simple_array(tmp_array.get(), tmp_schema.get());
+ if (result != 0) {
+ if (tmp_array.get()->release) tmp_array.get()->release(tmp_array.get());
+ if (tmp_schema.get()->release) tmp_schema.get()->release(tmp_schema.get());
+ return std::nullopt;
+ }
+ auto ret_array = std::make_unique<ArrowArray>();
Review Comment:
OK yeah so in that example the library interface is a C++ interface, so it's fine to return a `UniqueSchema`. That makes sense, I was just saying that I had written my example with the assumption that you'd typically want to code this kind of thing with nanoarrow assuming that the other library could be a C library consuming it.
I guess the core issue here is that I wrote an example that's a very odd hybrid of C and C++. I made some assumptions that we'd end up interfacing with a C library, but at the moment the functions use C++ interfaces. That's also clear from my second question about lifetimes. I think what you're getting at is that because I'm using a `unique_ptr` to the array, you'd need to set a custom deleter on that object so that when the pointer goes out of scope the right deleter is called, which is completely true. I was just assuming that in practice this object would need to be released and the underlying raw pointer would need to be passed back, at which point the real consumer would be in charge of calling the release callback as per the C data interface contract, but you're totally right that as the code is written now there's a potential memory leak because I'm handing back a `unique_ptr` that could go out of scope and delete the array without actually freeing its underlying buffers.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532993260
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
I removed the target and just set the options as public on the nanoarrow target so that they'll propagate to everything linking to it. That should be sufficient and avoids the weirdness of this target.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2011012682
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/406?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 88.74%. Comparing base [(`dc50114`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/dc50114756b7e9067b42181a6a86f928effc6e68?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`6a7678d`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/406?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #406 +/- ##
=======================================
Coverage 88.74% 88.74%
=======================================
Files 81 81
Lines 14398 14398
=======================================
Hits 12778 12778
Misses 1620 1620
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/406?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1531340192
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
install(DIRECTORY src/
DESTINATION include
FILES_MATCHING
- PATTERN "*.h")
+ PATTERN "*.h*")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/generated/nanoarrow_config.h
DESTINATION include/nanoarrow)
+
+ # Set up build tree as a CMake package.
+ include(CMakePackageConfigHelpers)
+ write_basic_package_version_file(
+ "${PROJECT_BINARY_DIR}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
+ COMPATIBILITY AnyNewerVersion)
+ configure_package_config_file("${CMAKE_CURRENT_LIST_DIR}/cmake/config.cmake.in"
+ "${PROJECT_BINARY_DIR}/nanoarrow-config.cmake"
+ INSTALL_DESTINATION "${PROJECT_BINARY_DIR}")
+ export(EXPORT nanoarrow-exports FILE "${PROJECT_BINARY_DIR}/nanoarrow-targets.cmake")
+
+ # Set up install tree as a CMake package.
+ include(GNUInstallDirs)
+ set(install_location "${CMAKE_INSTALL_LIBDIR}/cmake/nanoarrow")
+ set(build_location "${CMAKE_BINARY_DIR}/${install_location}")
+ write_basic_package_version_file(
+ "${build_location}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
Review Comment:
No plans at the moment (hence the care about namespacing and vendoring), although I think in a year or so the API will stabilize and we'll consider 1.0.0.
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
Should these example files have a `main()` function that prints out `ArrowNanoarrowVersion()`? I'm not sure what the danger is of missing symbols is here but it seems like it might be nice to check that at least one makes it through.
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
Does this need to be exported? It's come up before that the name `coverage_config` conflicts with another target...is this a good time to prefix it (`nanoarrow_coverage_config`)?
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
+option(FIND_NANOARROW "Find an existing nanoarrow" ON)
+
+# When adding nanoarrow's CMake directory to a CMake project that contains a library
+# intended for use by others, set NANOARROW_NAMESPACE to rename symbols in the
+# nanoarrow library such that they do not collide with other libraries that may also
+# link to their own copy of nanoarrow. You may wish to include the namespace only
+# on release builds, since the namespace implementation obscures inline help
+# available in many text editors.
+set(NANOARROW_NAMESPACE "ExampleCmakeMinimal")
+
+if(FIND_NANOARROW)
+ find_package(nanoarrow 0.5.0 REQUIRED)
Review Comment:
Will the version bit have to be updated with every release? Can it be dropped?
##########
examples/build_tests/build.sh:
##########
Review Comment:
Should running this file be added to `.github/workflows/examples.yaml`?
##########
cmake/config.cmake.in:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+
+@PACKAGE_INIT@
+
+cmake_minimum_required(VERSION @CMAKE_MINIMUM_REQUIRED_VERSION@)
Review Comment:
I wonder if this needs to be configured to a lower value...when I run `build.sh` I get:
```
-- Building using CMake version: 3.24.1
CMake Error at CMakeLists.txt:20 (cmake_minimum_required):
CMake 3.25 or higher is required. You are running version 3.24.1
```
3.25 seems like a pretty steep version constraint?
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2013232589
> You have it marked as a TODO already, but make sure the FetchContent call doesn't point to your fork.
I'll wait until I have approvals from both of you and then swap this out. Once I make that change CI won't pass on this PR, so I think we should plan to see a green run CI run with all the changes that you both want in place, then I can make that final change and you can admin merge.
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2014041946
Cool, thanks to you both for the reviews! I've pushed one last commit changing the FetchContent call to point back to the main branch. We should be good to merge 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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532634149
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
We can definitely discussing namespacing later.
Yeah we should be able to remove this, the problem is that the linkage of that target to nanoarrow itself is specified as public and based on your description it probably should be private.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2010599091
> The example code here is an expanded version of what's in the CMake minimal example
This is OK, but I wonder if just:
```
int main() {
return ArrowNanoarrowCheckRuntime();
}
```
...would be sufficient to check what you've written here. We can always expand the example later but I think keeping it relevant to the includes and symbols at hand is probably best.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532402335
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
I agree, this would be the right solution. I didn't do it initially because 1) I was worried about breaking existing users and 2) I didn't know how far the scope of this PR should go. In hindsight, point 1 is moot because this package wasn't previously importable anyway, so even with adding the namespace existing consumers would still see the un-namespaced targets. For point 2, IMO best practice would be to update all the examples/code within the repo to use the namespaced target consistently whether they're getting nanoarrow via FetchContent or `find_package`. However, I think that's probably out of scope for this PR and can be done in some future work. Would you agree?
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532493285
##########
cmake/config.cmake.in:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+
+@PACKAGE_INIT@
+
+cmake_minimum_required(VERSION @CMAKE_MINIMUM_REQUIRED_VERSION@)
Review Comment:
We don't have anything that checks yet, but it should align with whatever is in existing `CMakeLists.txt`, which might be 3.16. We can always conditionally use `FindCUDAToolkit` for a newer version when finalizing the device extension (scheduled for next quarter).
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2010368774
The example code here is an expanded version of what's in the CMake minimal example. I don't know if you still want to keep both around, I'll leave that up to you.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1533130121
##########
examples/cmake-scenarios/src/library.cpp:
##########
@@ -0,0 +1,39 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
+#include "nanoarrow/nanoarrow.hpp"
+
+#include "library.hpp"
+
+std::optional<std::pair<std::unique_ptr<ArrowArray>, std::unique_ptr<ArrowSchema>>>
+make_simple_array() {
+ nanoarrow::UniqueArray tmp_array;
+ nanoarrow::UniqueSchema tmp_schema;
+ auto result = make_simple_array(tmp_array.get(), tmp_schema.get());
+ if (result != 0) {
+ if (tmp_array.get()->release) tmp_array.get()->release(tmp_array.get());
+ if (tmp_schema.get()->release) tmp_schema.get()->release(tmp_schema.get());
+ return std::nullopt;
+ }
+ auto ret_array = std::make_unique<ArrowArray>();
Review Comment:
If you were going to do this in real life you would need to set a custom deleter to ensure that `array->release` was called for a non-null `array` and non-null `array->release`, and I would generally reccomend using the `UniqueArray` (but I think just a simpler example is the way to go here).
##########
examples/cmake-scenarios/CMakeLists.txt:
##########
@@ -0,0 +1,73 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+cmake_minimum_required(VERSION 3.12)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
Review Comment:
I had to add
```
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED ON)
```
...to get `build.sh` to run successfully (although `nanoarrow.hpp` only requires C++11)
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532495528
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
There's a `ArrowNanoarrowCheckRuntime()` that checks the include version vs. the version reported by `ArrowNanoarrowVersion()` that would probably be useful here.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532413610
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
Also for context @paleolimbot this is how I would anticipate consuming the components that I suggested in #407. You'd produce an importable nanoarrow that has targets like `nanoarrow::nanoarrow`, `nanoarrow::coverage_config`, `nanoarrow::ipc`, `nanoarrow::device`, and then the user could do something like `find_package(nanoarrow COMPONENTS device)` and then `target_link_libraries(my_pkg PRIVATE nanoarrow_device)`.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2007839205
Thank you! I will give this a review today and perhaps bring in some CMake expertise from previous contributors 🙂
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2010654018
> > The example code here is an expanded version of what's in the CMake minimal example
>
> This is OK, but I wonder if just:
>
> ```
> int main() {
> return ArrowNanoarrowCheckRuntime();
> }
> ```
>
> ...would be sufficient to check what you've written here. We can always expand the example later but I think keeping it relevant to the includes and symbols at hand is probably best.
I think it would be good to include something from both the C and C++ headers. So maybe also just a `nanoarrow::UniqueArray tmp;` declaration would suffice?
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532638734
##########
CMakeLists.txt:
##########
Review Comment:
If by two-step you're referring to the fact that I create separate files for the build and install trees, that's because the paths within those files cannot be identical due to the slightly different layouts. We could try to align them but that would place system-looking directories like `lib/cmake` into the build tree which is slightly suboptimal IMO and easy to avoid in this case.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532408983
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
install(DIRECTORY src/
DESTINATION include
FILES_MATCHING
- PATTERN "*.h")
+ PATTERN "*.h*")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/generated/nanoarrow_config.h
DESTINATION include/nanoarrow)
+
+ # Set up build tree as a CMake package.
+ include(CMakePackageConfigHelpers)
+ write_basic_package_version_file(
+ "${PROJECT_BINARY_DIR}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
+ COMPATIBILITY AnyNewerVersion)
+ configure_package_config_file("${CMAKE_CURRENT_LIST_DIR}/cmake/config.cmake.in"
+ "${PROJECT_BINARY_DIR}/nanoarrow-config.cmake"
+ INSTALL_DESTINATION "${PROJECT_BINARY_DIR}")
+ export(EXPORT nanoarrow-exports FILE "${PROJECT_BINARY_DIR}/nanoarrow-targets.cmake")
+
+ # Set up install tree as a CMake package.
+ include(GNUInstallDirs)
+ set(install_location "${CMAKE_INSTALL_LIBDIR}/cmake/nanoarrow")
+ set(build_location "${CMAKE_BINARY_DIR}/${install_location}")
+ write_basic_package_version_file(
+ "${build_location}/nanoarrow-config-version.cmake"
+ VERSION ${nanoarrow_VERSION}
+ # Not sure if nanoarrow plans to promise any compatibility.
Review Comment:
The reason I ask this question is that we need to decide what the behavior should be if someone does `find_package(nanoarrow 0.5.0)` and they have nanoarrow 0.4.0 installed on the system. Should that be considered found, or could 0.5.0 have broken something from 0.4.0? What about between major versions 2.0.0 and 1.0.0? The setting here will control that.
It sounds like perhaps the best solution for the moment is to set this to `ExactVersion`, but perhaps after 1.0.0 change this to `SameMajorVersion`. Does that align with your plans?
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532402335
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
I agree, this would be the right solution. I didn't do it initially because 1) I was worried about breaking existing users and 2) I didn't know how far the scope of this PR should go. In hindsight, point 1 is moot because this package wasn't previously importable anyway, so even with adding the namespace existing consumers would still see the un-namespaced targets. For point 2, IMO best practice would be to update all the examples/code within the repo to use the namespaced target consistently whether they're getting nanoarrow via FetchContent or `find_package`. This would be especially important for tools that rely on the former as a fallback for the latter, either manually or via something like CPM. However, I think that's probably out of scope for this PR and can be done in some future work. Would you agree?
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2011014335
> I think it would be good to include something from both the C and C++ headers. So maybe also just a nanoarrow::UniqueArray tmp; declaration would suffice?
That's a great point, and we should probably use it to do something to make extra sure it isn't optimized out. Maybe:
```cpp
int main() {
nanoarrow::UniqueSchema schema;
NANOARROW_RETURN_NOT_OK(ArrowSchemaInitFromType(schema.get(), NANOARROW_TYPE_INT32));
printf("Schema format for int32 is '%s'", schema->format);
return EXIT_SUCCESS;
}
```
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532503949
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
I think the namespacing is a good idea...I am not worried about existing users because everybody should be pinning a version right now (and we've made it pretty easy to do), and because our CMake configuration was almost unusable before this PR. I'm not sure if `ipc` should be `ipc` or `nanoarrow_ipc` (but that's a decision for later anyway).
Either way, the coverage configuration should almost certainly not be exported? It should really only be defined if `NANOARROW_BUILD_TESTS=ON` anyway.
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1534324652
##########
examples/cmake-scenarios/src/library.cpp:
##########
@@ -0,0 +1,39 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
+#include "nanoarrow/nanoarrow.hpp"
+
+#include "library.hpp"
+
+std::optional<std::pair<std::unique_ptr<ArrowArray>, std::unique_ptr<ArrowSchema>>>
+make_simple_array() {
+ nanoarrow::UniqueArray tmp_array;
+ nanoarrow::UniqueSchema tmp_schema;
+ auto result = make_simple_array(tmp_array.get(), tmp_schema.get());
+ if (result != 0) {
+ if (tmp_array.get()->release) tmp_array.get()->release(tmp_array.get());
+ if (tmp_schema.get()->release) tmp_schema.get()->release(tmp_schema.get());
+ return std::nullopt;
+ }
+ auto ret_array = std::make_unique<ArrowArray>();
Review Comment:
I can simplify, but I have two questions (mostly for my edification):
- Are you suggesting that you would make the API `make_simple_array` return a `UniqueArray` rather than an ArrowArray? I wrote this example with the mindset that you'd be writing C++ code internally but ultimately you'd still want to expose a C-like interface, so you'd end up needing to unpack the members of the pair and return the raw pointers via a C function.
- Does the `tmp_array.move(...)` call not transfer the the release function pointer from `tmp_array` to `ret_array`? I assumed that it would transfer all the members of the underlying `ArrowArray`, not just the data buffer pointers.
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1534360446
##########
examples/cmake-scenarios/src/library.cpp:
##########
@@ -0,0 +1,39 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
+#include "nanoarrow/nanoarrow.hpp"
+
+#include "library.hpp"
+
+std::optional<std::pair<std::unique_ptr<ArrowArray>, std::unique_ptr<ArrowSchema>>>
+make_simple_array() {
+ nanoarrow::UniqueArray tmp_array;
+ nanoarrow::UniqueSchema tmp_schema;
+ auto result = make_simple_array(tmp_array.get(), tmp_schema.get());
+ if (result != 0) {
+ if (tmp_array.get()->release) tmp_array.get()->release(tmp_array.get());
+ if (tmp_schema.get()->release) tmp_schema.get()->release(tmp_schema.get());
+ return std::nullopt;
+ }
+ auto ret_array = std::make_unique<ArrowArray>();
Review Comment:
This is all new, so take it with a grain of salt, but usually there are an internal set of helpers that make heavy use of nanoarrow library calls. These helpers should maybe look like:
```c
ArrowErrorCode MakeInt32(ArrowSchema* schema) {
nanoarrow::UniqueSchema tmp;
NANOARROW_RETURN_NOT_OK(ArrowSchemaInitFromType(tmp.get(), NANOARROW_TYPE_INT32));
ArrowSchemaMove(tmp.get(), schema);
return NANOARROW_OK;
}
```
(e.g., return `ArrowErrorCode` so that you can use `NANOARROW_RETURN_NOT_OK()` and use the `Unique` helpers to ensure you can return/throw wherever you want)
Then there comes a point where you're at the interface with another library, where in C++, you probably don't want a pointer output argument and you have some other error handling schema.
```cpp
nanarrow::UniqueSchema MakeInt32Outer() {
nanoarrow::UniqueSchema out;
ArrowErrorCode code = MakeInt32(out.get());
if (code != NANOARROW_OK) {
// report the error via whatever scheme the application is using
}
return out;
}
```
This is all open to interpretation and opinions, of course.
> Does the tmp_array.move(...) call not transfer the the release function pointer from tmp_array to ret_array? I assumed that it would transfer all the members of the underlying ArrowArray, not just the data buffer pointers.
It transfers everything; however, unless you give it very specific instructions, there's no guarantee that the `std::unique_ptr<ArrowArray>` will actually call the release callback when it is deleted.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2010373653
The example code here is an expanded version of what's in the CMake minimal example. I don't know if you still want to keep both around, I'll leave that up to you.
This PR is now ready for another round of review, I've addressed everything that was discussed I think.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532411156
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
Yeah I can add a simple test to make sure the library links correctly. I started simple to make sure that all the headers were being included, but I agree that we need to verify that the library works too.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532603646
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
+option(FIND_NANOARROW "Find an existing nanoarrow" ON)
+
+# When adding nanoarrow's CMake directory to a CMake project that contains a library
+# intended for use by others, set NANOARROW_NAMESPACE to rename symbols in the
+# nanoarrow library such that they do not collide with other libraries that may also
+# link to their own copy of nanoarrow. You may wish to include the namespace only
+# on release builds, since the namespace implementation obscures inline help
+# available in many text editors.
+set(NANOARROW_NAMESPACE "ExampleCmakeMinimal")
+
+if(FIND_NANOARROW)
+ find_package(nanoarrow 0.5.0 REQUIRED)
Review Comment:
OK, based on what we decided in that thread, yes this version is going to have to be bumped every release until we hit 1.0.0. Since this is kind of annoying churn for this example, I'll just make a comment that this is the best practice but remove the version constraint altogether for now. Post-1.0.0 we can add it back here if we like once we switch the compatibility strategy.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532495528
##########
examples/build_tests/src/library.c:
##########
@@ -0,0 +1,18 @@
+// 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.
+
+#include "nanoarrow/nanoarrow.h"
Review Comment:
There's a `ArrowNanoarrowCheckRuntime()` that (checks the include version vs. the version reported by `ArrowNanoarrowVersion()` that would probably be useful here.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532606182
##########
cmake/config.cmake.in:
##########
@@ -0,0 +1,28 @@
+# 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.
+
+
+@PACKAGE_INIT@
+
+cmake_minimum_required(VERSION @CMAKE_MINIMUM_REQUIRED_VERSION@)
Review Comment:
Everything here runs with 3.12, so I'll just set it to that (older than that requires setting policies, which would clutter the example).
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532828285
##########
CMakeLists.txt:
##########
@@ -154,13 +154,48 @@ else()
endif()
endif()
- install(TARGETS nanoarrow DESTINATION lib)
+ install(TARGETS nanoarrow
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
+ install(TARGETS coverage_config
+ DESTINATION lib
+ EXPORT nanoarrow-exports)
Review Comment:
Got it! Feel free to fix that (or leave this and I'll fix it later)
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532410191
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
+option(FIND_NANOARROW "Find an existing nanoarrow" ON)
+
+# When adding nanoarrow's CMake directory to a CMake project that contains a library
+# intended for use by others, set NANOARROW_NAMESPACE to rename symbols in the
+# nanoarrow library such that they do not collide with other libraries that may also
+# link to their own copy of nanoarrow. You may wish to include the namespace only
+# on release builds, since the namespace implementation obscures inline help
+# available in many text editors.
+set(NANOARROW_NAMESPACE "ExampleCmakeMinimal")
+
+if(FIND_NANOARROW)
+ find_package(nanoarrow 0.5.0 REQUIRED)
Review Comment:
The answer here is going to depend on what we decide on in [the thread above](https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1531340192).
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532423412
##########
CMakeLists.txt:
##########
Review Comment:
Sorry, I'm not seeing what line you're referring to here. What do you mean by a two-step process?
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1532411407
##########
examples/build_tests/build.sh:
##########
Review Comment:
Yup good call. I'll add 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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2013313010
Whoops, I should have waited for CI. Pending green CI it's good to go!
--
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
Re: [PR] fix: Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "vyasr (via GitHub)" <gi...@apache.org>.
vyasr commented on PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#issuecomment-2013557752
My bad, forgot to update the CI job.
--
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
Re: [PR] Make build and install dirs proper CMake package, fix C++ header inclusion, and add proper tests [arrow-nanoarrow]
Posted by "assignUser (via GitHub)" <gi...@apache.org>.
assignUser commented on code in PR #406:
URL: https://github.com/apache/arrow-nanoarrow/pull/406#discussion_r1531445501
##########
examples/build_tests/CMakeLists.txt:
##########
@@ -0,0 +1,50 @@
+# 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.
+
+message(STATUS "Building using CMake version: ${CMAKE_VERSION}")
+# Setting minimum to 3.12 for CMP0074
+cmake_minimum_required(VERSION 3.25)
+
+project(NanoArrowExampleCMakeMinimal LANGUAGES C CXX)
+
+option(FIND_NANOARROW "Find an existing nanoarrow" ON)
+
+# When adding nanoarrow's CMake directory to a CMake project that contains a library
+# intended for use by others, set NANOARROW_NAMESPACE to rename symbols in the
+# nanoarrow library such that they do not collide with other libraries that may also
+# link to their own copy of nanoarrow. You may wish to include the namespace only
+# on release builds, since the namespace implementation obscures inline help
+# available in many text editors.
+set(NANOARROW_NAMESPACE "ExampleCmakeMinimal")
+
+if(FIND_NANOARROW)
+ find_package(nanoarrow 0.5.0 REQUIRED)
Review Comment:
Depending on the [find-module/config](https://cmake.org/cmake/help/latest/module/CMakePackageConfigHelpers.html#generating-a-package-version-file) this usually means a minimum version, so it doesn't necessarily need to be updated. If you know the code used has a required minimum version it's good practice to add it in the find call to avoid confusing compile errors.
--
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