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

Build fails for lack of required (and stated, but not tested for) libraries #1

Open
eddelbuettel opened this issue Jul 6, 2022 · 30 comments

Comments

@eddelbuettel
Copy link

The CRANberries post on delaunay noted 'OS_type` so I got curious. In a quick Docker container (using my r2u where all required packages can be installed in seconds as binaries) we do see

g++ -std=gnu++14 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -flto=auto -Wl,-z,relro -o delaunay.so RcppExports.o del2D.o del2DC.o del3D.o delXY.o -lmpfr -lgmp -L/usr/lib/R/lib -lR
/usr/bin/ld: cannot find -lmpfr: No such file or directory                                                                                                                                                         
/usr/bin/ld: cannot find -lgmp: No such file or directory                                                
collect2: error: ld returned 1 exit status                                                                                                                                                                         
make: *** [/usr/share/R/share/make/shlib.mk:10: delaunay.so] Error 1                                     
ERROR: compilation failed for package ‘delaunay’                                                         
* removing ‘/usr/local/lib/R/site-library/delaunay’                 

and indeed you state this unconditionally in src/Makevars:

PKG_LIBS = -lmpfr -lgmp

(and leaving aside the gymnastics on BH which clearly violate the Suggests: nature, but let's leave that for another day) your (very impressive) journey through R packages is now at the point where you need to deal with external libraries which typically involves

  • a script configure to check if package GNU gmp and GNU mpfr are installed (for example via pkg-config --exists gmp && echo "yes")
  • possible use of pkg-config --libs (and pkg-config --cflags) to set the -L and -I options.

My container is based on Ubuntu jammy so I just did apt install libgmp-dev libmpfr-dev after which build succeeded with

g++ -std=gnu++14 -shared -L/usr/lib/R/lib -Wl,-Bsymbolic-functions -flto=auto -ffat-lto-objects -flto=auto -Wl,-z,relro -o delaunay.so RcppExports.o del2D.o del2DC.o del3D.o delXY.o -lmpfr -lgmp -L/usr/lib/R/lib -lR
installing to /usr/local/lib/R/site-library/00LOCK-delaunay/00new/delaunay/libs
** R
** inst
** byte-compile and prepare package for lazy loading
Warning: 'rgl.init' failed, running with 'rgl.useNULL = TRUE'.
** help
*** installing help indices
** building package indices
** testing if installed package can be loaded from temporary location
Warning: 'rgl.init' failed, running with 'rgl.useNULL = TRUE'.
** checking absolute paths in shared objects and dynamic libraries
** testing if installed package can be loaded from final location
Warning: 'rgl.init' failed, running with 'rgl.useNULL = TRUE'.
** testing if installed package keeps a record of temporary installation path
* DONE (delaunay)
@stla
Copy link
Owner

stla commented Jul 6, 2022

Thanks! I made this package Windows-only because of a warning with the CRAN checks on Unix, from the BH package, so the package is not acceptable to CRAN. On Windows there's no need of BH since Boost is provided by RTools.

I asked the question on StackOverflow about the configure script for Boost, but didn't get any reply. I'd like to require Boost as an external dependency for Unix users. In this way I could drop the restriction to Windows.

Cheers.

@eddelbuettel
Copy link
Author

I think you may need to demonstrate a preceived failing in BH more clearly. If I installed the missing libaries (which, to your credit) you list in SystemRequirements, your package both installs and checks just fine under Linux (here: Ubuntu 22.04).

And I fear you may also underestimate how diffcult Boost is with linking, which is why BH is header-only. A few BioConductor packages tried for Boost Graph algorithms and it is generally tricky as it also generally leads to linking requirement which is why BH is attractive. Anyway, best of luck, I was mostly just curious about a somewhat unusual setup.

@eddelbuettel
Copy link
Author

eddelbuettel commented Jul 6, 2022

(And if you wanted to check more 'loudly' more the two libraries, a file configure can be a shell script so you could first test if pkg-config is present and then use it to a) check for both gmp and mpfr and b) use it to adjust where those are. Your unconditional use of both from the standard location will work for most but not all setups which is why Writing R Extensions generally suggests what I just outlined. I can help you with that if you're interested.)

