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.

Sign up for GitHub

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

Jump to bottom

COMP: Improve Findquatlib for different configs on Windows #154

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

COMP: Improve Findquatlib for different configs on Windows #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
25 changes: 22 additions & 3 deletions cmake/Findquatlib.cmake
View file
Open in desktop
Expand Up @@ -65,19 +65,35 @@ else()
/usr/local)

# Look for the library.
find_library(QUATLIB_LIBRARY
find_library(QUATLIB_LIBRARY_RELEASE
NAMES
quat.lib
libquat.a
HINTS
PATH_SUFFIXES
ReleaseAcademicEdition
Copy link
Member

Choose a reason for hiding this comment

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

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

Release
Copy link
Member

Choose a reason for hiding this comment

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

You might, however, consider adding RelWithDebInfo and MinSizeRel after Release.

${_libsuffixes}
PATHS
Copy link
Member

Choose a reason for hiding this comment

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

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

"${QUATLIB_ROOT_DIR}"
"${_progfiles}/VRPN"
"${_progfiles}/quatlib"
C:/usr/local
/usr/local)
find_library(QUATLIB_LIBRARY_DEBUG
NAMES
quat.lib
libquat.a
PATH_SUFFIXES
DebugAcademicEdition
Copy link
Member

Choose a reason for hiding this comment

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

Same here - no debug academic edition.

Debug
Copy link
Member

Choose a reason for hiding this comment

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

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)

${_libsuffixes}
PATHS
"${QUATLIB_ROOT_DIR}"
"${_progfiles}/VRPN"
"${_progfiles}/quatlib"
C:/usr/local
/usr/local)
select_library_configurations(QUATLIB)
Copy link
Member

Choose a reason for hiding this comment

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

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! 👍 😁

endif()

# handle the QUIETLY and REQUIRED arguments and set QUATLIB_FOUND to TRUE if
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

QUATLIB_LIBRARY
QUATLIB_INCLUDE_DIR)