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 2024/06/08 19:27:40 UTC
[PR] Consolidated IPC extension into main project [arrow-nanoarrow]
WillAyd opened a new pull request, #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511
(no comment)
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#issuecomment-2157016588
Taking this on is heroic! All of the hard things here are probably because I designed it badly when I first wrote the IPC extension 😬 .
Just a note that some of these CI jobs you can run locally, too, in case you didn't know:
- `docker compose run --rm docs`
- `docker compose run --rm coverage`
- `export NANOARROW_PLATFORM=ubuntu && docker compose run --rm verify`
I started moving the bundling out of CMake here https://github.com/apache/arrow-nanoarrow/pull/508 and should finish that this week (which may help here if bundling is causing problems, which it usually does 🙂 )
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#issuecomment-2157177778
## [Codecov](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/511?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.61%. Comparing base [(`8894ebf`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/8894ebf42fa1f0c5aa088469e1a2149f6d824f2e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`1a934d5`)](https://app.codecov.io/gh/apache/arrow-nanoarrow/commit/1a934d5a6603211b381d3a9e3db94d52ad3096f7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 14 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #511 +/- ##
==========================================
- Coverage 89.21% 88.61% -0.60%
==========================================
Files 90 90
Lines 16294 16554 +260
==========================================
+ Hits 14536 14669 +133
- Misses 1758 1885 +127
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/arrow-nanoarrow/pull/511?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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#issuecomment-2157263636
I don't believe the two test failures are related to this change - seems like they showed up in the weekly verification run
https://github.com/apache/arrow-nanoarrow/actions/runs/9432588619
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot merged PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#discussion_r1633504181
##########
CMakeLists.txt:
##########
@@ -310,10 +433,82 @@ if(NANOARROW_BUILD_TESTS)
gtest_discover_tests(nanoarrow_hpp_test DISCOVERY_TIMEOUT 10)
gtest_discover_tests(nanoarrow_testing_test DISCOVERY_TIMEOUT 10)
gtest_discover_tests(c_data_integration_test DISCOVERY_TIMEOUT 10)
+
+ if(NANOARROW_IPC)
+
+ # zlib to decode gzipped integration testing JSON files
+ # We don't use Arrow C++ for this because building Arrow C++ with zlib
+ # is not trivial on Windows.
+ find_package(ZLIB)
+ if(NOT ZLIB_FOUND)
+ # Wrapper around FetchContent that better isolates the zlib CMakeLists.txt
+ message(STATUS "Using FetchContent to build a static zlib")
+ add_subdirectory(thirdparty/zlib)
+ endif()
+
+ enable_testing()
+
+ add_executable(nanoarrow_ipc_decoder_test src/nanoarrow/nanoarrow_ipc_decoder_test.cc)
+ add_executable(nanoarrow_ipc_reader_test src/nanoarrow/nanoarrow_ipc_reader_test.cc)
+ add_executable(nanoarrow_ipc_files_test src/nanoarrow/nanoarrow_ipc_files_test.cc)
+ add_executable(nanoarrow_ipc_hpp_test src/nanoarrow/nanoarrow_ipc_hpp_test.cc)
+
+ if(NANOARROW_CODE_COVERAGE)
+ target_compile_options(ipc_coverage_config INTERFACE -O0 -g --coverage)
+ target_link_options(ipc_coverage_config INTERFACE --coverage)
+ target_link_libraries(nanoarrow_ipc PRIVATE ipc_coverage_config)
+ endif()
+ target_link_libraries(nanoarrow_ipc_decoder_test
+ nanoarrow_ipc
+ nanoarrow
+ flatccrt
+ ${NANOARROW_ARROW_TARGET}
+ gtest_main
+ ipc_coverage_config)
+ target_include_directories(nanoarrow_ipc_decoder_test
+ PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
+ $<INSTALL_INTERFACE:include>)
Review Comment:
Thanks for catching this - mistake on my end. I got confused while trying to get tests to find the proper include paths when bundled, but the actual fix needed was elsewhere.
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "paleolimbot (via GitHub)" <gi...@apache.org>.
paleolimbot commented on code in PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#discussion_r1633352953
##########
ci/scripts/coverage.sh:
##########
@@ -81,11 +81,12 @@ function main() {
# Build + run tests with gcov for IPC extension
show_header "Build + test nanoarrow_ipc"
- mkdir "${SANDBOX_DIR}/nanoarrow_ipc"
- pushd "${SANDBOX_DIR}/nanoarrow_ipc"
+ rm -rf "${SANDBOX_DIR}/nanoarrow"
+ mkdir "${SANDBOX_DIR}/nanoarrow"
+ pushd "${SANDBOX_DIR}/nanoarrow"
- cmake "${TARGET_NANOARROW_DIR}/extensions/nanoarrow_ipc" \
- -DNANOARROW_IPC_BUILD_TESTS=ON -DNANOARROW_IPC_CODE_COVERAGE=ON
+ cmake "${TARGET_NANOARROW_DIR}" \
+ -DNANOARROW_IPC=ON -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_CODE_COVERAGE=ON
Review Comment:
Does this need to be separate (i.e, can we just add `NANOARROW_IPC=ON` to the main nanoarrow coverage call?)
##########
CMakeLists.txt:
##########
@@ -310,10 +433,82 @@ if(NANOARROW_BUILD_TESTS)
gtest_discover_tests(nanoarrow_hpp_test DISCOVERY_TIMEOUT 10)
gtest_discover_tests(nanoarrow_testing_test DISCOVERY_TIMEOUT 10)
gtest_discover_tests(c_data_integration_test DISCOVERY_TIMEOUT 10)
+
+ if(NANOARROW_IPC)
+
+ # zlib to decode gzipped integration testing JSON files
+ # We don't use Arrow C++ for this because building Arrow C++ with zlib
+ # is not trivial on Windows.
+ find_package(ZLIB)
+ if(NOT ZLIB_FOUND)
+ # Wrapper around FetchContent that better isolates the zlib CMakeLists.txt
+ message(STATUS "Using FetchContent to build a static zlib")
+ add_subdirectory(thirdparty/zlib)
+ endif()
+
+ enable_testing()
+
+ add_executable(nanoarrow_ipc_decoder_test src/nanoarrow/nanoarrow_ipc_decoder_test.cc)
+ add_executable(nanoarrow_ipc_reader_test src/nanoarrow/nanoarrow_ipc_reader_test.cc)
+ add_executable(nanoarrow_ipc_files_test src/nanoarrow/nanoarrow_ipc_files_test.cc)
+ add_executable(nanoarrow_ipc_hpp_test src/nanoarrow/nanoarrow_ipc_hpp_test.cc)
+
+ if(NANOARROW_CODE_COVERAGE)
+ target_compile_options(ipc_coverage_config INTERFACE -O0 -g --coverage)
+ target_link_options(ipc_coverage_config INTERFACE --coverage)
+ target_link_libraries(nanoarrow_ipc PRIVATE ipc_coverage_config)
+ endif()
+ target_link_libraries(nanoarrow_ipc_decoder_test
+ nanoarrow_ipc
+ nanoarrow
+ flatccrt
+ ${NANOARROW_ARROW_TARGET}
+ gtest_main
+ ipc_coverage_config)
+ target_include_directories(nanoarrow_ipc_decoder_test
+ PUBLIC $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/src>
+ $<INSTALL_INTERFACE:include>)
Review Comment:
Other tests defined in this CMakeLists.txt don't use these generator expressions...are they needed?
##########
.gitattributes:
##########
@@ -16,5 +16,4 @@
# under the License.
dist/* linguist-generated
-extensions/nanoarrow_ipc/thirdparty/* linguist-vendored
Review Comment:
Should this be changed to `thirdparty/*`?
##########
dev/release/verify-release-candidate.sh:
##########
@@ -246,17 +246,15 @@ test_c() {
test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON
show_header "Build and test C IPC extension"
- test_cmake_project build-ipc extensions/nanoarrow_ipc -DNANOARROW_IPC_BUILD_TESTS=ON
+ test_cmake_project build . -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_IPC=ON
Review Comment:
Does this need to be separate (i.e, can we just add `NANOARROW_IPC=ON` to the line above it?)
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#issuecomment-2159015303
Still think the windows CI failure might be unrelated - I don't see that as having run in #514
--
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] Consolidated IPC extension into main project [arrow-nanoarrow]
Posted by "WillAyd (via GitHub)" <gi...@apache.org>.
WillAyd commented on code in PR #511:
URL: https://github.com/apache/arrow-nanoarrow/pull/511#discussion_r1633505054
##########
ci/scripts/coverage.sh:
##########
@@ -81,11 +81,12 @@ function main() {
# Build + run tests with gcov for IPC extension
show_header "Build + test nanoarrow_ipc"
- mkdir "${SANDBOX_DIR}/nanoarrow_ipc"
- pushd "${SANDBOX_DIR}/nanoarrow_ipc"
+ rm -rf "${SANDBOX_DIR}/nanoarrow"
+ mkdir "${SANDBOX_DIR}/nanoarrow"
+ pushd "${SANDBOX_DIR}/nanoarrow"
- cmake "${TARGET_NANOARROW_DIR}/extensions/nanoarrow_ipc" \
- -DNANOARROW_IPC_BUILD_TESTS=ON -DNANOARROW_IPC_CODE_COVERAGE=ON
+ cmake "${TARGET_NANOARROW_DIR}" \
+ -DNANOARROW_IPC=ON -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_CODE_COVERAGE=ON
Review Comment:
Not at all - just kept as separate steps owing to the history, but happy to consolidate this into the existing "core" check
--
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