ARROW-11460: [R] Use system libraries if present on Linux #10710

thisisnic · 2021-07-13T10:55:07Z

No description provided.

github-actions · 2021-07-13T10:55:25Z

https://issues.apache.org/jira/browse/ARROW-11460

thisisnic · 2021-07-14T09:39:28Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-14T09:40:14Z

Revision: 58dc9db0deb842189306982ee87629d00faf0d86

Submitted crossbow builds: ursacomputing/crossbow @ actions-595

Task	Status
test-r-depsource

thisisnic · 2021-07-14T09:44:34Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-14T09:45:24Z

Revision: e3ffb69de9799801d5c7434405ca907867c2795e

Submitted crossbow builds: ursacomputing/crossbow @ actions-596

Task	Status
test-r-depsource

thisisnic · 2021-07-14T09:57:58Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-14T09:58:51Z

Revision: 55e28b6a55c417124d7a3b48c571eb473fa63136

Submitted crossbow builds: ursacomputing/crossbow @ actions-597

Task	Status
test-r-depsource

thisisnic · 2021-07-14T10:34:42Z

@github-actions crossbow submit test-r-depsource

thisisnic · 2021-07-14T10:46:33Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-14T10:47:27Z

Revision: 309842e1e8e21c0cf8b226bf8b4ba93b4fea5667

Submitted crossbow builds: ursacomputing/crossbow @ actions-598

Task	Status
test-r-depsource

thisisnic · 2021-07-14T11:53:32Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-14T11:54:20Z

Revision: aae65707cd2beed2ab38147b82c43a7f08af4ee6

Submitted crossbow builds: ursacomputing/crossbow @ actions-599

Task	Status
test-r-depsource

r/inst/build_arrow_static.sh

jonkeane · 2021-07-26T15:28:06Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-26T15:28:59Z

Revision: 525abeafa20107eb521a34ee7247d3e8284c8b9c

Submitted crossbow builds: ursacomputing/crossbow @ actions-643

Task	Status
test-r-depsource

thisisnic · 2021-07-27T10:26:48Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-27T10:27:36Z

Revision: 16b5bbc0b936621d1cc3f2cb75f81ca7c05c129a

Submitted crossbow builds: ursacomputing/crossbow @ actions-662

Task	Status
test-r-depsource

thisisnic · 2021-07-27T11:12:42Z

@github-actions crossbow submit test-r-depsource

github-actions · 2021-07-27T11:13:28Z

Revision: 0846bd785632cfc3c78d3f8a09dde4d40d50e1d9

Submitted crossbow builds: ursacomputing/crossbow @ actions-663

Task	Status
test-r-depsource

jonkeane · 2021-07-27T12:37:21Z

r/inst/build_arrow_static.sh

I don't think it makes sense to try and concoct a CI configuration that hits this else, but I'm curious if you've tried to do it locally and check that you get the expected behavior?

I have not but happy to do so to test it. What do you think is the most straightforward approach?

Should I just spin up a docker image?

For this I would ARROW_R_DEV=TRUE R_STATIC_DEPENDENCY_SOURCE=AUTO docker-compose build r and then interactively enter the docker container that is built there, uninstall pkg-config and then run /arrow/ci/scripts/r_test.sh or try installing manually

Oops, didn't see your second comment before I sent mine, yeah, exactly

I think you want ARROW_R_DEV=TRUE docker run -it apache/arrow-dev:r-rhub-ubuntu-gcc-release-latest -e ARROW_DEPENDENCY_SOURCE=AUTO /bin/bash (get the right variable name, and pass it into the runtime environment with -e)

oh you also need -v $(pwd):/arrow or something like that, that's what mounts the /arrow directory based on your local dir

Thanks Neal!

Just to clarify then, the reason why ARROW_DEPENDENCY_SOURCE needs to be passed in via -e but ARROW_R_DEV doesn't is:

