opensim-core icon indicating copy to clipboard operation
opensim-core copied to clipboard

Fix IPOPT library install with only OPENSIM_WITH_CASADI flag

Open carmichaelong opened this issue 3 years ago • 3 comments

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.

This change is Reviewable

carmichaelong avatar Aug 12 '22 04:08 carmichaelong

@nickbianco would you be able to test on your mac?

carmichaelong avatar Aug 12 '22 20:08 carmichaelong

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().

nickbianco avatar Aug 12 '22 23:08 nickbianco

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.

nickbianco avatar Aug 15 '22 22:08 nickbianco

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 avatar Aug 15 '22 22:08 aymanhab

@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).

carmichaelong avatar Aug 15 '22 23:08 carmichaelong

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 avatar Aug 16 '22 19:08 aymanhab

@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

carmichaelong avatar Aug 16 '22 23:08 carmichaelong