duckdb icon indicating copy to clipboard operation
duckdb copied to clipboard

[R] Package installation on aarch64 fails for v0.3.2

Open benz0li opened this issue 3 years ago • 26 comments

What happens?

R Package installation on aarch64 fails for v3.0.2: duckdb-0.3.2_aarch64_install-log.txt

To Reproduce (on aarch64)

  1. Shell:
    docker run --rm -ti registry.gitlab.b-data.ch/r/r-ver
    
  2. R:
    install.packages("duckdb")
    

Environment (please complete the following information):

  • OS: Debian GNU/Linux 11 (bullseye)
  • DuckDB Version: 0.3.2
  • DuckDB Client: R
> sessionInfo()
R version 4.1.2 (2021-11-01)
Platform: aarch64-unknown-linux-gnu (64-bit)
Running under: Debian GNU/Linux 11 (bullseye)

Matrix products: default
BLAS/LAPACK: /usr/lib/aarch64-linux-gnu/openblas-pthread/libopenblasp-r0.3.13.so

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods   base     

loaded via a namespace (and not attached):
[1] compiler_4.1.2

Before Submitting

  • [x] Have you tried this on the latest master branch?
  • Python: pip install duckdb --upgrade --pre
  • R: install.packages("https://github.com/duckdb/duckdb/releases/download/master-builds/duckdb_r_src.tar.gz", repos = NULL)
  • Other Platforms: You can find binaries here or compile from source.

👉 R Package successfully installs on aarch64 for branch master: duckdb-master_aarch64_install-log.txt

  • [x] Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?

benz0li avatar Feb 08 '22 07:02 benz0li

Do you have a ~/.R/Makevars file and if so, what's in it?

hannes avatar Feb 08 '22 09:02 hannes

Do you have a ~/.R/Makevars file and if so, what's in it?

I don't have a ~/.R/Makevars file.

benz0li avatar Feb 08 '22 09:02 benz0li

ℹ️ Latest master branch builds fine.

benz0li avatar Feb 08 '22 09:02 benz0li

That's very odd there were only minute changes....

hannes avatar Feb 08 '22 09:02 hannes

https://github.com/duckdb/duckdb/releases/download/master-builds/duckdb_r_src.tar.gz is from 4 Dec 2021.

benz0li avatar Feb 08 '22 11:02 benz0li

This looks like it fails because of compilation flags added by R itself, did you build R from source?

hannes avatar Feb 16 '22 12:02 hannes

This looks like it fails because of compilation flags added by R itself, did you build R from source?

Yes. But there is the same problem for the binary installation of R: duckdb-0.3.2_aarch64_binary-R_install-log.txt

benz0li avatar Feb 18 '22 07:02 benz0li

What's the output of R CMD config CXXPICFLAGS ?

hannes avatar Mar 17 '22 08:03 hannes

What's the output of R CMD config CXXPICFLAGS ?

Images using R built from source:

docker run --platform linux/arm64/v8 --rm -ti registry.gitlab.b-data.ch/r/r-ver:4.1.3 bash -c "R CMD config CXXPICFLAGS"
-fpic
docker run --platform linux/amd64 --rm -ti registry.gitlab.b-data.ch/r/r-ver:4.1.3 bash -c "R CMD config CXXPICFLAGS"
-fpic

Images using R binary installation:

docker run --platform linux/arm64/v8 --rm -ti debian bash -c "apt update && apt install -y r-base-dev && R CMD config CXXPICFLAGS"
[...]
-fpic
docker run --platform linux/amd64 --rm -ti debian bash -c "apt update && apt install -y r-base-dev && R CMD config CXXPICFLAGS"
[...]
-fpic

All the same.

benz0li avatar Mar 17 '22 08:03 benz0li

Well yes, that's the problem then. As the compile log says, this flag should be -fPIC instead. But we don't control that, whoever configures the R installation does.

hannes avatar Mar 17 '22 08:03 hannes

As the compile log says, this flag should be -fPIC instead.

I am fully aware what the compile log says. Other R packages build just fine - as duckdb did up to v0.3.1.

You might want to talk to @eddelbuettel when it comes to changing the build flags for the Debian R package.

benz0li avatar Mar 17 '22 08:03 benz0li

The package builds fine on Debian linux/amd64 with CXXPICFLAGS set to -fpic.

benz0li avatar Mar 17 '22 09:03 benz0li

And how is this relevant in the discussion?

hannes avatar Mar 17 '22 09:03 hannes

