You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/08/29 15:00:21 UTC
[GitHub] [arrow-nanoarrow] paleolimbot opened a new pull request, #38: [C] Add namespacing option
paleolimbot opened a new pull request, #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38
Fixes #28 (will be documented in a PR for #21).
This is far outside my expertise...I chose an implementation between H3 ( https://github.com/uber/h3/blob/master/src/h3lib/include/h3api.h.in#L27-L50 ) and xxHash ( https://github.com/Cyan4973/xxHash/blob/c4359b17db082888fdc18371eba918b957a6baaa/xxhash.h#L210-L225 ) but am open to suggestions!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot merged pull request #38: [C] Add namespacing option
Posted by GitBox <gi...@apache.org>.
paleolimbot merged PR #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] pitrou commented on a diff in pull request #38: [C] Add namespacing option
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38#discussion_r958486611
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -24,6 +24,81 @@
#include "nanoarrow_types.h"
+// If using CMake, optionally pass -DNANOARROW_NAMESPACE=MyNamespace which will set this
+// define in build_id.h. If not, you can optionally #define NANOARROW_NAMESPACE
+// MyNamespace here.
+
+// This section remaps the non-prefixed symbols to the prefixed symbols so that
+// code written against this build can be used independent of the value of
+// NANOARROW_NAMESPACE.
+#ifdef NANOARROW_NAMESPACE
+#define NANOARROW_CAT(A, B) A##B
+#define NANOARROW_SYMBOL(A, B) NANOARROW_CAT(A, B)
Review Comment:
`#undef` these at the end?
##########
.github/workflows/build-and-test.yaml:
##########
@@ -112,6 +113,16 @@ jobs:
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug -DGTest_DIR=`pwd`/../dist/lib/cmake/GTest -DArrow_DIR=`pwd`/../dist/lib/cmake/arrow -DArrowTesting_DIR=`pwd`/../dist/lib/cmake/arrow -DNANOARROW_CODE_COVERAGE=ON -DNANOARROW_BUILD_TESTS=ON
cmake --build .
+
+ - name: Build nanoarrow (namespaced)
+ if: matrix.config.label == 'namespaced-build'
+ run: |
+ export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:`pwd`/dist/lib
+ sudo ldconfig
+ mkdir build
+ cd build
+ cmake .. -DCMAKE_BUILD_TYPE=Debug -DGTest_DIR=`pwd`/../dist/lib/cmake/GTest -DArrow_DIR=`pwd`/../dist/lib/cmake/arrow -DArrowTesting_DIR=`pwd`/../dist/lib/cmake/arrow -DNANOARROW_CODE_COVERAGE=ON -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_NAMESPACE=SomeUserNamespace
+ cmake --build .
Review Comment:
Should you add a test step that inspects the produced library and checks that no symbol is non-namespaced?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] codecov-commenter commented on pull request #38: [C] Add namespacing option
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38#issuecomment-1233211138
# [Codecov](https://codecov.io/gh/apache/arrow-nanoarrow/pull/38?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#38](https://codecov.io/gh/apache/arrow-nanoarrow/pull/38?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2b74623) into [main](https://codecov.io/gh/apache/arrow-nanoarrow/commit/d3989298af067b9bedd66d93ef1c0bd84e94c42d?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d398929) will **not change** coverage.
> The diff coverage is `100.00%`.
> :exclamation: Current head 2b74623 differs from pull request most recent head 66207d1. Consider uploading reports for the commit 66207d1 to get more accurate results
```diff
@@ Coverage Diff @@
## main #38 +/- ##
=======================================
Coverage 92.00% 92.00%
=======================================
Files 6 6
Lines 1301 1301
Branches 47 47
=======================================
Hits 1197 1197
Misses 72 72
Partials 32 32
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-nanoarrow/pull/38?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/nanoarrow/array.c](https://codecov.io/gh/apache/arrow-nanoarrow/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9hcnJheS5j) | `90.39% <100.00%> (ø)` | |
| [src/nanoarrow/schema.c](https://codecov.io/gh/apache/arrow-nanoarrow/pull/38/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL25hbm9hcnJvdy9zY2hlbWEuYw==) | `91.74% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #38: [C] Add namespacing option
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on code in PR #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38#discussion_r959895686
##########
.github/workflows/build-and-test.yaml:
##########
@@ -112,6 +113,16 @@ jobs:
cd build
cmake .. -DCMAKE_BUILD_TYPE=Debug -DGTest_DIR=`pwd`/../dist/lib/cmake/GTest -DArrow_DIR=`pwd`/../dist/lib/cmake/arrow -DArrowTesting_DIR=`pwd`/../dist/lib/cmake/arrow -DNANOARROW_CODE_COVERAGE=ON -DNANOARROW_BUILD_TESTS=ON
cmake --build .
+
+ - name: Build nanoarrow (namespaced)
+ if: matrix.config.label == 'namespaced-build'
+ run: |
+ export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:`pwd`/dist/lib
+ sudo ldconfig
+ mkdir build
+ cd build
+ cmake .. -DCMAKE_BUILD_TYPE=Debug -DGTest_DIR=`pwd`/../dist/lib/cmake/GTest -DArrow_DIR=`pwd`/../dist/lib/cmake/arrow -DArrowTesting_DIR=`pwd`/../dist/lib/cmake/arrow -DNANOARROW_CODE_COVERAGE=ON -DNANOARROW_BUILD_TESTS=ON -DNANOARROW_NAMESPACE=SomeUserNamespace
+ cmake --build .
Review Comment:
Done! (Also this highlighted a number of symbols I'd still defined by accident). Is there any check beyond the output of `nm --extern-only build/libnanoarrow.a` that I need to 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
[GitHub] [arrow-nanoarrow] paleolimbot commented on a diff in pull request #38: [C] Add namespacing option
Posted by GitBox <gi...@apache.org>.
paleolimbot commented on code in PR #38:
URL: https://github.com/apache/arrow-nanoarrow/pull/38#discussion_r959894571
##########
src/nanoarrow/nanoarrow.h:
##########
@@ -24,6 +24,81 @@
#include "nanoarrow_types.h"
+// If using CMake, optionally pass -DNANOARROW_NAMESPACE=MyNamespace which will set this
+// define in build_id.h. If not, you can optionally #define NANOARROW_NAMESPACE
+// MyNamespace here.
+
+// This section remaps the non-prefixed symbols to the prefixed symbols so that
+// code written against this build can be used independent of the value of
+// NANOARROW_NAMESPACE.
+#ifdef NANOARROW_NAMESPACE
+#define NANOARROW_CAT(A, B) A##B
+#define NANOARROW_SYMBOL(A, B) NANOARROW_CAT(A, B)
Review Comment:
I think I need them to persist in order for the macro expansion to work (or at least, I can't figure out how to make the build pass if I undefine them (xxHash doesn't seem to undefine them, either).
--
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