-
Notifications
You must be signed in to change notification settings - Fork 130
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
Fix CMakeLists missing GLEW and typos for CMake #797
base: master
Are you sure you want to change the base?
Fix CMakeLists missing GLEW and typos for CMake #797
Conversation
Small fixes required to successfully build on CLion
Stops the "clamp isn't a thing" errors
INCLUDE(FindZLIB) | ||
find_package(ZLIB QUIET REQUIRED) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caused a compiler warning.
Using "Include" here feels like using eval("x = x + 1")
. Just a tiny code smell. I didn't find many opinions online but the few I found tended to agree
The warning given when using INCLUDE(FindZLIB)
:
CMake Warning (dev) at ...FindPackageHandleStandardArgs.cmake:438 (message):
The package name passed to `find_package_handle_standard_args` (ZLIB) does
not match the name of the calling package (PNG). This can lead to problems
in calling code that expects `find_package` result variables (e.g.,
`_FOUND`) to follow a certain pattern.
find_package_handle_standard_args(RAPID_JSON DEFAULT_MSG RAPID_JSON_INCLUDE_DIR ) No newline at end of file | ||
find_package_handle_standard_args(RapidJson DEFAULT_MSG RAPID_JSON_INCLUDE_DIR ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixes the following Compiler warning:
CMake Warning (dev) at .../FindPackageHandleStandardArgs.cmake:438 (message):
The package name passed to `find_package_handle_standard_args` (RAPID_JSON)
does not match the name of the calling package (RapidJson). This can lead
to problems in calling code that expects `find_package` result variables
(e.g., `_FOUND`) to follow a certain pattern.
The variables automatically set by this call (Eg: RAPID_JSON_FOUND) aren't used - from what I can see - so this change shouldn't have any effect on the build. I tested it with the windows build obviously, but, looking through the build pipeline I didn't see any references that would be broken by this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will take a look on Linux
Small fixes - a few that were benign, and two that were required to successfully build on CLion.
Most of the diff is the Readme I put together to help the next person that tries this on CLion. The readme can obviously be excluded if y'all would prefer not to have it on the official repo.
The necessary fixes:
find_package
,include_directories
, andtarget_link_libraries
calls for GLEW which were previously omitted.The cosmetic improvements:
The not-completely-necessary-but-prudent fixes
Quiet
andRequired
bits are bonus perks for using the more powerfulfind_package
.RAPID_JSON
lib variable name toRapidJson
to match the casing of theFindRapidJson
filename as recommended by the compiler.But after these changes everything compiles just fine with zero warnings. Tested with debug / release builds on x64 windows.
Happy to re-submit with only the necessary fixes if this is too much.