reenable use of user-specified Eigen and CppAD

nimble-dev / nimble

The base NIMBLE package for R

http://R-nimble.org

BSD 3-Clause "New" or "Revised" License

160 stars 24 forks source link

reenable use of user-specified Eigen and CppAD #1443

Closed paciorek closed 5 months ago

paciorek commented 6 months ago

In modifying configure.ac to deal with CXX11/17 issues (roughly May 2023), I broke use of user-specified Eigen and CppAD libraries.

This fixes that, addressing the latest aspect of #1411 .

As part of this, I realized that it is now the case that for building libnimble we now "include" Eigen in some CppAD-related files. So this PR also replaces inst/CppCode/Makevars with inst/CppCode/Makevars.in so we can add in the -I for user-defined Eigen for use in building libnimble via configure.ac use of AC_SUBST().

@perrydv will be good to have you take a look.

paciorek commented 6 months ago

Windows testing is failing as follows. Need to see if changes in this PR affect compilation of dynamicRegistrations.

── 1. Error ('test-modelValuesInterface.R:27:5'): copying of argument passing in
177

178
Error in `inDL(x, as.logical(local), as.logical(now), ...)`: unable to load shared object 'C:/Users/RUNNER~1/AppData/Local/Temp/RtmpukY6Gy/nimble_generatedCode/dynamicRegistrations_05_18_21_51_01.dll':
179

180
  LoadLibrary failure:  The specified module could not be found.
181

perrydv commented 6 months ago

Thanks @paciorek . I'll get my Windows system updated and look at this. Are there corresponding changes to configure.win needed, and/or a Makevars.win (see src)?

paciorek commented 6 months ago

The buggy change that prompted this was in configure on the Linux side, so I wasn't anticipating needing to change on the Windows side. But I don't feel like I have a good understanding of the Windows side and there is the testing failure...

I think we'll want to try a user-specified Eigen on Windows and see what happens, in particular whether building libnimble succeeds now that that involves an Eigen include.

perrydv commented 6 months ago

@paciorek I think I fixed Windows package building for this. A step in configure.win was needed to be sure CppCode/Makevars was in place.

A few comments to check on if you have time:

We can't support user-provided CppAD, so it would be best to remove the implied support from configure.ac. If it is left in, is there a $ missing on line 116, i.e. we have test -n "CPPAD_DIR" but should it be "$CPPAD_DIR"? (But I think we'll be removing this anyway.)
We don't support user-provided Eigen on Windows.
Among other things your changes to configure.ac did was to remove an export. I am not clear on whether or why, so I am noting it.

paciorek commented 5 months ago

I've removed possibility of user-provided CppAD.

As far as the export, I removed it because the various variables are being set for internal use within the configure script and not used in child processes.