Fix dependency handling in CMake config file #971
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Unset cmake module path at exit to avoid interfering with the caller's subsequent find_package calls * Add if (NOT TARGET guards around add_library to avoid creating duplicate targets * Finds OpenMP if acados is built with it
FreyJo
approved these changes
Oct 5, 2023
Hs293Go
added a commit
to Hs293Go/acados
that referenced
this pull request
Apr 24, 2024
Adjusting CMAKE_PREFIX_PATH in acadosConfig.cmake breaks dependency handling by catkin when building a ROS package that depends on acados. A similar path adjustment issue had been fixed by acados#971. Said ad-hoc path adjustment is likely made unnecessary by acados#1065, which introduced installing cmake scripts to standard locations.
Hs293Go
added a commit
to Hs293Go/acados
that referenced
this pull request
Apr 24, 2024
Adjusting CMAKE_PREFIX_PATH in acadosConfig.cmake breaks dependency handling by catkin when building a ROS package that depends on acados. A similar path adjustment issue had been fixed by acados#971.
FreyJo
pushed a commit
that referenced
this pull request
Apr 24, 2024
Adjusting CMAKE_PREFIX_PATH in acadosConfig.cmake breaks dependency handling by catkin when building a ROS package that depends on acados. ## Details ROS's buildtool `catkin` controls `CMAKE_PREFIX_PATH` in the environment to [handle "workspaces"](https://catkin-tools.readthedocs.io/en/latest/verbs/catkin_config.html#explicitly-specifying-workspace-chaining), which collides with `acadosConfig.cmake`'s own path adjustments. Instead of replicating #971's means of reverting path adjustment at exit, I simply deleted the path adjustment command as it seems to serve no purpose. The associated comment suggests an absurd thing - no `*Config.cmake` files could exist in the parent of the `cmake` folder, which could either be root `acados` in case of default install or `/usr/local` in case of global install
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses 3 issues when consuming acados as a CMake dependency from a downstream project.
find_package(acados)breaks all followingfind_packagecalls sinceacadosConfig.cmakeadjusts theCMAKE_MODULE_PATH. This should be restored to the original state per this cmake dev in the manner shown here.add_library(gfortran)andadd_library(openblas)triggers target with the same name already exists errors if the acados config file is processed twice, so they should be guarded against doubleadd_libraryby if guardsACADOS_WITH_OPENMP, the dependency onOpenMPis not handled in the config file