astamm / nloptr

nloptr provides an R interface to NLopt, a free/open-source library for nonlinear optimization providing a common interface to a number of different optimization routines which can handle nonlinear constraints and lower and upper bounds for the controls.
https://astamm.github.io/nloptr/
Other
105 stars 35 forks source link

Linking issue when several versions of nlopt are available #117

Closed astamm closed 2 years ago

astamm commented 2 years ago

I notice that the latest version of nloptr does not compile on any macOS systems because of tests checking that the version of nlopt is at least 2.7.0 fails systematically. See https://cran.r-project.org/web/checks/check_results_nloptr.html.

It seems indeed that the version of nlopt used by nloptr is anterior to 2.7.0 even though the package install does compile the included 2.7.1.

It seems that pkg-config does detect a system nlopt which is too old and thus nloptr install compiles a newer one but then it still links to the system version.

How can we ensure that the package properly links with the included version ? @eddelbuettel do you have any insights on this ?

Also, it might be linked, I received a mail from Simon Urbanek who says:

to make things worse it also creates a zombie process from sh tools/cmake_call.sh which manages to stall the entire build process as the master make is waiting for your process to terminate which it never does. For that reason I was forced to remove nloptr for the macOS CRAN automated build process. If you ever decide to fix your package, please make sure you also fix that issue and contact me directly so I can see if the package can be re-enabled.

It might be linked to the same issue. I suggest we try to fix the linking issue and see if the problem spotted by Simon persists.

astamm commented 2 years ago

I add this url from my morning test in case it can be helpful: https://mac.R-project.org/macbuilder/results/1652692135-0af7debb66a3e4d0/

eddelbuettel commented 2 years ago

Odd and weird, and sorry I have no macOS insight here. I have found I can usually rely on R to build working packages and link correctly. If that is no longer the case then I do not know where to start. I don't think we do anything special here -- apart of cause from involving cmake which as I mentioned in the past is a bit unusual for R packages but should not be deadly per se. It is the one difference though that this package has. You could try to simplify and use R's build process along with a local static build of libnlopt.a . It's a tough call. Do you have access to a macOS version as old as what Simon uses?

astamm commented 2 years ago

I am working a very recent macOS machines. But I run regular checks on the Mac-builder machine and this is where I realised that, despite having built the 2.7.1 version that we ship with the package, it still links to the system 2.6.1 as spotted by the first unit test of the C API. I think this is a question about how to make package install link to our 2.7.1 build when the latter co-exists with a system build of NLopt that is older than 2.7.0.

Whether this fix will be sufficient to address Simon's issue, I am not sure. But I wonder if we can first fix the linking issue and then see if it magically solves the other.

eddelbuettel commented 2 years ago

My configure.ac layer just calls your scripts so 'over to you'.

astamm commented 2 years ago

I know I know, I was just wondering if you had been confronted already to this problem and had a quick solution. I'll dig into it. Thanks.

eddelbuettel commented 2 years ago

We could also try to change the logic to ignore pkg-config and its advice if not on Linux and hence rely on local builds. But maybe cmake gets tricked by old installation. AFAICT this is very much specific to the "nloptr using cmake" approach you advocated (and which generally works, but apparently not always -- I was unaware of what @s-u told you until this morning).

s-u commented 2 years ago

FWIW from this discussion it seems that the CRAN issue would be trivial to solve if you just told us that you need newer version of nlopt: https://github.com/R-macos/recipes/blob/master/recipes/nlopt (or sent a PR) - in fact it would be much preferred to trying to build things on the fly (and it would also solve the zombie issue) - [if that is truly the issue].

eddelbuettel commented 2 years ago

@s-u That has been public knowledge and in the major release announcement. @astamm worked really hard to pick up @jyypma but there was a (known and documented) hard dependency:

https://github.com/astamm/nloptr/blob/master/NEWS.md#nloptr-200

The mistake, if any, @astamm made was maybe trusting cmake too much and assuming other / older system library versions would not get in the way. If memory serves me right now, this also bit us on CRAN's Fedora systems. The fact that we as package maintainers have no clear and uniform way of knowing what runs / is installed where does not exactly help. Ah well. One issue at a time then.

astamm commented 2 years ago

Thanks @s-u for helping us solving the issue. I was aware of a place to put external software for Windows but did not know you had set up the same system for macOS and that it is in use with R 4.2.0 (nloptr used to work before that). I have seen that you updated the version of nlopt some hours ago. The Mac-builder has not received it yet as it is still complaining that it links with the 2.6.2 but I'll try again later.

More generally, does that mean that we as package developer should send a PR systematically for any external software we want to use in our R packages? Both to macOS/recipes and rtools42 repos? What is the process for including external software in the general sense for CRAN packages? Is there a place where this is documented?

I also wonder what is the best strategy whenever I want to guarantee a given version for an external software I use in a package. Currently, for nloptr, if I detect a version that I consider too old, I build the soft from included sources. It seemed a neat solution at the time. But, now, I understand that if an older version exists, the package might link to it instead of the newly built one at install, causing unexpected behaviour. I wonder how to tell the compiler to systematically use the newly built one. I thought by reading through R documentations that linking and includes instructed through PKG_LIBS and PKG_CPPFLAGS were supposed to take precedence but it seems they do not.

In any case, thanks for your help and for the nlopt version update. I hope it solves the current issues and that we will be able to soon put nloptr back on CRAN.

eddelbuettel commented 2 years ago

It's a hard-ish (and also "boring" because mostly coordination rather than pure engineering) problem but I think "we" (looking for example at @s-u and myself as far as the R Foundation goes) should really do something to coordinate better information and ideally even choice about build requirements and availability on

s-u commented 2 years ago

@astamm for documentation see External Libraries for CRAN packages

As for linking, you should simply use the correct path to the library so in you case that would be nlopt/lib/libnlopt.a. You cannot use -l because that does a full search that will do a lot of unwanted things like looking for dynamic versions, system locations etc.

astamm commented 2 years ago

Thanks to both of you for following up on this. Thanks @s-u for the documentation, that's exactly what I needed. My last check on mac-builder succeeded: https://mac.R-project.org/macbuilder/results/1652823534-c20172e7c82e0ffe/. So it seems it picked up the new recipe for nlopt. I also pushed the linking modification you suggested which will avoid the potential -l mess for users who would have a too old system install of nlopt. Should I submit a new version to CRAN so that everything gets back in order?

s-u commented 2 years ago

As far as the macOS binaries are concerned, that is now fixed with the library update, so it's no longer in my court. The linking issue will affect anyone who has an older version of nlopt (most OSes prefer dynamic libraries to static so it's very unlikely that anyone will end up using the library you built), so a release would be likely a good thing, but possibly not urgent.

eddelbuettel commented 2 years ago

The new version is already on CRAN. (Note that the configure logic has been in the package since 2014 to prefer an available system library and avoid a local build. It generally just works -- but it got a little more complicated when we also enforced the preference for nlopt 2.7.0 or later. We needed a similar update by Tomas on Windows as well as he had an older version. Should all be good now.)

astamm commented 2 years ago

Thanks to both of you. Closing this now. It is fixed by 9f42e4d.