Thanks for tagging me in this thread -- won't claim I read every one of your many messages in detail. The use of -fpic vs -fPIC is something governed by R's own configure which Kurt Hornik looks after who is generally quite responsive. We had a recent bug report against the R package where an update was needed for 'alpha', the patch is still applied in 4.1.3 but has been added to r-devel after we got it and forwarded it. So if this is a Debian-on-aarch issue in general, it should bite you in more packages than just duckdb. @hannes is quite right that he is bystander here: an R package just "receives" these flags. You could test this locally by editing /etc/R/Makeconf on the failing system and see if it helps.

[ I have been meaning to turn LTO on as well but am a little fearful that we then end up with lots of reports like this. So so far I just follow along to what R Core and Debian default. ]

eddelbuettel avatar Mar 17 '22 11:03 eddelbuettel

With the following set in ~/.R/Makevars the package builds successfully:

ALL_CXXFLAGS =  $(PKG_CXXFLAGS) -fPIC $(SHLIB_CXXFLAGS) $(CXXFLAGS)

Thanks for all the feedback.

benz0li avatar Mar 17 '22 12:03 benz0li

Glad to hear, but that is not what I asked for. Can you please make a copy of /etc/R/Makeconf and change it there:

edd@rob:~$ grep fpic /etc/R/Makeconf 
CPICFLAGS = -fpic
CXXPICFLAGS = -fpic
CXX11PICFLAGS = -fpic
CXX14PICFLAGS = -fpic
CXX17PICFLAGS = -fpic
CXX20PICFLAGS = -fpic
FPICFLAGS = -fpic
edd@rob:~$ 

Can you also share if/when/how this is needed for other CRAN packages you build from source? If so we may indeed need to change the R package for Debian on aarch64 (and continue this as a Debian bug report). If however you only need this fpic -> fPIC fix for duckdb then the cause maybe somewhere in the (involved enough) build setup for duckdb.

The link error did come from a vendored library. So it could be duckdb, or it could be Debian.

eddelbuettel avatar Mar 17 '22 12:03 eddelbuettel

Glad to hear, but that is not what I asked for. Can you please make a copy of /etc/R/Makeconf and change it there:

Done. Works. See duckdb-0.3.2_aarch64_fPIC_install-log.txt.

Can you also share if/when/how this is needed for other CRAN packages you build from source?

None except duckdb so far (while building images [almost] identical to rocker/r-ver, rocker/tidyverse, rocker/verse, rocker/geospatial w/o RStudio for linux/arm64/v8).


I.e. the following 284 packages (including duckdb-0.3.1) build fine with *PICFLAGS = -fpic: packages.csv

benz0li avatar Mar 17 '22 12:03 benz0li

Error when building v0.3.3 from source:

[...]
g++ -std=gnu++11 -shared -L/usr/local/lib/R/lib -L/usr/local/lib -o duckdb.so rapi.o database.o connection.o statement.o register.o scan.o utils.o altrep.o types.o cpp11.o -L/usr/local/src/duckdb/build/debug/src -lduckdb_static -L/usr/local/src/duckdb/build/debug/extension/parquet -lparquet_extension -ldl -L/usr/local/src/duckdb/build/debug/third_party/fmt -lduckdb_fmt -L/usr/local/src/duckdb/build/debug/third_party/libpg_query -lduckdb_pg_query -L/usr/local/src/duckdb/build/debug/third_party/re2 -lduckdb_re2 -L/usr/local/src/duckdb/build/debug/third_party/miniz -lduckdb_miniz -L/usr/local/src/duckdb/build/debug/third_party/utf8proc -lduckdb_utf8proc -L/usr/local/src/duckdb/build/debug/third_party/hyperloglog -lduckdb_hyperloglog -L/usr/local/src/duckdb/build/debug/third_party/fastpforlib -lduckdb_fastpforlib -lpthread -L/usr/local/src/duckdb/build/debug/extension/parquet -lparquet_extension -L/usr/local/src/duckdb/build/debug/extension/icu -licu_extension -L/usr/local/src/duckdb/build/debug/extension/visualizer -lvisualizer_extension -L/usr/local/lib/R/lib -lR
register.o: in function `void std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>(char*, char*, std::forward_iterator_tag)':
/usr/include/c++/10/bits/basic_string.tcc:206:(.text+0x14): relocation truncated to fit: R_AARCH64_LD64_GOTPAGE_LO15 against symbol `__stack_chk_guard@@GLIBC_2.17' defined in .data.rel.ro section in /lib/ld-linux-aarch64.so.1
/usr/bin/ld: /usr/include/c++/10/bits/basic_string.tcc:206: warning: too many GOT entries for -fpic, please recompile with -fPIC
collect2: error: ld returned 1 exit status
make: *** [/usr/local/lib/R/share/make/shlib.mk:10: duckdb.so] Error 1
ERROR: compilation failed for package ‘duckdb’
* removing ‘/usr/local/lib/R/site-library/duckdb’