'r' (defined in docker-compose.yml) is a service

specifying environment variables ahead of calling docker run allows you to set env vars for running that service

with ARROW_R_DEV, it doesn't matter whether you specify it via -e or via prefixing the call to docker run as it gets passed through anyway via the environment field of the r service

with ARROW_DEPENDENCY_SOURCE, it must be set via -e as there's nothing defined in the service to explicitly pass it through

Now tested and working as expected when pkg-config isn't installed

r/inst/build_arrow_static.sh

nealrichardson · 2021-07-27T12:50:00Z

.env

I don't think we need this here. We have to put env vars here that affect what the Dockerfiles build, but this is a runtime env var, so we can just pass it with -e and don't need to declare it here.

To add to that, here's one example of how that's done for a different CI job: https://github.com/apache/arrow/blob/master/.github/workflows/r-without-arrow.yml#L76

And for here, I would do ~the opposite of what I said yesterday, we want something like -e ARROW_DEPENDENCY_SOURCE=... in the docker call since that's the environment variable that we advertise and use throughout Arrow to determine that source.

Yesterday I mentioned calling it something like R_STATIC_DEP... in the .env file, which if we needed to pass it there would be good to mark that it only was getting passed to the R builds and wasn't a parameter that would help on a python build. But Neal's right that we should just put it in the docker command in the GHA workflow file and not worry about passing it around in docker-compose or the .env file. Sorry for my misdirection there yesterday!

nealrichardson · 2021-07-27T12:59:40Z

r/inst/build_arrow_static.sh

This isn't quite right/complete, and I think if we look at what is required to be complete, we'll see that we don't actually want this logic in this script. Working backwards, the point of Kou's change is that the pkgconfig files that the arrow C++ build does now contain all of the -l libs that the build requires from the system (which shouldn't be any if we're doing a BUNDLED build, but with AUTO there will be libs in it). So we need to make sure the PKG_LIBS env var in r/configure gets them. And since this script, and even r/tools/nixlibs.R, can't return anything or set anything in the env that r/configure is running in, we need to call pkg-config on the arrow C++ install dir from r/configure. (This is the missing par.) But that will only work if pkg-config exists, so we need to check for it like you do here--but I think we need to check for it in r/configure so that that script know whether to set AUTO or BUNDLED.

nealrichardson · 2021-07-28T14:45:30Z

r/configure

You don't want this change, this is for checking whether libarrow already exists on your system. (If you want to make a change here, edit the comments to clarify which case this is.)

Suggested change

if [ $? -eq 0 ] && [ "$ARROW_USE_PKG_CONFIG" != "false" ] && [ "$ARROW_DEPENDENCY_SOURCE" == "AUTO" ]; then

if [ $? -eq 0 ] && [ "$ARROW_USE_PKG_CONFIG" != "false" ]; then

However, you may want to pull Kou's suggestion from #10404 (comment) in here (though I'm not certain about the --static part in this block, may have to experiment locally)

I think these comments are still valid.

I don't disagree! I'm still working on this - converted it to a draft for the moment whilst I go read up on things as I realised I was just changing code as per all the helpful suggestions, but not understanding why.

nealrichardson · 2021-07-28T14:48:00Z

r/configure

You don't want this here. This is only relevant if you've run nixlibs.R to build libarrow. Put this somewhere around L176.

Also, before L160 (the call to nixlibs.R), add something like:

if [ "${ARROW_DEPENDENCY_SOURCE}" = "" ]; then # TODO: BUNDLED is still default for now, but we plan to change it to AUTO ARROW_DEPENDENCY_SOURCE=BUNDLED; export ARROW_DEPENDENCY_SOURCE fi if [ "${ARROW_DEPENDENCY_SOURCE}" = "AUTO" ]; then which pkg-config >/dev/null 2>&1 if [ $? -eq 0 ]; then export ARROW_DEPENDENCY_SOURCE=BUNDLED # echo something? fi fi

