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