By the way, the same error when building v0.3.2 from source.


@eddelbuettel I don't think the R package for Debian on aarch64 needs to be changed. The following 284 packages (including duckdb-0.3.1) build fine with *PICFLAGS = -fpic: packages.csv


FYI @eitsupi for future linux/arm64/v8 builds of https://github.com/rocker-org/rocker-versioned2. I currently use the following workaround when building on aarch64:

if [ "$(dpkg --print-architecture)" = "arm64" ]; then
  cp -a /usr/local/lib/R/etc/Makeconf /usr/local/lib/R/etc/Makeconf.bak;
  sed -i 's/fpic/fPIC/g' /usr/local/lib/R/etc/Makeconf;
  install2.r --error --skipinstalled duckdb;
  mv /usr/local/lib/R/etc/Makeconf.bak /usr/local/lib/R/etc/Makeconf;
else
  install2.r --error --skipinstalled duckdb;
fi

benz0li avatar Apr 12 '22 12:04 benz0li

@benz0li Thank you for sharing your workaround. I too have been wondering about the recent failure of the arm64 build of the Docker image that installs those packages, and this is definitely the cause. https://github.com/eitsupi/r-ver

eitsupi avatar Apr 12 '22 12:04 eitsupi

(Just for completeness, the Debian packages does nothing "opionionated" itself here. So if on a particular platform we feel a -fpic needs to be a -fPIC (or vice versa) it is really upstream we need to talk to. I can (and have) test-drive a patch before talking about it to upstream, but I don't have the hardware platform so at the end of the day the easiest / cleanest is just for for @benz0li to locally modify his setup, test that everything works and then propose something. Now, we should probably get out of this thread here as it all is getting a wee bit tangential for duckdb.)

eddelbuettel avatar Apr 12 '22 12:04 eddelbuettel

Cross reference: https://bugs.r-project.org/show_bug.cgi?id=18326

benz0li avatar Apr 20 '22 06:04 benz0li

Simon Urbanek, 2022-04-27 09:59:44 UTC

Just a quick comment: please note that this is not an issue for R since it doesn't get anywhere close to the limit, so this is really low priority (if an issue at all). Packages with exceptionally large sizes can handle their own flags. Some further research suggests the overrides are intentional due to performance issues on some platforms so you'd likely have to convince experts in that field that that such chage does not cause more harm that good. Given the earlier comments I'm certainly not touching this.

– https://bugs.r-project.org/show_bug.cgi?id=18326#c14

benz0li avatar May 02 '22 04:05 benz0li

Olivier Benz, 2022-05-02 09:08:27 UTC

(In reply to Martin Maechler from comment #18)

So couldn't the package maintainer of such large packages, e.g. Hannes for duckdb not simply add a line such as

PKG_CXXFLAGS = -fPIC

to the package specific src/Makevars file?

Yes he could.

Or are you arguing that this should not be package specific, but arm64/aarch64 should always use -fPIC ?

IMHO arm64/aarch64 should always use -fPIC.

IIUC, Simon argues that this could be suboptimal as it would affect the build of R and all (src/ - using) packages (by default), when instead it's only needed for exceptionally large packages.

Simon uses -fPIC for Apple M1 (= arm64/aarch64), too.

– https://bugs.r-project.org/show_bug.cgi?id=18326#c19

benz0li avatar May 04 '22 09:05 benz0li

Martin Maechler, 2022-08-05 20:08:58 UTC

Now committed such a minimal change to configure.ac (and configure).

A more daring change would removed (almost?) all of these non-defaults from configure.ac and let default configuration "figure it out" or at least only listed the (few but very important) cases where to choose -fpic explicitly ...

– https://bugs.r-project.org/show_bug.cgi?id=18326#c22

benz0li avatar Aug 06 '22 06:08 benz0li

Will be fixed with the next minor release of R (~~4.2.2~~ 4.3.0^1) as *PICFLAGS ~~are now~~ will be set to -fPIC for aarch64.

benz0li avatar Aug 06 '22 06:08 benz0li

Inaction on bug reports can be hair-raisingly frustrating (not a complaint, everybody is busy... but our hands are tied) so good to see this unclogged by @mmaechler. As I recall, mlpack was the other (large) package affected by this and it may help there too.

eddelbuettel avatar Aug 06 '22 13:08 eddelbuettel