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

CMake update #1065

Merged
merged 5 commits into from
Apr 4, 2024
Merged

CMake update #1065

merged 5 commits into from
Apr 4, 2024

Conversation

Hs293Go
Copy link
Contributor

@Hs293Go Hs293Go commented Apr 4, 2024

  • Make including QORE directories conditional: Addresses Nonexistent include directories attached to acados cmake target #1062
  • Replace find_package with modern find_dependency in acadosConfig.cmake to let acados propagate find_package args like REQUIRED
  • Installs acados's own Find*.cmake scripts to let them be found if the user installs to non-default locations by tweaking ACADOS_INSTALL_DIR
  • Remove unused FindNumpy.cmake, FindMatlab.cmake

The value of ACADOS_WITH_QORE toggles whether qore include directories
are attached to the acados target's. This stops projects not using
qore from getting missing-include-dir warnings
- Lets acadosConfig.cmake find OOQP dependencies OpenBLAS and
  FortranLibs only if ACADOS_WITH_OOQP is set.

- Swaps find_package with modern find_dependency in acadosConfig.cmake
  to let acados propagate find_package args like REQUIRED

- Installs acados's own Find*.cmake scripts to let them be found if the
  user installs to non-default locations by tweaking ACADOS_INSTALL_DIR
@Hs293Go
Copy link
Contributor Author

Hs293Go commented Apr 4, 2024

A second part of this PR addresses a similar issue of optional dependencies leaking and showing warnings to users that consume acados, i.e. find_package(FortranLibs) and find_package(OpenBLAS) are unconditional inside acadosConfig.cmake.in

https://github.com/acados/acados/blob/66bd329cd7b512bda697abb8cb43401bfb15361a/cmake/acadosConfig.cmake.in#L73C1-L73C23

even though they appear to be needed by ooqp only.

Furthermore, find_package calls inside acadosConfig.cmake.in is modernized to find_dependency calls such that find_package args like REQUIRED and QUIET will be propagated.

Finally, logic is added in acados/CMakeLists.txt to install FindFortranLibs.cmake and FindOpenBLAS.cmake (and other Find... scripts). Otherwise these scripts will not be found if the user tweaks ACADOS_INSTALL_DIR and installs acados to other locations, e.g. the system /usr/local

Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!
I left two comments.

acados/CMakeLists.txt Outdated Show resolved Hide resolved
acados/CMakeLists.txt Show resolved Hide resolved
@FreyJo
Copy link
Member

FreyJo commented Apr 4, 2024

Just a note: In acados, we decided to squash merge the PRs and use the PR description as the commit message.
Thus, it would be nice to update the PR description to contain more detailed information as in your nice commit messages :)

@Hs293Go
Copy link
Contributor Author

Hs293Go commented Apr 4, 2024

I complied with the changes you suggested. I had been conservative in wanting to minimize diffs.

Changes were divided in two commits was because initially I wanted to just fix qore include dirs only. But then I skimmed the forum and issues and found #1047 and some other issues about people wanting to tweak their Acados install location e.g. system-wide install to /usr/local, which would require cooperation from find_package. I judged the time is ripe and took the liberty to squeeze in a second commit in this PR.

@FreyJo FreyJo changed the title Makes including qore directories conditional CMake update Apr 4, 2024
Copy link
Member

@FreyJo FreyJo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the PR description, looks all good now 👍

@FreyJo FreyJo merged commit 14559b2 into acados:master Apr 4, 2024
5 checks passed
FreyJo pushed a commit that referenced this pull request Apr 23, 2024
Although most of the issues with using acados in another cmake project
(#1025) have been fixed with
#1065, there are still a few lines
missing in acadosConfig.cmake.in.

---------

Co-authored-by: Franek Stark <franek.stark@dfki.de>
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#953. 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. Said ad-hoc path
adjustment is likely made unnecessary by acados#1065, which introduced
installing cmake scripts to standard locations.
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.

None yet

2 participants