cmake: properly find libsamplerate #1678
Conversation
hlolli
force-pushed
the
feature/cmake-find-libsamplerate
branch
2 times, most recently
from
12ae0b0
to
54e8b97
Compare
hlolli
force-pushed
the
feature/cmake-find-libsamplerate
branch
from
54e8b97
to
8efff2e
Compare
CMakeLists.txt
Outdated
| @@ -205,6 +207,11 @@ else() | |||
| message(STATUS "Not using Custom.cmake file.") | |||
| endif() | |||
|
|
|||
| if(LIBSAMPLERATE_LIBRARY AND LIBSAMPLERATE_INCLUDE_DIRECTORY) | |||
| set(LIBSAMPLERATE_FOUND TRUE) | |||
There was a problem hiding this comment.
This should be set in the find module:
csound/cmake/Modules/FindEIGEN3.cmake
Line 66 in a1580f9
So you import it along with the rest of the set or unset vars. You should avoid using include_ family of functions and use target_ only to avoid including these paths for all targets.
There was a problem hiding this comment.
I did that initially, the problem I encountered, is that I am defining the location of LIBSAMPLERATE_INCLUDE_DIRECTORY in my custom.cmake file. Since it says in the comment where the custom.cmake file is imported https://github.com/csound/csound/blob/develop/CMakeLists.txt#L197-L199 that the idea of it is to override variables from cmake after find_/locate_ operations are done, then it means that the value of LIBSAMPLERATE_INCLUDE_DIRECTORY wont exist until later.
I am aware that this is a terrible excuse. I'm just worried about needing to resort to -DVar.. flags for this, which is bit annoying.
The correct solution, in my opinion, is that custom.cmake will be placed at the top (or very close to it) and that way, the developers don't get access to "cheat-mode", because if cmake doesn't resolve the locations correctly when the custom file is evaluated at the beginning, it will also translate to non reproduce-able situations when we are trying to debug issues from users.
With custom.cmake at the top, I'd be totally open for having a second custom cmake file, called something like, overrides.cmake, that gets called after all variables are set.
The second point you made, preferring target_ over include_, totally, I will fix :)
There was a problem hiding this comment.
I'm thinking to just not have a _FOUND variable and check for both vars existing, it willsuffice until we can rethink (or not) the order of custom cmake import.
There was a problem hiding this comment.
Afaik the idea of custom.cmake is mostly for setting feature flags for the app. I don't think you should be overriding find_ module vars this way. You can add a path hint maybe.
|
do we still need this? |
I think it makes sense to keep the cmake module pattern for finding the libraries?
Note that the file util/CMakeLists.txt was DOS formatted, so whenever I changed it I kept getting ^M line endings, so I converted it to unix if that's ok?
closes: #1677