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

reenable use of user-specified Eigen and CppAD #1443

Merged
merged 4 commits into from
May 29, 2024
Merged

reenable use of user-specified Eigen and CppAD #1443

merged 4 commits into from
May 29, 2024

Conversation

paciorek
Copy link
Contributor

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.

that CPPFLAGS is set correctly; also rework inst/CppCode/Makevars
to use user EIGEN_DIR as we now include Eigen in files compiled in libnimble
@paciorek paciorek requested a review from perrydv May 18, 2024 21:44
@paciorek
Copy link
Contributor Author

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

perrydv commented May 21, 2024

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

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

perrydv commented May 27, 2024

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

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.

@paciorek paciorek merged commit 77b36be into devel May 29, 2024
8 checks passed
@paciorek paciorek deleted the user_eigen branch May 29, 2024 17:56
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

Successfully merging this pull request may close these issues.

2 participants