Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 tasks done
benz0li opened this issue Feb 8, 2022 · 26 comments
Closed
2 tasks done

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

benz0li opened this issue Feb 8, 2022 · 26 comments

Comments

@benz0li
Copy link

benz0li commented Feb 8, 2022

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

  • 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

  • Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?
@nbenn nbenn added the R label Feb 8, 2022
@hannes
Copy link
Member

hannes commented Feb 8, 2022

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

@benz0li
Copy link
Author

benz0li commented Feb 8, 2022

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

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

@benz0li
Copy link
Author

benz0li commented Feb 8, 2022

ℹ️ Latest master branch builds fine.

@hannes
Copy link
Member

hannes commented Feb 8, 2022

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

@benz0li
Copy link
Author

benz0li commented Feb 8, 2022

@hannes
Copy link
Member

hannes commented Feb 16, 2022

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

@benz0li
Copy link
Author

benz0li commented Feb 18, 2022

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

@hannes
Copy link
Member

hannes commented Mar 17, 2022

What's the output of R CMD config CXXPICFLAGS ?

@duckdb duckdb deleted a comment from benz0li Mar 17, 2022
@benz0li
Copy link
Author

benz0li commented Mar 17, 2022

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.

@hannes
Copy link
Member

hannes commented Mar 17, 2022

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.

@benz0li
Copy link
Author

benz0li commented Mar 17, 2022

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
Copy link
Author

benz0li commented Mar 17, 2022

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

@hannes
Copy link
Member

hannes commented Mar 17, 2022

And how is this relevant in the discussion?

@eddelbuettel
Copy link

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. ]

@benz0li
Copy link
Author

benz0li commented Mar 17, 2022

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

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

Thanks for all the feedback.

@eddelbuettel
Copy link

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.

@benz0li
Copy link
Author

benz0li commented Mar 17, 2022

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
Copy link
Author

benz0li commented Apr 12, 2022

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

@eitsupi
Copy link

eitsupi commented Apr 12, 2022

@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

@eddelbuettel
Copy link

(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.)

@benz0li benz0li closed this as completed Apr 12, 2022
@benz0li
Copy link
Author

benz0li commented Apr 20, 2022

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

@benz0li
Copy link
Author

benz0li commented May 2, 2022

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 benz0li reopened this May 2, 2022
@benz0li
Copy link
Author

benz0li commented May 4, 2022

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
Copy link
Author

benz0li commented Aug 6, 2022

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
Copy link
Author

benz0li commented Aug 6, 2022

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

Footnotes

  1. https://github.com/rocker-org/rocker-versioned2/issues/144#issuecomment-1230086099

@eddelbuettel
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants