-
Notifications
You must be signed in to change notification settings - Fork 173
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: make include paths consistent #1390
Conversation
tests pass
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.
Some of these are actually system includes, but overall an improvement 👍🏻
@@ -4,7 +4,7 @@ | |||
#include "Game.h" | |||
#include "Logger.h" | |||
|
|||
#include <zlib.h> | |||
#include "zlib.h" |
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.
that could be an actual system dep though?
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.
DarkflameServer/dCommon/CMakeLists.txt
Lines 35 to 64 in 66ac5a1
if (UNIX) | |
find_package(ZLIB REQUIRED) | |
elseif (WIN32) | |
include(FetchContent) | |
# TODO Keep an eye on the zlib repository for an update to disable testing. Don't forget to update CMakePresets | |
FetchContent_Declare( | |
zlib | |
URL https://github.com/madler/zlib/archive/refs/tags/v1.2.11.zip | |
URL_HASH MD5=9d6a627693163bbbf3f26403a3a0b0b1 | |
) | |
# Disable warning about no project version. | |
set(CMAKE_POLICY_DEFAULT_CMP0048 NEW) | |
# Disable warning about the minimum version of cmake used for bcrypt being deprecated in the future | |
set(CMAKE_WARN_DEPRECATED OFF CACHE BOOL "" FORCE) | |
FetchContent_MakeAvailable(zlib) | |
set(ZLIB_INCLUDE_DIRS ${zlib_SOURCE_DIR} ${zlib_BINARY_DIR}) | |
set_target_properties(zlib PROPERTIES INTERFACE_INCLUDE_DIRECTORIES "${ZLIB_INCLUDE_DIRS}") | |
add_library(ZLIB::ZLIB ALIAS zlib) | |
else () | |
message( | |
FATAL_ERROR | |
"This platform does not have a way to use zlib.\nCreate an issue on GitHub with your build system so it can be configured." | |
) | |
endif () | |
target_link_libraries(dCommon ZLIB::ZLIB) |
It isn't
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.
DarkflameServer/dCommon/CMakeLists.txt
Line 36 in 66ac5a1
find_package(ZLIB REQUIRED) |
How is this case not potentially a system path? That package could be found anywhere.
tests pass
Looked through the codebase for
#include "..
and(#include <)(.*)(\.h>)
and updated include paths to what we use across the codebase.