COMP: Improve Findquatlib for different configs on Windows #154
Conversation
Search for quatlib RELEASE and DEBUG libraries and use select_library_configurations(QUATLIB) to be able to link against the adequate library with target_link_libraries.
|
Is select_library_configurations() defined in CMake 2.8.3? If not, probably want to bump the required version the master CMakeLists.txt file to 3.0.2 so we make sure it will be available. |
|
Bumping the required version has non-obvious side effects (it changes cmake policy as well as enforcing a minimum version). My CMake modules https://github.com/rpavlik/cmake-modules have a handy little method of backporting modules, but it does suggest that 2.8.0 contains selectlibraryconfigurations (since only below that version will the directory containing this module be added to the |
| NAMES | ||
| quat.lib | ||
| libquat.a | ||
| HINTS | ||
| PATH_SUFFIXES | ||
| ReleaseAcademicEdition |
There was a problem hiding this comment.
I've never heard of quatlib being built in ReleaseAcademicEdition. This sounds somewhat like a copy-and-paste typo from my "OpenHaptics" module - where they do actually distinguish their Academic Edition version of the SDK folder
| PATH_SUFFIXES | ||
| DebugAcademicEdition |
There was a problem hiding this comment.
Same here - no debug academic edition.
| HINTS | ||
| PATH_SUFFIXES | ||
| ReleaseAcademicEdition | ||
| Release |
There was a problem hiding this comment.
You might, however, consider adding RelWithDebInfo and MinSizeRel after Release.
| PATH_SUFFIXES | ||
| DebugAcademicEdition | ||
| Debug |
There was a problem hiding this comment.
but it's correct here to only have Debug - don't want RelWithDebInfo in there, or strange things may happen when you try to link using MSVC. (The debug C runtime is different, so it's best not to mix them - in some cases, it's entirely forbidden to mix them)
| @@ -101,4 +117,7 @@ else() | |||
| set(QUATLIB_INCLUDE_DIRS) | |||
| endif() | |||
|
|
|||
| mark_as_advanced(QUATLIB_LIBRARY QUATLIB_INCLUDE_DIR) | |||
| mark_as_advanced(QUATLIB_LIBRARY_RELEASE | |||
| QUATLIB_LIBRARY_DEBUG | |||
There was a problem hiding this comment.
On GitHub, this looks way more indented than the rest, which makes me suspect it's using tabs and the rest of the file is using spaces. I'm not super-addicted to a single convention over another, but consistency within a file improves readability. Just double-check (preferably in an editor with a "show whitespace" feature - many have this, on Windows, Notepad2-mod is great for whitespace cleanup) that your changes are internally consistent with the existing code in the file.
| ReleaseAcademicEdition | ||
| Release | ||
| ${_libsuffixes} | ||
| PATHS |
There was a problem hiding this comment.
The previous code distinguished the supplied QUATLIB_ROOT_DIR by putting it under HINTS rather than PATHS - this code changes it to just be the first in PATHS. I suspect this is probably actually the correct convention, but wanted to make sure that you had made this change intentionally for that reason (improving convention) and not confusion or issues regarding PATH_SUFFIXES. PATH_SUFFIXES are applied equally to HINTS, the built-in search paths, and PATHS, in that order, if I remember correctly
| "${_progfiles}/VRPN" | ||
| "${_progfiles}/quatlib" | ||
| C:/usr/local | ||
| /usr/local) | ||
| select_library_configurations(QUATLIB) |
There was a problem hiding this comment.
Was going to say, I think the capitalization is wrong, but double-checked and nope, you got it totally right here - it's the find_package_handle_standard_args one that has the commonly-confused capitalization (where it needs the name of the package, not the prefix of the variables - it will capitalize on its own by default).
Nice work! 👍 😁
Search for quatlib
RELEASEandDEBUGlibraries and useselect_library_configurations(QUATLIB)to be able tolink against the adequate library with
target_link_libraries.