Repository navigation
GH-35140: [R] Rewrite configure script and ensure we don't use mismatched libarrow - #35147
Conversation
|
|
|
|
|
If this is in fact the fix for the conda package issues (conda-forge/r-arrow-feedstock#56 (comment), conda-forge/r-arrow-feedstock#62), then we would have a way to test this in the nightly conda packaging jobs. We currently build the packages but don't install them and assert anything about their capabilities. Perhaps we can add something to https://github.com/apache/arrow/blob/main/dev/tasks/conda-recipes/r-arrow/build.sh to load the installed package and test that it has parquet, dataset, etc. |
|
@github-actions crossbow submit conda-linux--r4 conda-osx--r4 |
|
Revision: 1bc1a0132178aff5ce942a18f7a62bda4a689454 Submitted crossbow builds: ursacomputing/crossbow @ actions-02dfd6461d
|
There was a problem hiding this comment.
I think that the following approach (we must set LIB_DIR when PKG_* are set) is better than this approach:
diff --git a/r/configure b/r/configure
index 65528bdc3..f452fffb1 100755
--- a/r/configure
+++ b/r/configure
@@ -126,10 +126,11 @@ else
if [ "$UNAME" = "Darwin" ] && [ "$FORCE_BUNDLED_BUILD" != "true" ]; then
if [ "$FORCE_AUTOBREW" != "true" ] && [ "`command -v brew`" ] && [ "`brew ls --versions ${PKG_BREW_NAME}`" != "" ]; then
echo "*** Using Homebrew ${PKG_BREW_NAME}"
- BREWDIR=`brew --prefix`
+ BREWDIR=`brew --prefix ${PKG_BREW_NAME}`
+ LIB_DIR="$BREWDIR/lib"
PKG_LIBS="-larrow -larrow_bundled_dependencies"
- PKG_DIRS="-L$BREWDIR/opt/$PKG_BREW_NAME/lib $PKG_DIRS"
- PKG_CFLAGS="-I$BREWDIR/opt/$PKG_BREW_NAME/include $PKG_CFLAGS"
+ PKG_DIRS="-L$LIB_DIR $PKG_DIRS"
+ PKG_CFLAGS="-I$BREWDIR/include $PKG_CFLAGS"
else
echo "*** Downloading ${PKG_BREW_NAME}"
if [ -f "autobrew" ]; then
@@ -144,7 +145,9 @@ else
if [ $? -ne 0 ]; then
echo "Failed to retrieve binary for ${PKG_BREW_NAME}"
fi
- # autobrew sets `PKG_LIBS`, `PKG_DIRS`, and `PKG_CFLAGS`
+ # autobrew sets `PKG_LIBS`, `PKG_DIRS`, `PKG_CFLAGS`, and `BREWDIR`
+ # but `LIB_DIR` isn't set.
+ LIB_DIR="$BREWDIR/lib"
fi
else
if [ "${NOT_CRAN}" = "true" ]; thenThere was a problem hiding this comment.
The main reason I didn't take this approach is that r/configure has gotten so complex that I didn't think I could reason about all of the different branches of the code to know that I had found them all (see also #35049). Other, more concrete issues:
- I did confirm what @paleolimbot suggested, which is that if you have
pkg-configavailable and have installedapache-arrowviabrew, you won't get to this*** Using Homebrewblock, you'll see that you're using arrow found by pkg-config. So at a minimum, we (also) needLIB_DIRset in the pkg-config section above this. - Likewise, there have been similar reports, like the conda-forge one and [R] Improve package installation (on Fedora 36) with arrow system packages #31989, not on macOS/brew, so a brew-only fix wouldn't be sufficient.
- We actually don't need the autobrew one because (for "historical reasons", I assume) we explicitly set all of the feature flags in the autobrew script https://github.com/apache/arrow/blob/apache-arrow-11.0.0/r/tools/autobrew#L77
There was a problem hiding this comment.
@kou is this possibly relevant? PKG_LIBS and PKG_DIRS seem to be set correctly but for some reason LIB_DIR isn't.
diff --git a/r/configure b/r/configure
index 65528bdc38..8e130c53b4 100755
--- a/r/configure
+++ b/r/configure
@@ -110,7 +110,7 @@ else
PKG_CFLAGS="$PKGCONFIG_CFLAGS $PKG_CFLAGS"
PKG_LIBS="${PKGCONFIG_LIBS}"
PKG_DIRS="${PKGCONFIG_DIRS}"
- LIB_DIR=${FOUND_LIB_DIR}
+ LIB_DIR="${FOUND_LIB_DIR}"
# Check for version mismatch
PC_LIB_VERSION=`pkg-config --modversion arrow`There was a problem hiding this comment.
Sorry. Could you explain more the context for it? Can I see a build log for the case? This case #35140 ? Or other case?
If this is for #35140, I think that the isn't related because FOUND_LIB_DIR doesn't have any space:
*** Arrow C++ libraries found via pkg-config at /opt/homebrew/Cellar/apache-arrow/11.0.0_3/lib
There was a problem hiding this comment.
Ah, #35140 uses Apache Arrow 11.0.0.
I was seeing the main branch.
Yes. The change #35147 (comment) will fix #35140. And it's already in the main branch: #34229
There was a problem hiding this comment.
From the conda-side, we were able to fix it by specifying LIB_DIR=$PREFIX/lib in our build script, see conda-forge/r-arrow-feedstock#63
There was a problem hiding this comment.
🤦 my mistake, sorry I missed #34229, that explains some of the confusion. I'll tidy this PR up with any remaining useful changes and get it merged.
There was a problem hiding this comment.
That part is fine and already working (as in: shows the build fails) in conda-forge/r-arrow-feedstock#63.
However, I don't see how the BREW-specific patch above would apply to conda-forge (we'd need $PREFIX), nor do I know what changed between 10 & 11. While taking a first look, my comment in that conda-forge PR was:
Do we need to do something like in the upstream R Makefile to discover the deps correctly?
|
I've applied the patches and added a bunch of comments to |
|
@github-actions crossbow submit -g r |
|
Revision: 9dfa86d337e8428818148320c84881b3e89cb935 Submitted crossbow builds: ursacomputing/crossbow @ actions-8ccc8bc92d |
|
I will inspect the successful builds to make sure features are enabled as they should be. Of the failures:
|
|
@github-actions crossbow submit -g r |
|
I think that this problem is related to |
|
Ah, wait. The job was passed at #35147 (comment) . |
It passed there because I changed it not to use the bundled build. That said, it didn't pick up the wrong absl, so maybe there's still something to learn from it. |
|
Hmm. It seems that autobrew not Homebrew was used in the passed job. |
|
Ah, OK. Thanks. |
|
In the failed build, |
|
autobrew does not use pkg-config to set PKG_CFLAGS, so |
|
I think that this is the same problem as #34523 .
Abseil was mixed when we build C++'s |
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
|
@github-actions crossbow submit -g r |
|
Revision: a769f6a Submitted crossbow builds: ursacomputing/crossbow @ actions-674657ab73 |
|
I don't fully understand the devdocs failure, but it has to do with the fact that arrow 12.0.0 has been released (https://github.com/ursacomputing/crossbow/actions/runs/4862485406/jobs/8668915213#step:9:3878 vs. https://github.com/ursacomputing/crossbow/actions/runs/4833910722/jobs/8614497534#step:9:3880 where it passed before). The cflags/libs are identical between the two builds. Rebasing to pull in the new version number would probably solve it, though I'm not sure it's worth the electricity to confirm it (that's an expensive build for very little value, it seems). |
|
Works great on Ubuntu via Docker (arm64 and amd64)! It doesn't seem to be pulling the hosted static libraries for development, which I expected on Ubuntu/x86 (but maybe it's not supposed to?). |
We don't host binaries for x.y.z.9000, the nightly versions are like x.y.z.1000123, so if you wanted it to pull a nightly for a .9000 dev build, we'd have to teach it how to find the most recent one. Something for you (or whoever) to consider on #35049 I suppose. |
|
Made #35391 for the segfault on the ubuntu force tests job, which I see on |
|
Benchmark runs are scheduled for baseline = f794b20 and contender = ec89360. ec89360 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
|
['Python', 'R'] benchmarks have high level of regressions. |
…mismatched libarrow (apache#35147) I've significantly rewritten `r/configure` to make it easier to reason about and harder for issues like apache#34229 and apache#35140 to happen. I've also added a version check to make sure that we don't obviously try to use a system C++ library that doesn't match the R package version. Making sure this was applied in all of the right places and handling what to do if the versions didn't match was the impetus for the whole refactor. `configure` has been broken up into some functions, and the flow of the script is, as is documented at the top of the file: ``` # * Find libarrow on the system. If it is present, make sure # that its version is compatible with the R package. # * If no suitable libarrow is found, download it (where allowed) # or build it from source. # * Determine what features this libarrow has and what other # flags it requires, and set them in src/Makevars for use when # compiling the bindings. # * Run a test program to confirm that arrow headers are found ``` All of the detection of CFLAGS and `-L` dirs etc. happen in one place now, and they all prefer using `pkg-config` to read from the libarrow build what libraries and flags it requires, rather than hard-coding. (autobrew is the only remaining exception, but I didn't feel like messing with that today.) This should make the builds more future proof, should make it so more build configurations work (e.g. I suspect that a static build in ARROW_HOME wouldn't have gotten picked up correctly because it didn't add `-larrow_bundled_dependencies` to the libs, but now it will), and it may eliminate the redundant `-l` and `-D` setting I've observed in some builds (not harmful but definitely sloppy). Version checking has been added in an R script for ease of testing (and for easier handling of arithmetic), and there is an accompanying `test-check-versions.R` added. These are run on all the builds that use `ci/scripts/r_test.sh`. ### Behavior changes * If libarrow is found on the system (via ARROW_HOME, pkg-config, or brew), but the version does not match, it will not be used, and we will try a bundled build. This should mean that users installing a released version will never have libarrow version problems. * If both the found C++ library and R package are on matching dev versions (i.e. not identical given the x.y.z.9000 vs x+1.y.z-SNAPSHOT difference), it will proceed with a warning that you may need to rebuild if there are issues. This means that regular developers will see an extra message in the build output. * autobrew is only used on a release version unless you set FORCE_AUTOBREW=true. This eliminates another source of version mismatches (C++ release version, R dev version). * The path where you could set `LIB_DIR` and `INCLUDE_DIR` env vars has been removed. Use `ARROW_HOME` instead. * Closes: apache#35140 * Closes: apache#31989 Lead-authored-by: Neal Richardson <neal.p.richardson@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
…mismatched libarrow (apache#35147) I've significantly rewritten `r/configure` to make it easier to reason about and harder for issues like apache#34229 and apache#35140 to happen. I've also added a version check to make sure that we don't obviously try to use a system C++ library that doesn't match the R package version. Making sure this was applied in all of the right places and handling what to do if the versions didn't match was the impetus for the whole refactor. `configure` has been broken up into some functions, and the flow of the script is, as is documented at the top of the file: ``` # * Find libarrow on the system. If it is present, make sure # that its version is compatible with the R package. # * If no suitable libarrow is found, download it (where allowed) # or build it from source. # * Determine what features this libarrow has and what other # flags it requires, and set them in src/Makevars for use when # compiling the bindings. # * Run a test program to confirm that arrow headers are found ``` All of the detection of CFLAGS and `-L` dirs etc. happen in one place now, and they all prefer using `pkg-config` to read from the libarrow build what libraries and flags it requires, rather than hard-coding. (autobrew is the only remaining exception, but I didn't feel like messing with that today.) This should make the builds more future proof, should make it so more build configurations work (e.g. I suspect that a static build in ARROW_HOME wouldn't have gotten picked up correctly because it didn't add `-larrow_bundled_dependencies` to the libs, but now it will), and it may eliminate the redundant `-l` and `-D` setting I've observed in some builds (not harmful but definitely sloppy). Version checking has been added in an R script for ease of testing (and for easier handling of arithmetic), and there is an accompanying `test-check-versions.R` added. These are run on all the builds that use `ci/scripts/r_test.sh`. ### Behavior changes * If libarrow is found on the system (via ARROW_HOME, pkg-config, or brew), but the version does not match, it will not be used, and we will try a bundled build. This should mean that users installing a released version will never have libarrow version problems. * If both the found C++ library and R package are on matching dev versions (i.e. not identical given the x.y.z.9000 vs x+1.y.z-SNAPSHOT difference), it will proceed with a warning that you may need to rebuild if there are issues. This means that regular developers will see an extra message in the build output. * autobrew is only used on a release version unless you set FORCE_AUTOBREW=true. This eliminates another source of version mismatches (C++ release version, R dev version). * The path where you could set `LIB_DIR` and `INCLUDE_DIR` env vars has been removed. Use `ARROW_HOME` instead. * Closes: apache#35140 * Closes: apache#31989 Lead-authored-by: Neal Richardson <neal.p.richardson@gmail.com> Co-authored-by: Sutou Kouhei <kou@cozmixng.org> Signed-off-by: Neal Richardson <neal.p.richardson@gmail.com>
I've significantly rewritten
r/configureto make it easier to reason about and harder for issues like #34229 and #35140 to happen. I've also added a version check to make sure that we don't obviously try to use a system C++ library that doesn't match the R package version. Making sure this was applied in all of the right places and handling what to do if the versions didn't match was the impetus for the whole refactor.configurehas been broken up into some functions, and the flow of the script is, as is documented at the top of the file:All of the detection of CFLAGS and
-Ldirs etc. happen in one place now, and they all prefer usingpkg-configto read from the libarrow build what libraries and flags it requires, rather than hard-coding. (autobrew is the only remaining exception, but I didn't feel like messing with that today.) This should make the builds more future proof, should make it so more build configurations work (e.g. I suspect that a static build in ARROW_HOME wouldn't have gotten picked up correctly because it didn't add-larrow_bundled_dependenciesto the libs, but now it will), and it may eliminate the redundant-land-Dsetting I've observed in some builds (not harmful but definitely sloppy).Version checking has been added in an R script for ease of testing (and for easier handling of arithmetic), and there is an accompanying
test-check-versions.Radded. These are run on all the builds that useci/scripts/r_test.sh.Behavior changes
If libarrow is found on the system (via ARROW_HOME, pkg-config, or brew), but the version does not match, it will not be used, and we will try a bundled build. This should mean that users installing a released version will never have libarrow version problems.
If both the found C++ library and R package are on matching dev versions (i.e. not identical given the x.y.z.9000 vs x+1.y.z-SNAPSHOT difference), it will proceed with a warning that you may need to rebuild if there are issues. This means that regular developers will see an extra message in the build output.
autobrew is only used on a release version unless you set FORCE_AUTOBREW=true. This eliminates another source of version mismatches (C++ release version, R dev version).
The path where you could set
LIB_DIRandINCLUDE_DIRenv vars has been removed. UseARROW_HOMEinstead.Closes: [R] configure script for R fails to set PKG_CFLAGS #35140
Closes: [R] Improve package installation (on Fedora 36) with arrow system packages #31989