nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585770203
##########
File path: r/configure
##########
@@ -26,13 +26,14 @@
# R CMD INSTALL --configure-vars='INCLUDE_DIR=/.../include LIB_DIR=/.../lib'
# Library settings
-PKG_CONFIG_NAME="arrow parquet arrow-dataset"
+PKG_CONFIG_NAME="arrow"
PKG_DEB_NAME="(unsuppored)"
PKG_RPM_NAME="(unsuppored)"
PKG_BREW_NAME="apache-arrow"
PKG_TEST_HEADER="<arrow/api.h>"
-# These must be the same order as $(pkg-config --libs arrow-dataset)
-PKG_LIBS="-larrow_dataset -lparquet -larrow"
+PKG_LIBS="-larrow"
Review comment:
I don't understand the specifics but apparently the order of the `-l`
libs matters. So we should make sure below that we build up `$PKG_LIBS` so that
the order is correct (and note in comments that the order matters).
##########
File path: r/configure
##########
@@ -182,15 +187,33 @@ if [ $? -eq 0 ] || [ "$UNAME" = "Darwin" ]; then
# Always build with arrow on macOS
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ARROW"
# Check for features
- LIB_DIR=`echo $PKG_LIBS | sed -e 's/ -l.*//' | sed -e 's/^-L//'`
- grep 'set(ARROW_S3 "ON")' $LIB_DIR/cmake/arrow/ArrowOptions.cmake >/dev/null
2>&1
+ LIB_DIR=`echo $PKG_DIRS | sed -e 's/^-L//'`
+ ARROW_OPTS_CMAKE="$LIB_DIR/cmake/arrow/ArrowOptions.cmake"
+ # Check for Arrow Dataset subcomponent
Review comment:
Per comment above, I think we should prepend -lparquet before
-larrow_dataset
##########
File path: r/configure
##########
@@ -69,19 +70,22 @@ fi
if [ "$INCLUDE_DIR" ] || [ "$LIB_DIR" ]; then
echo "*** Using INCLUDE_DIR/LIB_DIR"
PKG_CFLAGS="-I$INCLUDE_DIR $PKG_CFLAGS"
- PKG_LIBS="-L$LIB_DIR $PKG_LIBS"
+ PKG_DIRS="-L$LIB_DIR $PKG_DIRS"
else
# Use pkg-config if available and allowed
pkg-config --version >/dev/null 2>&1
if [ "$ARROW_USE_PKG_CONFIG" != "false" ] && [ $? -eq 0 ]; then
PKGCONFIG_CFLAGS=`pkg-config --cflags --silence-errors ${PKG_CONFIG_NAME}`
- PKGCONFIG_LIBS=`pkg-config --libs --silence-errors ${PKG_CONFIG_NAME}`
+ PKGCONFIG_LIBS=`pkg-config --libs-only-l --silence-errors
${PKG_CONFIG_NAME}`
+ PKGCONFIG_DIRS=`pkg-config --libs-only-L --silence-errors
${PKG_CONFIG_NAME}`
+ # TODO: what about --libs-only-other?
Review comment:
FYI, some of this logic (including stuff further down) should change
once https://issues.apache.org/jira/browse/ARROW-6312 happens and the pkgconfig
files declare what -l libs they require, but this should be ok for now.
##########
File path: r/configure
##########
@@ -69,19 +70,22 @@ fi
if [ "$INCLUDE_DIR" ] || [ "$LIB_DIR" ]; then
echo "*** Using INCLUDE_DIR/LIB_DIR"
PKG_CFLAGS="-I$INCLUDE_DIR $PKG_CFLAGS"
- PKG_LIBS="-L$LIB_DIR $PKG_LIBS"
+ PKG_DIRS="-L$LIB_DIR $PKG_DIRS"
Review comment:
You've defined `$PKG_DIRS` to be empty above, so there's nothing to
append here.
```suggestion
PKG_DIRS="-L$LIB_DIR"
```
##########
File path: r/configure
##########
@@ -145,7 +150,7 @@ else
# TODO: what about non-bundled deps?
BUNDLED_LIBS=`cd $LIB_DIR && ls *.a`
BUNDLED_LIBS=`echo $BUNDLED_LIBS | sed -E "s/lib(.*)\.a/-l\1/" | sed
-e "s/\\.a lib/ -l/g"`
- PKG_LIBS="-L$(pwd)/$LIB_DIR $PKG_LIBS $BUNDLED_LIBS"
+ PKG_DIRS="-L$(pwd)/$LIB_DIR $PKG_DIRS"
Review comment:
How would `$PKG_DIRS` be set before here?
Also `$(pwd)` presumably is a bashism we can't use, right? No idea why that
hasn't caused us problems before (we even test this on CI).
##########
File path: r/configure
##########
@@ -26,13 +26,14 @@
# R CMD INSTALL --configure-vars='INCLUDE_DIR=/.../include LIB_DIR=/.../lib'
# Library settings
-PKG_CONFIG_NAME="arrow parquet arrow-dataset"
+PKG_CONFIG_NAME="arrow"
PKG_DEB_NAME="(unsuppored)"
PKG_RPM_NAME="(unsuppored)"
PKG_BREW_NAME="apache-arrow"
PKG_TEST_HEADER="<arrow/api.h>"
-# These must be the same order as $(pkg-config --libs arrow-dataset)
-PKG_LIBS="-larrow_dataset -lparquet -larrow"
+PKG_LIBS="-larrow"
+PKG_DIRS=""
Review comment:
Reading through the code, I'm not sure this is needed. I think in every
case, you're defining $PKG_DIRS and never want to append an empty string to it.
##########
File path: r/configure
##########
@@ -182,15 +187,33 @@ if [ $? -eq 0 ] || [ "$UNAME" = "Darwin" ]; then
# Always build with arrow on macOS
PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_ARROW"
# Check for features
- LIB_DIR=`echo $PKG_LIBS | sed -e 's/ -l.*//' | sed -e 's/^-L//'`
- grep 'set(ARROW_S3 "ON")' $LIB_DIR/cmake/arrow/ArrowOptions.cmake >/dev/null
2>&1
+ LIB_DIR=`echo $PKG_DIRS | sed -e 's/^-L//'`
+ ARROW_OPTS_CMAKE="$LIB_DIR/cmake/arrow/ArrowOptions.cmake"
+ # Check for Arrow Dataset subcomponent
+ grep 'set(ARROW_DATASET "ON")' $ARROW_OPTS_CMAKE >/dev/null 2>&1
+ if [ $? -eq 0 ]; then
+ PKG_CFLAGS="$PKG_CFLAGS -DARROW_R_WITH_DATASET"
+ PKG_LIBS="-larrow_dataset $PKG_LIBS"
+ # TODO: what if arrow-dataset has a different -L location than arrow?
Review comment:
Not supported. Replace this comment with a comment that just notes we
assume that all arrow libs are in the same directory (which in practice they
always will be).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
