opened
nealrichardson
username_0
These extra flags got dropped in https://github.com/apache/arrow/commit/a44d1f553a3d1716dfafa7bb02753b8c4ed13d3f#diff-2816240325a8e9e6df50fb7c1faa8113R21. This change resolves the issue for me locally on macOS, and I'll confirm that it fixes the Ubuntu build for sparklyr and that it doesn't break our Travis build.
ARROW-5332: [R] Fix for failure to find libarrow on R package installation
issue
created
nealrichardson
username_0
Confirmed that this fixes the installation on sparklyr too: https://travis-ci.org/username_0/sparklyr/jobs/533045007
comment
created
javierluraschi
username_1
Probably worth waiting for a review from @romainfrancois... Those flags were added for the initial commit from Romain: https://github.com/apache/arrow/commit/ea8940ae8d524b0625bdce194b141315bfa260ea#diff-2816240325a8e9e6df50fb7c1faa8113 and then removed again by Romain with a comment pointing to @username_2. I don't see other R packages using the path explicitly. So maybe there is some abnormal configuration in Travis and your machine or maybe there is something deeper going on. I did spot this article: https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Unix_002dalikes -- In any case, probably worth getting more eyes on this change.
comment
created
jeroen
username_2
You shouldn't hardcode your local installation path in the generic makefile. What problem are you trying to solve exactly? This works for me on macos:

```
brew install apache-arrow
```

And then in R:

```r
remotes::install_github("apache/arrow/r")
```
comment
created
nealrichardson
username_0
That works for me, too, but that's not the problem I'm trying to solve here. See https://issues.apache.org/jira/browse/ARROW-5332 for details. Installing the R package using the locally built C++ library is failing, not only on my machine but also on Travis on Ubuntu for the [sparklyr](https://travis-ci.org/rstudio/sparklyr/jobs/523326800) package. 

It was not my intention to hardcode _my_ path; I merely bisected git history to find out when this stopped working, and I landed on that line in that diff. 

A couple of alternatives that come to mind:

1. Determine that `-rpath` location dynamically and maybe only include it when `pkg-config` is used
2. Figure out what's different about how https://github.com/apache/arrow/blob/master/ci/travis_before_script_cpp.sh builds and installs and do that since that works. I noticed, for example, that it installs with `-DARROW_INSTALL_NAME_RPATH=OFF`. I tried that and that appeared to work locally for me, but I still got a failure to install on [sparklyr Travis](https://travis-ci.org/username_0/sparklyr/jobs/533410402). Fortunately, arrow's CMake run now prints a build configuration summary so we can look to see what's different; unfortunately, the summary is over 80 lines long because there are so many flags.
comment
created
jeroen
username_2
If it works for the brewed libarrow, and for arrow 0.13, I think this is a problem in the C++ library? Either way I think hardcoding a local path in the r bindings is not the solution.
comment
created
wesm
username_3
See relevant rpath-related build options

https://github.com/apache/arrow/blob/b199b27646ecf8432d3b6d9a70858a8a875d8283/cpp/cmake_modules/DefineOptions.cmake#L81