@stla
Copy link
Owner

stla commented Jul 6, 2022

Thanks, I will give a try. I check the package on Ubuntu with a Github action, and there's no warning. But at CRAN there's a warning with the Debian OS.

@eddelbuettel
Copy link
Author

You can use Docker to deploy containers for just about any flavour of Linux. The official R container is called r-base, and I maintain it (as it is copy / PR off our rocker/r-base container I look after). That makes it pretty easy to test, but 'chacun a son gout'. I find GitHub Actions somewhat non-transparent (and it so happens am currently debugging an issue for $dayjib there).

So in short, I don't think you can rely on one setup working at GitHub Actions. CRAN famously runs more including Debian setups (in Vienna) and Fedora (in Oxford).

@eddelbuettel eddelbuettel changed the title Build fails for lack of required libraries Build fails for lack of required (and stated, but not tested for) libraries Jul 6, 2022
@eddelbuettel
Copy link
Author

(Had a look around my machine and repos to find short (shell-based) configure and one I had was this one from websocket which follows the format set up by Jeroen -- there are many more examples. All of mine wrapping pkg-config are apparently couched within configure.ac to be compiled by autoconf.)

@eddelbuettel
Copy link
Author

As a very simply proof of concept, there is successful (on attempt, I forgot to flip to Ubuntu 22.04 in the first try) Actions run in a simple fork: https://github.com/eddelbuettel/delaunay/runs/7266688030?check_suite_focus=true

(I used my own ci script setup as a test of my recent r2u binaru repo, that required 22.04; feel free to ignore that and try with the standard actions script.)

The main changes are

  • a minor tweak to DESCRIPTION to use BH as a normal link depends (there may still be versioning issues between Cgal and BH which one could address in script configure, I have not done that yet)
  • a very simply script configure which tests that gmp amd mpfr are present and asks pkg-config for the linker and compiler flags
  • a simple src/Makevars.in that gets fed those flags and uses them

With those changes it builds fine here as I mentioned to you before, and also at GitHub Actions.

I also tried r-universe, but that just failed. I had contacted Jeroen about whether his setup was smart enough to auto-install gmp and mpfr (but never heard back), apparently it is not.

I'll remove delaunay again from my r-universe matrix to the linked page may disappear.

@eddelbuettel
Copy link
Author

And as shown in the GitHub Actions runs that works for both Ubuntu 20.04 (still "ubuntu-latest" at GitHub but otherwise now dated) and 22.04 (my preference now) which are the two flavours supported by my r2u making such tests quick and easy.

@stla
Copy link
Owner

stla commented Jul 11, 2022

Thanks.

Sadly I have two other packages with this issue: SurfaceReconstruction and MeshesOperations. They are wrappers of CGAL too, and they are very nice (and it was a considerable work).