Finally, you also need to edit build_arrow_static.sh to take ARROW_DEPENDENCY_SOURCE from the environment (default arg there BUNDLED)

nealrichardson · 2021-07-28T14:53:00Z

r/configure

I think you want something like this, though we may need to adapt PKG_CONFIG_NAME based on whether dataset and parquet are built (i.e. listed in BUNDLED_LIBS, defined right above where this goes)

Suggested change

PKG_LIBS = "pkgconfig ${LIB_DIR} $PKG_LIBS"

PKG_LIBS="$PKG_LIBS `PKG_CONFIG_PATH=${LIB_DIR} pkg-config --libs-only-l --libs-only-other --static --silence-errors ${PKG_CONFIG_NAME}`"

nealrichardson · 2021-07-30T14:32:27Z

r/configure

I think you still need #10710 (comment) here, and also adding a comment explaining what's up would help our future selves.

…ENCY_SOURCE in linux-apt-r

jonkeane · 2021-08-17T15:03:24Z

@github-actions crossbow submit conda-linux-gcc-py36-cpu-r40 conda-linux-gcc-py37-cpu-r41 test-r-depsource-auto test-r-depsource-system test-r-ubuntu-21.04 test-r-gcc-11

github-actions · 2021-08-17T15:05:09Z

Revision: 2cf3a26

Submitted crossbow builds: ursacomputing/crossbow @ actions-781

Task	Status
conda-linux-gcc-py36-cpu-r40
conda-linux-gcc-py37-cpu-r41
test-r-depsource-auto
test-r-depsource-system
test-r-gcc-11
test-r-ubuntu-21.04

jonkeane · 2021-08-17T17:35:50Z

@github-actions crossbow -g r

github-actions · 2021-08-17T17:36:20Z

No such option: -g

jonkeane · 2021-08-17T17:38:02Z

@github-actions crossbow submit -g r

github-actions · 2021-08-17T17:38:54Z

Revision: 2cf3a26

Submitted crossbow builds: ursacomputing/crossbow @ actions-782

Task	Status
conda-linux-gcc-py36-cpu-r40
conda-linux-gcc-py37-cpu-r41
conda-osx-clang-py36-r40
conda-osx-clang-py37-r41
conda-win-vs2017-py36-r40
conda-win-vs2017-py37-r41
homebrew-r-autobrew
test-r-depsource-auto
test-r-depsource-system
test-r-devdocs
test-r-gcc-11
test-r-install-local
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-ubuntu-gcc-release-latest
test-r-rocker-r-base-latest
test-r-rstudio-r-base-3.6-bionic
test-r-rstudio-r-base-3.6-centos7-devtoolset-8
test-r-rstudio-r-base-3.6-centos8
test-r-rstudio-r-base-3.6-opensuse15
test-r-rstudio-r-base-3.6-opensuse42
test-r-ubuntu-21.04
test-r-version-compatibility
test-r-versions
test-r-without-arrow
test-ubuntu-18.04-r-sanitizer

jonkeane · 2021-08-17T19:28:33Z

AFAICT, conda-osx-clang-py36-r40, conda-osx-clang-py37-r41, homebrew-r-autobrew, test-r-rhub-debian-gcc-devel-lto-latest, and test-ubuntu-18.04-r-sanitizer are all failing on crossbow already independent of this change.

nealrichardson · 2021-08-19T16:06:17Z

Ok, I checked the test-r-depsource-auto build output. It does seem to be picking up system libz and libbz2, though that's it. We do have a few tests that are skip_if_not_available("gzip"), and those tests were not skipped, so that confirms that we've successfully used the system libz.

So, +1 from me again on this. Will merge.

github-actions bot added the Component: R label Jul 13, 2021

thisisnic changed the title ~~ARROW-11460: [R] Use system libraries if present on Linux [WIP]~~ ARROW-11460: [R] Use system libraries if present on Linux Jul 13, 2021

