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 2021/03/01 23:20:19 UTC
[GitHub] [arrow] ianmcook opened a new pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
ianmcook opened a new pull request #9610:
URL: https://github.com/apache/arrow/pull/9610
Not implemented for Windows yet
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586741670
##########
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:
Yeah, I followed that. I just meant that I don't think it's possible for `PKG_DIRS` to be set to anything other than "" before here, so you don't need to append it do you, just set `PKG_DIRS="-L$(pwd)/$LIB_DIR"`?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791841345
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587917624
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Oops, need to append not overwrite. Fixed in 318371fbaa07f1b6fcdf71f58d7d5460a931da13
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791158004
Revision: 6eda5896c638ca1c7af577c02ec56bd802a2fe42
Submitted crossbow builds: [ursacomputing/crossbow @ actions-185](https://github.com/ursacomputing/crossbow/branches/all?query=actions-185)
|Task|Status|
|----|------|
|conda-linux-gcc-py36-cpu-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-linux-gcc-py36-cpu-r36)|
|conda-linux-gcc-py37-cpu-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-linux-gcc-py37-cpu-r40)|
|conda-osx-clang-py36-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-osx-clang-py36-r36)|
|conda-osx-clang-py37-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-osx-clang-py37-r40)|
|conda-win-vs2017-py36-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-win-vs2017-py36-r36)|
|conda-win-vs2017-py37-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-conda-win-vs2017-py37-r40)|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-185-github-homebrew-r-autobrew)|
|test-r-linux-as-cran|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-185-github-test-r-linux-as-cran)|
|test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rhub-ubuntu-gcc-release)|
|test-r-rocker-r-base-latest|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rocker-r-base-latest)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos7-devtoolset-8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rstudio-r-base-3.6-centos7-devtoolset-8)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-rstudio-r-base-3.6-opensuse42)|
|test-r-version-compatibility|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-185-github-test-r-version-compatibility)|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-185-github-test-r-versions)|
|test-r-without-dataset-parquet-s3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-r-without-dataset-parquet-s3)|
|test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-185-azure-test-ubuntu-18.04-r-sanitizer)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791154662
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586763543
##########
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:
Correction: the order was wrong, but this was corrected in b7cdbe26ef6ab9496b2a1f810e84469db9620fe1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r589618534
##########
File path: dev/tasks/conda-recipes/r-arrow/configure.win
##########
@@ -2,7 +2,7 @@
set -euxo pipefail
-echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_S3" > src/Makevars.win
+echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET -DARROW_R_WITH_S3" > src/Makevars.win
Review comment:
Merged in https://github.com/conda-forge/r-arrow-feedstock/commit/ed01e9badf1cd831d5201d6f9b35dcf01c873cf6
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586783964
##########
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:
Fixed in b7cdbe26ef6ab9496b2a1f810e84469db9620fe1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587998671
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Ok, thanks. I tried a few different things just for kicks, then used `-e` per your suggestion in 9479c6ce079c7230b0e290dcb5658436d1d62fde
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586564929
##########
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:
Yep, the order in which the code below prepends to `PKG_LIBS` matches the output of `pkg-config --libs-only-l arrow-dataset` (at least on my Mac) which is:
```
-larrow_dataset -lparquet -larrow
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587557839
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Done in 6918441bba2b0ca09f2103eaf5631adc0a076a6c
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791439409
The environment variables are being set in the container now, and Arrow is building with Dataset and Parquet off, but S3 is remaining on despite the environment variable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588782921
##########
File path: dev/tasks/conda-recipes/r-arrow/configure.win
##########
@@ -2,7 +2,7 @@
set -euxo pipefail
-echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_S3" > src/Makevars.win
+echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET -DARROW_R_WITH_S3" > src/Makevars.win
Review comment:
Done https://github.com/conda-forge/r-arrow-feedstock/pull/30
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588442497
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-without-dataset-parquet-s3:
Review comment:
I actually liked the "minimal" name, matches the cpp minimal build example we test. If you're worried about confusion with the `LIBARROW_MINIMAL` env var (I'm not personally), we could try to make that var be truly minimal. I think currently we only ever check for whether it is set to `false`, to mean "build all the compression libs etc.", but we don't check if it is `true`. So we could make `true` be truly minimal, without these optional features, and the default (unset) means the current default of all features on but not compression and S3.
On an unrelated note, you should add this crossbow job to the `r` and `nightly` groups so that it gets run regularly.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585149473
##########
File path: r/tests/testthat/latin1.R
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-x <- iconv("Veitingasta�ir", to = "latin1")
+x <- iconv("Veitingasta�ir", to = "latin1")
Review comment:
Yeah you need to open this in a text editor that won't reencode to UTF-8 (`vim` may be suited for this), or otherwise set the encoding in the editor to latin1 (or whatever its official name is).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586648630
##########
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:
Cool, thanks for pointing that out.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588692513
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-without-dataset-parquet-s3:
Review comment:
> S3 is on because linuxlibs.R is turning it on: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=1741&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=24
>
> I think you'll find the issue pretty quickly but LMK if you need a pointer.
Looks like `configure` is setting `LIBARROW_MINIMAL=false` because `NOT_CRAN` is `true`:
```sh
if [ "${LIBARROW_MINIMAL}" = "" ] && [ "${NOT_CRAN}" = "true" ]; then
LIBARROW_MINIMAL=false; export LIBARROW_MINIMAL
fi
```
and then `linuxlibs.r` sets `arrow_s3` to `TRUE`:
```r
arrow_s3 <- toupper(Sys.getenv("ARROW_S3")) == "ON" || tolower(Sys.getenv("LIBARROW_MINIMAL")) == "false"
```
I will try to solve this by explicitly setting `LIBARROW_MINIMAL=true` in this build.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791851303
Revision: 96e91046ebcc2cd83289c63fe4f78c8e7841d40f
Submitted crossbow builds: [ursacomputing/crossbow @ actions-198](https://github.com/ursacomputing/crossbow/branches/all?query=actions-198)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-198-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790966208
Revision: 6f70c9911262d5d956c7f4ded49647269af848d9
Submitted crossbow builds: [ursacomputing/crossbow @ actions-175](https://github.com/ursacomputing/crossbow/branches/all?query=actions-175)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-175-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790925316
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587550424
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
What is the relationship of this script (`r/tools/autobrew`) to https://github.com/autobrew/scripts/blob/master/apache-arrow?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r623263639
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
Turns out we also needed to add `-DARROW_R_WITH_PARQUET` and `-DARROW_R_WITH_DATASET` to the cflags in ttps://github.com/autobrew/scripts/blob/master/apache-arrow (ARROW-12604)
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
Turns out we also needed to add `-DARROW_R_WITH_PARQUET` and `-DARROW_R_WITH_DATASET` to the cflags in https://github.com/autobrew/scripts/blob/master/apache-arrow (ARROW-12604)
--
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791839980
> Sorry, I just merged the dataset mutate() PR which added a cpp function. Can you please rebase and update the annotation on that function? Otherwise this is looking good, will merge.
Done
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791103746
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587961901
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Ah yeah, I think it's the same problem--note (for comparison) that `R_ORG` et al. are in the .env file, and they're environment variables for docker-compose, not passed into the docker container.
The fact that `ARROW_R_DEV` is in the env file, and then passed through in docker-compose.yml to the build environments, may just be due to my ignorance of all of this at the time I added 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585150504
##########
File path: r/tests/testthat/latin1.R
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-x <- iconv("Veitingasta�ir", to = "latin1")
+x <- iconv("Veitingasta�ir", to = "latin1")
Review comment:
cmon VS Code!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586568597
##########
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:
I thought so too at first, but when I run
```
pkg-config --libs-only-l arrow-dataset
```
I get `-larrow_dataset` first and `-lparquet` second.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586783164
##########
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:
I confirmed this. Removed in edbfb7e3732cf63e63ba2a7380771a2c56e722f2
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586785240
##########
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:
Oh, gotcha. I just went through and checked the control flow more carefully, and I agree—`PKG_DIRS` will never be set here. I removed it in 31bd5e9c12023b793a7ef30d6d14f2290698ffc1
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585147323
##########
File path: r/tests/testthat/test-python-flight.R
##########
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
+skip("Temporarily skip this")
Review comment:
Ah shoot, sorry, I wasn't supposed to commit that 😂
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson closed pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson closed pull request #9610:
URL: https://github.com/apache/arrow/pull/9610
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791095801
Revision: 27c514cb1b14e7125e8ba50714c6480c93dc231a
Submitted crossbow builds: [ursacomputing/crossbow @ actions-181](https://github.com/ursacomputing/crossbow/branches/all?query=actions-181)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-181-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791036468
Revision: 318371fbaa07f1b6fcdf71f58d7d5460a931da13
Submitted crossbow builds: [ursacomputing/crossbow @ actions-177](https://github.com/ursacomputing/crossbow/branches/all?query=actions-177)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-177-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587841807
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-minimal-build:
Review comment:
But it is minimal in the sense that the cpp minimal build in here is. I think it's fine.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588696628
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-without-dataset-parquet-s3:
Review comment:
> I actually liked the "minimal" name, matches the cpp minimal build example we test. If you're worried about confusion with the `LIBARROW_MINIMAL` env var (I'm not personally), we could try to make that var be truly minimal. I think currently we only ever check for whether it is set to `false`, to mean "build all the compression libs etc.", but we don't check if it is `true`. So we could make `true` be truly minimal, without these optional features, and the default (unset) means the current default of all features on but not compression and S3.
>
> On an unrelated note, you should add this crossbow job to the `r` and `nightly` groups so that it gets run regularly.
For now I think I'll just set `LIBARROW_MINIMAL=true` so the "minimal" name is not misleading. Later we might consider whether an explicit `LIBARROW_MINIMAL=true` should toggle Dataset and Parquet off.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588021512
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
The build looks good after 9479c6c:
https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-184-azure-test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790887608
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587526925
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Gotcha. For now, I'll remove the conditional in the example and I'll also change the `\donttest` to `\dontrun`. That way the example checks will pass on Solaris.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791854899
Revision: 1b1680f2e8b1cc05f19ed460905db2b349d6d1e3
Submitted crossbow builds: [ursacomputing/crossbow @ actions-199](https://github.com/ursacomputing/crossbow/branches/all?query=actions-199)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-minimal-build)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos7-devtoolset-8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-rstudio-r-base-3.6-centos7-devtoolset-8)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-199-azure-test-r-rstudio-r-base-3.6-opensuse42)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586750914
##########
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:
D'oh—I switched from appending to prepending and forgot to change the order 😳 Thank you for catching that.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook removed a comment on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook removed a comment on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791139873
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587917115
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Got it, thanks. Let's see if it works to add them to `.env` (066ce065124cf7dcaa08225cae83177594da3965) and if not then I'll do it with `-e`.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586743366
##########
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:
Sorry, my note was ambiguous. The order should be `-larrow_dataset -lparquet`, correct. But if we're prepending them one at a time, we should first prepend parquet, then arrow_dataset. If I'm following correctly, the code here first does if ARROW_DATASET, do `PKG_LIBS="-larrow_dataset $PKG_LIBS"`, then if ARROW_PARQUET, do `PKG_LIBS="-lparquet $PKG_LIBS"`, which would result in `-lparquet -larrow_dataset`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791155609
@github-actions crossbow submit test-r-without-dataset-parquet-s3
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586084391
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
I created ARROW-11849 for us to do that after this feature makes it to CRAN
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587609001
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Thanks, please also remove the conditionals from the other examples. I don't feel strongly about `\donttest` vs `\dontrun` (CRAN complains about dontrun examples on initial submission, which is why they were switched to donttest in the first place, but that's not our concern now) so that's fine, but FWIW `\donttest` is safe on Solaris: [we currently "pass" Solaris checks](https://www.r-project.org/nosvn/R.check/r-patched-solaris-x86/arrow-00check.html) with the fake arrow-without-arrow build.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791158165
Revision: 6eda5896c638ca1c7af577c02ec56bd802a2fe42
Submitted crossbow builds: [ursacomputing/crossbow @ actions-186](https://github.com/ursacomputing/crossbow/branches/all?query=actions-186)
|Task|Status|
|----|------|
|test-r-without-dataset-parquet-s3|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-186-azure-test-r-without-dataset-parquet-s3)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790693193
> Oh, one other thing: in `inst/build_arrow_static/sh`, you'll want to make ARROW_PARQUET and ARROW_DATASET configurable by env var but default ON, just like ARROW_JEMALLOC. That way, we can make a CI job that sets them to OFF in the env so we can test this.
Done in 8c5b235dddac47833b229323b4a216049eae5fea
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585147423
##########
File path: r/tests/testthat/latin1.R
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-x <- iconv("Veitingasta�ir", to = "latin1")
+x <- iconv("Veitingasta�ir", to = "latin1")
Review comment:
Whoa, no
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791722491
Revision: e46ce97d204950e7f9088abd710c0f009522a816
Submitted crossbow builds: [ursacomputing/crossbow @ actions-193](https://github.com/ursacomputing/crossbow/branches/all?query=actions-193)
|Task|Status|
|----|------|
|conda-linux-gcc-py36-cpu-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-linux-gcc-py36-cpu-r36)|
|conda-linux-gcc-py37-cpu-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-linux-gcc-py37-cpu-r40)|
|conda-osx-clang-py36-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-osx-clang-py36-r36)|
|conda-osx-clang-py37-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-osx-clang-py37-r40)|
|conda-win-vs2017-py36-r36|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-win-vs2017-py36-r36)|
|conda-win-vs2017-py37-r40|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-conda-win-vs2017-py37-r40)|
|homebrew-r-autobrew|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-193-github-homebrew-r-autobrew)|
|test-r-linux-as-cran|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-193-github-test-r-linux-as-cran)|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-minimal-build)|
|test-r-rhub-ubuntu-gcc-release|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rhub-ubuntu-gcc-release)|
|test-r-rocker-r-base-latest|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rocker-r-base-latest)|
|test-r-rstudio-r-base-3.6-bionic|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rstudio-r-base-3.6-bionic)|
|test-r-rstudio-r-base-3.6-centos7-devtoolset-8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rstudio-r-base-3.6-centos7-devtoolset-8)|
|test-r-rstudio-r-base-3.6-centos8|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rstudio-r-base-3.6-centos8)|
|test-r-rstudio-r-base-3.6-opensuse15|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rstudio-r-base-3.6-opensuse15)|
|test-r-rstudio-r-base-3.6-opensuse42|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-r-rstudio-r-base-3.6-opensuse42)|
|test-r-version-compatibility|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-193-github-test-r-version-compatibility)|
|test-r-versions|[](https://github.com/ursacomputing/crossbow/actions?query=branch:actions-193-github-test-r-versions)|
|test-ubuntu-18.04-r-sanitizer|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-193-azure-test-ubuntu-18.04-r-sanitizer)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586587025
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
We don't need roxygen2 to be on CRAN in order to use it, the Rd it generates is valid.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791095464
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790954184
> @nealrichardson do you want `LIBARROW_MINIMAL` to toggle Dataset and Parquet? I assume no.
Correct, we don't want to change that meaning (yet).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588753808
##########
File path: dev/tasks/conda-recipes/r-arrow/configure.win
##########
@@ -2,7 +2,7 @@
set -euxo pipefail
-echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_S3" > src/Makevars.win
+echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET -DARROW_R_WITH_S3" > src/Makevars.win
Review comment:
Yes. You could make the change now in fact, nothing would be hurt by having extra -D flags on. https://github.com/conda-forge/r-arrow-feedstock (FYI @xhochy)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791854799
@github-actions crossbow submit test-r-rstudio* test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588816147
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-without-dataset-parquet-s3:
Review comment:
The `r` and `nightly` groups are defined with wildcards that will match the name I used for this test.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587918983
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
I think the only danger to appending to the .env file is that if you ran this locally, wouldn't it modify the file locally?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790929111
@nealrichardson do you want `LIBARROW_MINIMAL` to toggle Dataset and Parquet? I assume no.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586564929
##########
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:
Yep, the order in which the code below prepends to `PKG_LIBS` makes its value match the output of `pkg-config --libs-only-l arrow-dataset` (at least on my Mac) which is:
```
-larrow_dataset -lparquet -larrow
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586819763
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
We will need to remember to upstream this change when we release to CRAN
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586560582
##########
File path: r/data-raw/codegen.R
##########
@@ -194,10 +194,16 @@ arrow_exports_cpp <- glue::glue('
{feature_available("arrow")}
+{feature_available("dataset")}
Review comment:
Done in c8d390d
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586615537
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Right, roxygen2 does not run on CRAN, but I think we should avoid requiring R package contributors to use dev versions of devtools, roxygen2, etc.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791154080
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-789092250
Oh, one other thing: in `inst/build_arrow_static/sh`, you'll want to make ARROW_PARQUET and ARROW_DATASET configurable by env var but default ON, just like ARROW_JEMALLOC. That way, we can make a CI job that sets them to OFF in the env so we can test this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586639762
##########
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:
`$(command)` is apparently not a bashism: https://stackoverflow.com/questions/3789894/is-command-substitution-foo-bashism
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585139173
##########
File path: r/tests/testthat/test-python-flight.R
##########
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
+skip("Temporarily skip this")
Review comment:
Remove this (also why?)
##########
File path: r/data-raw/codegen.R
##########
@@ -194,10 +194,16 @@ arrow_exports_cpp <- glue::glue('
{feature_available("arrow")}
+{feature_available("dataset")}
Review comment:
It might be good to make these bits iterate over `features` rather than copy-paste, just to help with future features.
##########
File path: r/tests/testthat/test-dataset.R
##########
@@ -15,6 +15,8 @@
# specific language governing permissions and limitations
# under the License.
+skip_if_not_available("dataset")
Review comment:
You can skip a whole file like this? Cool.
##########
File path: r/tests/testthat/latin1.R
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-x <- iconv("Veitingasta�ir", to = "latin1")
+x <- iconv("Veitingasta�ir", to = "latin1")
Review comment:
Was this change intentional?
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Dev roxygen2 has an `@examplesIf` feature that might be better here. If we put the conditional in the example code, it may suggest to the user that they also need to check this every time, and generally they shouldn't.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791722178
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791117825
Revision: 9479c6ce079c7230b0e290dcb5658436d1d62fde
Submitted crossbow builds: [ursacomputing/crossbow @ actions-184](https://github.com/ursacomputing/crossbow/branches/all?query=actions-184)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-184-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587956206
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Ok, if folks might be running these commands locally then I can see how that would cause damage.
Looks like it didn't work anyhow. I will investigate.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588715419
##########
File path: dev/tasks/conda-recipes/r-arrow/configure.win
##########
@@ -2,7 +2,7 @@
set -euxo pipefail
-echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_S3" > src/Makevars.win
+echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET -DARROW_R_WITH_S3" > src/Makevars.win
Review comment:
Without this, the conda Windows builds were failing:
https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=1732&view=logs&j=4c86bc1b-1091-5192-4404-c74dfaad23e7&t=41795ef0-6501-5db4-3ad4-33c0cf085626&l=812
@nealrichardson is this another change that we will need to upstream at the next release?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586783690
##########
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:
Done in 3dc2061abc13949249e689c64cbdab810948ec65
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791139873
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586818018
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
I don't feel strongly about not requiring dev roxygen, but it's fine either way.
If you don't want to use examplesIf here, I think you can leave off the `if (arrow_with_dataset())` checks everywhere since it's already inside a `\donttest`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588715419
##########
File path: dev/tasks/conda-recipes/r-arrow/configure.win
##########
@@ -2,7 +2,7 @@
set -euxo pipefail
-echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_S3" > src/Makevars.win
+echo "PKG_CPPFLAGS=-DNDEBUG -I\"${LIBRARY_PREFIX}/include\" -I\"${PREFIX}/include\" -DARROW_R_WITH_ARROW -DARROW_R_WITH_PARQUET -DARROW_R_WITH_DATASET -DARROW_R_WITH_S3" > src/Makevars.win
Review comment:
@nealrichardson is this another change that we will need to upstream at the next release?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587761603
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
Done in 26753e4e0ed08739467588cfb3ddcfe09f5aebcc. I will change the `\dontrun`s back to `\donttest` (or remove them altogether if possible) when I implement `@examplesIf`, as noted in ARROW-11849
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586859667
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
It does but I think CRAN doesn't do `--run-donttest` everywhere. Anyway we won't be turning off these features on CRAN except for Solaris so I think it's fine, or worth the risk so as not to pollute the examples with potentially confusing (to users) wrapping.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook removed a comment on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook removed a comment on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-790887608
@github-actions crossbow submit -g r
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791036107
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587597922
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
It's based on an earlier version of Jeroen's autobrew, and it's closest now to how he actually builds the C++ dependencies. It looks like the new autobrew script is based on some artifacts that are bundled up in a separate process (see https://github.com/autobrew/bundler/blob/master/apache-arrow.sh).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587812949
##########
File path: dev/tasks/tasks.yml
##########
@@ -1789,6 +1789,18 @@ tasks:
r_image: r-base
r_tag: 3.6-opensuse42
not_cran: "TRUE"
+
+ test-r-minimal-build:
Review comment:
TODO: Use a different word because "minimal" already has a specific meaning (`LIBARROW_MINIMAL`) that's not this
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook removed a comment on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook removed a comment on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791154080
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-788387441
https://issues.apache.org/jira/browse/ARROW-11735
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586830050
##########
File path: r/R/dataset-partition.R
##########
@@ -76,7 +76,9 @@ HivePartitioning$create <- dataset___HivePartitioning
#' calling `hive_partition()` with no arguments.
#' @examples
#' \donttest{
-#' hive_partition(year = int16(), month = int8())
+#' if (arrow_with_dataset()) {
Review comment:
I initially did that, but apparently as of R 4.0.0:
>`R CMD check --as-cran` now runs `\donttest` examples
(from https://cran.r-project.org/doc/manuals/r-devel/NEWS.html)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791783688
> 1. configure.win should determine Arrow C++ library capabilities dynamically (like we do in configure)
ARROW-11884
> 2. `LIBARROW_MINIMAL=true` should entail that all optional features are `OFF`
ARROW-11885
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791749746
Good catch about the Windows builds. Could you make followup JIRAs for:
1. configure.win should determine Arrow C++ library capabilities dynamically (like we do in configure)
2. `LIBARROW_MINIMAL=true` should entail that all optional features are `OFF`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791851206
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588825318
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -50,7 +50,12 @@ jobs:
# we have to export this (right?) because we need it in the build env
export ARROW_R_DEV={{ not_cran }}
# Note that ci/scripts/r_test.sh sets NOT_CRAN=true if ARROW_R_DEV=TRUE
- docker-compose run r
+ docker-compose run \
+ -e ARROW_DATASET={{ arrow_dataset|default("ON") }} \
+ -e ARROW_PARQUET={{ arrow_parquet|default("ON") }} \
+ -e ARROW_S3={{ arrow_s3|default("ON") }} \
Review comment:
This should work too right? That way by default we're testing the default, unset versions.
```suggestion
-e ARROW_DATASET={{ arrow_dataset|default("") }} \
-e ARROW_PARQUET={{ arrow_parquet|default("") }} \
-e ARROW_S3={{ arrow_s3|default("") }} \
```
I'll try it and revert if it's no good.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r588825758
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -50,7 +50,12 @@ jobs:
# we have to export this (right?) because we need it in the build env
export ARROW_R_DEV={{ not_cran }}
# Note that ci/scripts/r_test.sh sets NOT_CRAN=true if ARROW_R_DEV=TRUE
- docker-compose run r
+ docker-compose run \
+ -e ARROW_DATASET={{ arrow_dataset|default("ON") }} \
+ -e ARROW_PARQUET={{ arrow_parquet|default("ON") }} \
+ -e ARROW_S3={{ arrow_s3|default("ON") }} \
Review comment:
I think so
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587897840
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Unfortunately this didn't pass through all the way: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=1721&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=240
I believe what you instead want is to pass them into docker-compose below, something like `docker-compose -e ARROW_DATASET={{ arrow_dataset|default("ON") }} -e ARROW_PARQUET={{ arrow_parquet|default("ON") }} -e ARROW_S3={{ arrow_s3|default("ON") }} r`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791104150
Revision: a810b3fce373c7f5090e367e5a6afc70db60414a
Submitted crossbow builds: [ursacomputing/crossbow @ actions-182](https://github.com/ursacomputing/crossbow/branches/all?query=actions-182)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-182-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791817416
Sorry, I just merged the dataset mutate() PR which added a cpp function. Can you please rebase and update the annotation on that function? Otherwise this is looking good, will merge.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586818947
##########
File path: r/configure
##########
@@ -26,13 +26,13 @@
# 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"
+BUNDLED_LIBS=""
Review comment:
Likewise I don't think you need to set this here, "" is what it will be anyway if it's not set.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587765098
##########
File path: r/tools/autobrew
##########
@@ -48,7 +48,8 @@ fi
# Hardcode this for my custom autobrew build
rm -f $BREWDIR/lib/*.dylib
AWS_LIBS="-laws-cpp-sdk-config -laws-cpp-sdk-transfer -laws-cpp-sdk-identity-management -laws-cpp-sdk-cognito-identity -laws-cpp-sdk-sts -laws-cpp-sdk-s3 -laws-cpp-sdk-core -laws-c-event-stream -laws-checksums -laws-c-common -lpthread -lcurl"
-PKG_LIBS="-L$BREWDIR/lib -lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_LIBS="-lparquet -larrow_dataset -larrow -larrow_bundled_dependencies -lthrift -llz4 -lsnappy -lzstd $AWS_LIBS"
+PKG_DIRS="-L$BREWDIR/lib"
Review comment:
I created ARROW-11861 to remind us of this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791518948
S3 is on because linuxlibs.R is turning it on: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=1741&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=24
I think you'll find the issue pretty quickly but LMK if you need a pointer.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586568597
##########
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:
I thought so too at first, but when I run
```
pkg-config --libs-only-l arrow-dataset
```
I get `-larrow-dataset` first and `-lparquet` second.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587897840
##########
File path: dev/tasks/r/azure.linux.yml
##########
@@ -47,6 +47,9 @@ jobs:
export R_ORG={{ r_org }}
export R_IMAGE={{ r_image }}
export R_TAG={{ r_tag }}
+ export ARROW_DATASET={{ arrow_dataset|default("ON") }}
+ export ARROW_PARQUET={{ arrow_parquet|default("ON") }}
Review comment:
Unfortunately this didn't pass through all the way: https://dev.azure.com/ursacomputing/crossbow/_build/results?buildId=1721&view=logs&j=0da5d1d9-276d-5173-c4c4-9d4d4ed14fdb&t=6c939d89-0d1a-51f2-8b30-091a7a82e98c&l=240
Setting them here affects the environment in which docker-compose is called, but it doesn't forward into the docker environment.
I believe what you instead want is to pass them into docker-compose below, something like `docker-compose -e ARROW_DATASET={{ arrow_dataset|default("ON") }} -e ARROW_PARQUET={{ arrow_parquet|default("ON") }} -e ARROW_S3={{ arrow_s3|default("ON") }} r`
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r585612183
##########
File path: r/tests/testthat/latin1.R
##########
@@ -15,7 +15,7 @@
# specific language governing permissions and limitations
# under the License.
-x <- iconv("Veitingasta�ir", to = "latin1")
+x <- iconv("Veitingasta�ir", to = "latin1")
Review comment:
Fixed in e894bdd
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791117362
@github-actions crossbow submit test-r-minimal-build
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586643234
##########
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:
`PKG_DIRS` contains only the `-L` flags, which are directories. `PKG_LIBS` contain only the `-l` flags, which are package names. Previously these were combined together in `PKG_LIBS`. I separated them because it makes the appending/prepending much more straightforward.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r587557393
##########
File path: r/configure
##########
@@ -26,13 +26,13 @@
# 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"
+BUNDLED_LIBS=""
Review comment:
Done in 13edc1478e5003e3a2b3dc27612c6d85f2aa9269
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] ianmcook commented on a change in pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
ianmcook commented on a change in pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#discussion_r586564929
##########
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:
Yep, the order in which the code below prepends to `PKG_LIBS` makes its value match the output of
```
pkg-config --libs-only-l arrow-dataset
```
(at least on my Mac) which is:
```
-larrow_dataset -lparquet -larrow
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #9610: ARROW-11735: [R] Allow Parquet and Arrow Dataset to be optional components
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9610:
URL: https://github.com/apache/arrow/pull/9610#issuecomment-791841697
Revision: b5ed1c0dace3dfa93b17d8545fd8a93a96dc4153
Submitted crossbow builds: [ursacomputing/crossbow @ actions-197](https://github.com/ursacomputing/crossbow/branches/all?query=actions-197)
|Task|Status|
|----|------|
|test-r-minimal-build|[](https://dev.azure.com/ursacomputing/crossbow/_build/latest?definitionId=1&branchName=actions-197-azure-test-r-minimal-build)|
----------------------------------------------------------------
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:
users@infra.apache.org