The CRAN team discouraged me to submit Windows-only packages (they told me this doesn't respect the CRAN policies).

@eddelbuettel
Copy link
Author

The CRAN team discouraged me to submit Windows-only packages

Which is why I got curious and tried to help you. The package over in my work builds everywhere where gmp and mpfr are installed. So CRAN is right, there is no reason for a Windows-only restriction.

@stla
Copy link
Owner

stla commented Jul 11, 2022

But how to deal with the warning? It occurs on Debian. A CRAN member told me this is because the Debian machine uses GCC-12 (not sure to correctly remember this point).

@eddelbuettel
Copy link
Author

eddelbuettel commented Jul 11, 2022

Have you tried to reproduce this with the compiler used on Windows Debian? Rtools42 just like Rtools4 before it uses exactly one compiler and we know which. You can try to use the same compiler under Linux relatively easily. On the operating system I use most we currently default to gcc-11 and g++-11 but 12 is available.

You keep reiterating that line "only on Debian". So it could be caused by g++-12 and more stringent analysis by the compiler. It could also be a bug. We could (and should !!) replicate it. What you show in that link is a line from Windows which most clearly is not Debian. I think it would help us all if you constructed a replication of the warning. I can help you with Docker tricks as well with Debian / Ubuntu commands to select compilers.

Once we have it reproducing we could look at what Boost upstream is saying / if it still reproduces with current Boost version. Maintaining BH is a quite a bit of work / affects many package so I try to update only once year. But that means I may be a little behind in fixes.

In short, I am trying to help you. But it would help me if you could make the issue clear and reproducible . The above is not that (yet).

(Edited first paragraph as I got in your showing-Windows-when-you-meant-Debian text)

@eddelbuettel
Copy link
Author

For example this (older) ci.yaml of mine loops through g++ 7, 8, 9, and 10 on Ubuntu (as we needed that at some point, like a year or two ago). But starting from ubuntu-22.04 rather than ubuntu-latest (which is still 20.04 at GitHub Actions) we could get g++-12.

(That is not a reproduction of Vienna's Debian setup for which I would use Debian containers but your package has a few dependencies we'd need to compile each time and I want my CI to run fast which is why I currently prefer Ubuntu and r2u. But g++-12 is what matters so this may be sufficient. I may have time to work on this evening, but not now.)

@stla
Copy link
Owner

stla commented Jul 11, 2022

This is the same warning as the Windows one. But wait, I will show you the true CRAN warning, I have to change the computer...

@eddelbuettel
Copy link
Author

eddelbuettel commented Jul 11, 2022

I don't really need a textual line by line copy. I am suggesting you try to find way to reproduce it as only that gives you an ability to develop or test or evaluate alternatives.

@stla
Copy link
Owner

stla commented Jul 11, 2022

Ok, will try with Github action or Rhub. But here is what Kurt Hornik wrote:

It is a Debian machine, but afaict the issue is with using the libstdc++
headers from GCC 12, and the Debian check systems happen to be the only
ones using these.

And the warning:

> * checking whether package ‘delaunay’ can be installed ... [64s/93s] WARNING
> Found the following significant warnings:
>   /home/Hornik/lib/R/Library/4.2/x86_64-linux-gnu/BH/include/boost/container/
> detail/copy_move_algo.hpp:184:19: warning: ‘void* memmove(void*, const void*,
> size_t)’ writing to an object of type ‘value_type’ {aka ‘struct std::pair
> <CGAL::internal::CC_iterator<CGAL::Compact_container
> <CGAL::Constrained_triangulation_face_base_2<CGAL::Epick,
> CGAL::Triangulation_face_base_with_info_2<FaceInfo2, CGAL::Epick,
> CGAL::Triangulation_face_base_2<CGAL::Epick,
> CGAL::Triangulation_ds_face_base_2<CGAL::Triangulation_data_structure_2
> <CGAL::Triangulation_vertex_base_with_info_2<int, CGAL::Epick>,
> CGAL::Constrained_triangulation_face_base_2<CGAL::Epick,
> CGAL::Triangulation_face_base_with_info_2<FaceInfo2, CGAL::Epick> > > > > > >,
> CGAL::Default, CGAL::Default, CGAL::Default>, false>, int>’} with no trivial
> copy-assignment; use copy-assignment or copy-initialization instead
> [-Wclass-memaccess]
>   /home/Hornik/lib/R/Library/4.2/x86_64-linux-gnu/BH/include/boost/container/
> detail/copy_move_algo.hpp:184:19: warning: ‘void* memmove(void*, const void*,
> size_t)’ writing to an object of type ‘value_type’ {aka ‘struct std::pair
> <CGAL::internal::CC_iterator<CGAL::Compact_container
> <CGAL::Triangulation_ds_cell_base_3<CGAL::Triangulation_data_structure_3
> <CGAL::Triangulation_vertex_base_with_info_3<int, CGAL::Epick> > >,
> CGAL::Default, CGAL::Default, CGAL::Default>, false>, int>’} with no trivial
> copy-assignment; use copy-assignment or copy-initialization instead
> [-Wclass-memaccess]

@eddelbuettel
Copy link
Author

That to me makes it 100% clear that you

  • need to get yourself an ability run / build under Debian (and it hurts that RHub is currenty down / limited -- but there is Docker for; Github Actions does not offer Debian as an image) and that
  • you probably want to talk to / look into CGAL and Boost upstream as this is external code you rely upon but do not use.

BH is based on Boost 1.78. Upstream is now at 1.79; this will likely release 1.80 in August. Maybe they changed that?

@stla
Copy link
Owner

stla commented Jul 11, 2022

I remember I filled an issue on the Boost repo. And I think (not 100% sure) this was this one. The CGAL guys rarely reply (maybe I abuse their patience...). I show them an issue with Valgrind but it looks like they don't care.

Ok, good opportunity to try Docker. I used it only one time and I don't remember. But there are some tutos.

@eddelbuettel
Copy link
Author

Yes, Boost may be complicated as it is so many people. It helps to have subrepos. But even just being able to reproduce and to say to Kurt Hornik that is purely upstream is a better approach than your current 'oh I just make it Windows only and pretend Debian does not exist'.

@eddelbuettel
Copy link
Author

I am glad to see you are trying my files.

Would you consider actually acknowledging where those came from? There is zero record in any of the files, and zero record in the commit log. That is not exactly the recommended way to go about it.

@stla
Copy link
Owner

stla commented Jul 11, 2022

Ok. They work fine, thanks.

@eddelbuettel
Copy link
Author

I know they work. I tested them as I wrote them,

Did you read the second paragraph too in my previous comment? You took them without attribution or credit which is not exactly fair or common.

@stla
Copy link
Owner

stla commented Jul 11, 2022

Yes, I've just pushed them with the acknowledgment.

@stla
Copy link
Owner

stla commented Jul 11, 2022

I haven't buy a new computer yet. Currently, the only computer I have with admin rights is an old laptop with Windows 7. I will try Virtual Box to emulate a Linux machine and try Docker. But I should buy a new computer soon.

@eddelbuettel
Copy link
Author

Yes, I've just pushed them with the acknowledgment.

I appreciate that.

The only other problem is that you more or less broke my fork my created a parallel instance. The standard, cleaner, multi-user way would have been to try what you did in a branch and leave the main alone where you could then have asked me to submit a pull request,

Currently, the only computer I have with admin rights is an old laptop with Windows 7. I will try Virtual Box to emulate a Linux machine

It's very slow that way though because windows is not fast, and virtual box adds a layer. You can probably get a cheap and 'good enough' PC for Linux a few hundred Euros, or maybe a friend can share an account with you.

One other free alternative may be to use the 50 hours / month you can use at gitpod.io. I have gitpod setup and accessible via the bottom part of r2u README to launch a live session (they run on Google Cloud) with the repo I specified. The README even has this animated gif demo showing it. Easy enough to setup. And free Linux at your fingertips -- you get a real shell. Similar (for fewer hours) at Rstudio.cloud.

I had in the meantime changed my fork further where the one active active CI file now runs a 'matrix' with g++-11 and g++-12 (both under Ubuntu). I does not seem to replicate the issue Kurt sees.

@eddelbuettel
Copy link
Author

Some more news:

  • Boost is quite loud. There is not a lot we can do. As I recall we are allowed to tell Boost to not use auto_ptr. The other two settings I use now in the CI demo would not be allowed in ~/src/Makevars
  • But Boost knows. A little googling let me to to this issue and discussion so I think the next BH update (in December) may be better for us
  • Switching to C++17 does not yet help us
  • I now have added a local installation as part of the CI run too under g++-12 with added options and it is now nicely quiet

image

You have a good package here. We should make sure it gets the widest possible use.

@stla
Copy link
Owner

stla commented Jul 16, 2022

A fix !!! : boostorg/container#209 (comment)

@stla
Copy link
Owner

stla commented Jul 19, 2022

And the CRAN team has fixed something as well. Looks like the warning has gone 👍

@eddelbuettel
Copy link
Author

Nice to know -- they are mostly reasonable (if overworked/overwhelmed) folks.

Congrats also on getting qsplines onto CRAN. You remain on a mission.

@stla
Copy link
Owner

stla commented Jul 19, 2022

Yeah, they've published delaunay. I will submit my two other packages which use RcppCGAL.

For qsplines, I feared to not been able to translate the R code to C++, because there is a root-finding and an integral. I was happy to manage, thanks to Boost. Very easy actually with the help of lambda functions.

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

2 participants