jonkeane reviewed Jul 26, 2021

View reviewed changes

r/inst/build_arrow_static.sh Outdated Show resolved Hide resolved

thisisnic marked this pull request as ready for review July 27, 2021 10:26

thisisnic force-pushed the patch-1 branch from 16b5bbc to ca03227 Compare July 27, 2021 10:39

jonkeane reviewed Jul 27, 2021

View reviewed changes

r/inst/build_arrow_static.sh Outdated Show resolved Hide resolved

nealrichardson requested changes Jul 27, 2021

View reviewed changes

nealrichardson requested changes Jul 28, 2021

View reviewed changes

nealrichardson reviewed Jul 30, 2021

View reviewed changes

thisisnic marked this pull request as draft July 30, 2021 15:02

thisisnic force-pushed the patch-1 branch from d118d40 to 83cb89c Compare August 2, 2021 13:25

jonkeane and others added 19 commits August 17, 2021 17:01

Add if: always() to make the dump step actually work

c3ea16b

oops

ab1d149

more bash

593b930

pass AWSSDK_SOURCE

956b3d6

Kou's patch

acb276b

[[ -> [

2593146

Add find_package PKGConfig

4765f8e

Run ldconfig

2533871

use ubuntu-r-only-r for 21.04

3ef25a4

add pkgconfig to the pkg-config path

39156ee

cleanup a whitespace cruft

9e2c3e2

Move PkgConfig finding to correct location

27baa42

a ARROW_DEPENDENCY_SOURCE=SYSTEM task + trying to unsent ARROW_DEPEND…

e841f80

…ENCY_SOURCE in linux-apt-r

try backing out --static

a3b0f18

no --static anywhere

13b6b6b

Update cpp/cmake_modules/ThirdpartyToolchain.cmake

bcba8ba

try adding back in one --static?

ec61d2d

remove an overly-zealous addition

d2f6d1a

shuffle where we unset ARROW_DEPENDENCY_SOURCE

2cf3a26

jonkeane force-pushed the patch-1 branch from 88332f9 to 2cf3a26 Compare August 17, 2021 15:02

nealrichardson closed this in b747e2c Aug 19, 2021

asfimport mentioned this pull request Aug 19, 2021

[R] Use system libraries if present on Linux #27345

Closed

	if [ $? -eq 0 ] && [ "$ARROW_USE_PKG_CONFIG" != "false" ] && [ "$ARROW_DEPENDENCY_SOURCE" == "AUTO" ]; then
	if [ $? -eq 0 ] && [ "$ARROW_USE_PKG_CONFIG" != "false" ]; then

	PKG_LIBS = "pkgconfig ${LIB_DIR} $PKG_LIBS"
	PKG_LIBS="$PKG_LIBS `PKG_CONFIG_PATH=${LIB_DIR} pkg-config --libs-only-l --libs-only-other --static --silence-errors ${PKG_CONFIG_NAME}`"

ARROW-11460: [R] Use system libraries if present on Linux #10710

ARROW-11460: [R] Use system libraries if present on Linux #10710

Uh oh!

Conversation

thisisnic commented Jul 13, 2021

Uh oh!

github-actions bot commented Jul 13, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

thisisnic commented Jul 14, 2021

Uh oh!

github-actions bot commented Jul 14, 2021

Uh oh!

Uh oh!

jonkeane commented Jul 26, 2021

Uh oh!

github-actions bot commented Jul 26, 2021

Uh oh!

thisisnic commented Jul 27, 2021

Uh oh!

github-actions bot commented Jul 27, 2021

Uh oh!

thisisnic commented Jul 27, 2021

Uh oh!

github-actions bot commented Jul 27, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic Aug 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thisisnic Aug 2, 2021 •

edited

Loading

nealrichardson Jul 28, 2021 •

edited

Loading