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

Fix CMakeLists missing GLEW and typos for CMake #797

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

philipandresen
Copy link

@philipandresen philipandresen commented Oct 5, 2023

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:

  1. Added the find_package, include_directories, and target_link_libraries calls for GLEW which were previously omitted.
  2. Add language standard flags for the CMake release and debug builds

The cosmetic improvements:

  1. Added a readme for building on CLion
  2. Added a Jetbrains project directory line to the gitignore.

The not-completely-necessary-but-prudent fixes

  1. Fixed a missing dollar sign on the environment variable reference for GAME_ENABLED.
  2. Changed INCLUDE(FindZLIB) to find_package(ZLIB QUIET REQUIRED) on the advice of CMake warnings. Functionally it is identical except the Quiet and Required bits are bonus perks for using the more powerful find_package.
  3. Changed RAPID_JSON lib variable name to RapidJson to match the casing of the FindRapidJson 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.

Small fixes required to successfully build on CLion
@philipandresen philipandresen marked this pull request as ready for review October 5, 2023 00:29
Stops the "clamp isn't a thing" errors
INCLUDE(FindZLIB)
find_package(ZLIB QUIET REQUIRED)
Copy link
Author

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.

Comment on lines 28 to 29
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 )
Copy link
Author

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.

Copy link
Contributor

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

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.

2 participants