Not sure if changing those fixes the issue, but this is the first rpath issue we've seen in quite some time
comment
created
wesm
username_3
@username_0 is libarrow in your LD_LIBRARY_PATH/DYLD_LIBRARY_PATH for your local install? If the library is in a non-standard location then you need to do that
comment
created
nealrichardson
username_0
Like I said, setting `-DARROW_INSTALL_NAME_RPATH=OFF` (specifically, adding it to the `cmake` command given on the [R README](https://github.com/apache/arrow/blob/master/r/README.Rmd#L42-L43)), following what the Arrow Travis build does, resolves the issue for me locally, without hardcoding an rpath in the R makevars. But when I [added that to sparklyr's Travis build script](https://github.com/username_0/sparklyr/commit/a9f4507dc2530e3283f96566808710c9701799de), it still [failed](https://travis-ci.org/username_0/sparklyr/jobs/533410402). I haven't further dug into how its Travis build setup differs from the Arrow build setup.

Stepping back, I wonder if it would make sense for us to provide an installation script that compiles with the appropriate flags that the R package needs (leaving aside this path issue, it seems that R doesn't currently need Gandiva, Flight, et al.). The R README could recommend using it to install from source, and other packages like `sparklyr` (and I expect there will be more) could use it too. That way it would also be something we own and maintain, rather than have others' maintain their own bespoke scripts that drift and cause bug reports like this.
comment
created
nealrichardson
username_0
For posterity, I came across this discussion around the original hard-coding of the rpath: https://github.com/apache/arrow/pull/2489#discussion_r215664651.
comment
created
kou
username_4
`ARROW_INSTALL_NAME_RPATH` is for macOS. So the Travis CI failure is unrelated because it's on Linux.

On Linux, you need to add the directory where your `libarrow.so` to `LD_LIBRARY_PATH`. The following patch will fix the failure:

```diff
diff --git a/.travis.yml b/.travis.yml
index 77574796..62211779 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -74,6 +74,7 @@ matrix:
         - ARROW_ENABLED="true"
         - ARROW_VERSION="devel"
         - ARROW_SOURCE="build"
+        - LD_LIBRARY_PATH="/usr/local/lib"
         - JAVA_VERSION=oraclejdk8
     - name: "Deps Devel (tidyverse, r-lib, forge)"
       env: R_DEVEL_PACKAGES="true"
@@ -92,6 +93,7 @@ matrix:
         - ARROW_ENABLED="true"
         - ARROW_VERSION="devel"
         - ARROW_SOURCE="build"
+        - LD_LIBRARY_PATH="/usr/local/lib"
         - JAVA_VERSION=oraclejdk8
 
 before_install:
```

See https://travis-ci.org/username_4/sparklyr/builds/534033939 for the result with the change.

Note that R build is broken on the current master because R needs follow changes at https://github.com/apache/arrow/commit/e68ca7f9aed876a1afcad81a417afb87c94ee951 . You need to use https://github.com/apache/arrow/commit/1ed608aa94f0d22cfa69c81687006b4ebaefa258 for now:

```diff
diff --git a/ci/arrow-build.sh b/ci/arrow-build.sh
index 247df096..a6fa4f43 100644
--- a/ci/arrow-build.sh
+++ b/ci/arrow-build.sh
@@ -9,6 +9,7 @@ sudo apt install -y -V autoconf
 if [[ $ARROW_VERSION == "devel" ]]; then
   git clone https://github.com/apache/arrow
   cd arrow/cpp
+  git checusername_4t 1ed608aa94f0d22cfa69c81687006b4ebaefa258
 else
   wget https://arrowlib.rstudio.com/dist/arrow/arrow-$ARROW_VERSION/apache-arrow-$ARROW_VERSION.tar.gz
   tar -xvzf apache-arrow-$ARROW_VERSION.tar.gz

```
comment
created
kou
username_4
FYI: Setting `LD_LIBRARY_PATH` by users is a general task when users install a library to non `/usr/lib` directory. It's not Arrow specific task.
comment
created
wesm
username_3
I agree with @username_4 that this particular issue (making sure your system can "see" the installed C++ libraries) is something that developers need to be aware of (which we should document adequately, of course) -- I'm skeptical that a one-size-fits-all and cross-platform solution can be developed. 

For Python we modify LD_LIBRARY_PATH to point to the custom install location, as do the GLib and Ruby tests

https://github.com/apache/arrow/blob/d8e4763c4fafce9bb8fa9621d69a2ac4200186ea/ci/travis_script_python.sh#L27

For _development_ it is not recommended to install development libraries into default system locations because it contaminates any packages installed by package managers such as `brew`
comment
created
nealrichardson
username_0
Thanks @username_4! I'll rework this patch to just update the R README with appropriate installation guidelines, and I'll take care of submitting the sparklyr Travis patch, unless you beat me to it.

More generally, 

1. I wonder if the default for `ARROW_INSTALL_NAME_RPATH` is wrong, because I see lots of places that have to set it to `OFF`
2. I still think we can do better at collecting our C++ dev installation best practices in one or more scripts that we recommend people to use, or at least start with. Maybe not for Arrow C++ devs who want more control but at least for developers of the Python, Ruby, and R bindings who want to work off of the latest C++ lib and can't just install the published packages, and for those building libraries on top of those bindings (like the `sparklyr` case). It may not be one-size-fits-all, but I think it's a small number of variations we need to care about. We have the core of this already in the scripts in `/ci`. I'll make a Jira ticket and we can discuss there.
comment
created
kou
username_4
`ARROW_INSTALL_NAME_RPATH` changes `install_name` of `libarrow.dylib`.

If `ARROW_INSTALL_NAME_RPATH` is used, the `install_name` is `@rpath/libarryw.dylib`. If we use`@rpath`, we can put `libarrow.dylib` anywhere. Because we can replace `@rpath` by `DYLD_LIBRARY_PATH` environment variable at run-time.

If `ARROW_INSTALL_NAME_RPATH` isn't used, the `install_name` is `${CMAKE_INSTALL_PREFIX}/lib/libarrow.dylib`. If we use the static path, we can put `libarrow.dylib` to only `${CMAKE_INSTALL_PREFIX}/lib`. (`make install` does this.)

If we build, install and use `libarrow.dylib` on the same machine, we can use the static path (we can use `-DARROW_INSTALL_NAME_RPATH=OFF`.) But if we build `libarrow.dylib` on A and install and use `libarrow.dylib` on B, we should use `@rpath`. Because we can't assume where `libarrow.dylib` is installed.
comment
created
kou
username_4
This may be an out of scope topic of this but it may be better that we provide an install script for Travis CI.
I did this for my product Groonga: https://github.com/groonga/groonga/blob/master/data/travis/setup.sh
Users can install Groonga by `curl --silent --location https://github.com/groonga/groonga/raw/master/data/travis/setup.sh | sh`: https://github.com/pgroonga/pgroonga/blob/master/.travis.yml#L43
If users want to use master instead of released version, users just define `GROONGA_MASTER=yes` variable:
* https://github.com/pgroonga/pgroonga/blob/master/.travis.yml#L38
* https://github.com/groonga/groonga/blob/master/data/travis/setup.sh#L21

We can do the same thing for Apache Arrow.
comment
created
nealrichardson
username_0
Thanks @username_4 for the explanation, that's really helpful. 

I agree that it would be a good idea to provide and manage an install script, or set of scripts, for use in other projects on Travis and otherwise. I'll make a new Jira for that. I'll limit this ticket to be about the R installation docs/README. 

One last thought here: what if `make install` did some version of `export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$ARROW_CPP_INSTALL/lib` at the end? I see some version of that in 5 of our `ci/` scripts and recommended in the docs for several language bindings. For the use case where someone is building/installing the C++ library in order to then build some other project, it would help, and it wouldn't hurt anything else (IIUC).
comment
created
kou
username_4
I'm sorry but I couldn't understand.
Do you want to know which is recommended, `LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$ARROW_CPP_INSTALL/lib` or `LD_LIBRARY_PATH=$ARROW_CPP_INSTALL/lib:$LD_LIBRARY_PATH`?
comment
created
nealrichardson
username_0
I meant, let's assume this workflow for installing, adapted from the [R package README](https://github.com/apache/arrow/blob/master/r/README.Rmd#L38-L52)

```shell
git clone https://github.com/apache/arrow.git
mkdir arrow/cpp/build
cd !$
cmake .. -DARROW_PARQUET=ON -DCMAKE_BUILD_TYPE=Release -DARROW_BOOST_USE_SHARED:BOOL=Off
make install
cd ../../r
R CMD INSTALL .
```

That will apparently fail unless I do `export LD_LIBRARY_PATH=/usr/local/bin` before I `R CMD INSTALL .`, otherwise it won't find libarrow. So I can add that `export` as a step in the recommended instructions. But why not have `make install` set that env var for me?
comment
created
wesm
username_3
The build system isn't able to modify the environment like that. Such a modification would not be permanent, anyway, so it would be lost the next time you open a terminal. It's better to put such modifications in a shell script that is executed like `source arrow_set_env.sh` or similar (that's how I do development locally, anyway)
comment
created
jeroen
username_2
@username_0 do you really need to build arrow from source? Perhaps you can install the arrow for linux from the bintray repositories as described here: https://arrow.apache.org/install/

After that you can simply use `remotes::install_github("apache/arrow/r")` in R.
comment
created
nealrichardson
username_0
@username_2 yeah I do need to install from source unless there are nightly builds available for download somewhere. I was looking into adding bindings for features in the C++ library that have been added since the 0.13 release. Plus I was interested to see if the installation instructions were still valid...

@username_3 alright, I'll update the README here and ticket any future work on installation scripts. 

Thanks all for your patience and help.
comment
created
kou
username_4
@username_0 Thanks. I understand.

We can't do it as @username_3 said.

How about changing the instruction to the following in README.md?

```shell
cmake .. -DCMAKE_INSTALL_PREFIX=$HOME/local/arrow -DARROW_PARQUET=ON -DCMAKE_BUILD_TYPE=Release -DARROW_BOOST_USE_SHARED:BOOL=Off
make install
echo "export LD_LIBRARY_PATH=\$HOME/local/arrow/lib:\$LD_LIBRARY_PATH" >> ~/.$(basename $SHELL)rc
```

@username_2 I agree to use Arrow C++ package for using the R bindings. But I don't agree to use Arrow C++ package for developing the R bindings.

Because Arrow C++ API may be changed and added after the latest release. The R bindings should follow these changes and new features.
comment
created
nealrichardson
username_0
Thanks all for your help here. I've taken a different approach and just updated the README to reflect what appears to be best practice now, plus some pointers to help if things go wrong. No build changes or paths hard-coded.
comment
created
nealrichardson
username_0
Thanks @username_4! 🎉
comment