opensim-core
opensim-core copied to clipboard
Fix IPOPT library install with only OPENSIM_WITH_CASADI flag
Fixes issue #3267
Brief summary of changes
Moved the logic for IPOPT libraries out of the Vendors/tropter folder and into the base CMakeLists file
Testing I've completed
With OPENSIM_WITH_CASADI = ON and OPENSIM_WITH_TROPER = OFF, ran tests and a few Moco examples through MATLAB successfully
Looking for feedback on...
Local testing on another platform would be helpful since there are different branches for Windows and not-Windows,
CHANGELOG.md (choose one)
- updated.
@nickbianco would you be able to test on your mac?
The build is still looking for ADOL-C when I turn off OPENSIM_WITH_TROPTER. I think this line needs to be surrounded by an if (OPENSIM_WITH_TROPTER) endif().
Ignore the comment above. As per discussion with @carmichaelong, my issue was related to not building all the dependencies for both CasADi and Tropter. The current scope of the PR only covers the CasADi/Tropter CMake flags for the core build, not the dependencies build.
LGTM as long as we don't need to pull/build ipopt if neither OPENSIM_WITH_CASADI nor OPENSIM_WITH_TROPTER are on. If that scenario has been tested feel free to merge. Thanks @carmichaelong
@aymanhab @nickbianco Let's hold off on the merge for now, and I can dive a little more into the combinations of what's checked between dependencies and the main opensim build. If it's easy enough, it'd be good to put that in this PR, otherwise, I think we could still merge since the testing so far shows that it fixes a current problem without introducing other issues (just discovering other related issues).
Thanks again @carmichaelong I don't want to make up new work for you but if our build of dependencies with both flags OFF doesn't require IPOPT to be superbuilt or downloaded then we're good to go.
@aymanhab answer to your questions:
if our build of dependencies with both flags OFF doesn't require IPOPT to be superbuilt or downloaded then we're good to go.
This should already be the case before these changes.
Going to merge this based on feedback at the meeting (thanks @nickbianco and @aymanhab). I've also opened up #3281 and #3282 for other issues discovered while working on this one