[GitHub] [arrow] kou commented on a diff in pull request #35147: GH-35140: [R] configure script needs LIB_DIR to find ArrowOptions.cmake

["kou (via GitHub)" <gi...@apache.org>]

kou commented on code in PR #35147:
URL: https://github.com/apache/arrow/pull/35147#discussion_r1174794249


##########
r/configure:
##########
@@ -252,6 +252,10 @@ echo "#include $PKG_TEST_HEADER" | ${TEST_CMD} >/dev/null 2>&1
 
 if [ $? -eq 0 ]; then
   # Check for features
+  if [ "$LIB_DIR" = "" ]; then
+    # In case LIB_DIR wasn't previously set, infer from PKG_DIRS
+    LIB_DIR=`echo $PKG_DIRS | sed -e 's/^-L//'`
+  fi

Review Comment:
   I think that the following approach (we must set `LIB_DIR` when `PKG_*` are set) is better than this approach:
   
   ```diff
   diff --git a/r/configure b/r/configure
   index 65528bdc3..f452fffb1 100755
   --- a/r/configure
   +++ b/r/configure
   @@ -126,10 +126,11 @@ else
        if [ "$UNAME" = "Darwin" ] && [ "$FORCE_BUNDLED_BUILD" != "true" ]; then
          if [ "$FORCE_AUTOBREW" != "true" ] && [ "`command -v brew`" ] && [ "`brew ls --versions ${PKG_BREW_NAME}`" != "" ]; then
            echo "*** Using Homebrew ${PKG_BREW_NAME}"
   -        BREWDIR=`brew --prefix`
   +        BREWDIR=`brew --prefix ${PKG_BREW_NAME}`
   +        LIB_DIR="$BREWDIR/lib"
            PKG_LIBS="-larrow -larrow_bundled_dependencies"
   -        PKG_DIRS="-L$BREWDIR/opt/$PKG_BREW_NAME/lib $PKG_DIRS"
   -        PKG_CFLAGS="-I$BREWDIR/opt/$PKG_BREW_NAME/include $PKG_CFLAGS"
   +        PKG_DIRS="-L$LIB_DIR $PKG_DIRS"
   +        PKG_CFLAGS="-I$BREWDIR/include $PKG_CFLAGS"
          else
            echo "*** Downloading ${PKG_BREW_NAME}"
            if [ -f "autobrew" ]; then
   @@ -144,7 +145,9 @@ else
            if [ $? -ne 0 ]; then
              echo "Failed to retrieve binary for ${PKG_BREW_NAME}"
            fi
   -        # autobrew sets `PKG_LIBS`, `PKG_DIRS`, and `PKG_CFLAGS`
   +        # autobrew sets `PKG_LIBS`, `PKG_DIRS`, `PKG_CFLAGS`, and `BREWDIR`
   +        # but `LIB_DIR` isn't set.
   +        LIB_DIR="$BREWDIR/lib"
          fi
        else
          if [ "${NOT_CRAN}" = "true" ]; then
   ```



##########
dev/tasks/conda-recipes/r-arrow/build.sh:
##########
@@ -14,3 +14,5 @@ fi
 
 # ${R_ARGS} necessary to support cross-compilation
 ${R} CMD INSTALL --build r/. ${R_ARGS}
+# Ensure that features are enabled in the R build (feel free to add others)
+${R} -s -e 'library(arrow); stopifnot(arrow_with_dataset(), arrow_with_parquet(), arrow_with_s3())'

Review Comment:
   @h-vetinari Could you review conda part?



-- 
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