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: make include paths consistent #1390

Merged
merged 2 commits into from
Jan 5, 2024
Merged

fix: make include paths consistent #1390

merged 2 commits into from
Jan 5, 2024

Conversation

EmosewaMC
Copy link
Collaborator

@EmosewaMC EmosewaMC commented Jan 5, 2024

tests pass

Looked through the codebase for #include ".. and (#include <)(.*)(\.h>) and updated include paths to what we use across the codebase.

@EmosewaMC EmosewaMC changed the title fix: bad header includes fix: dont include game headers in common tests Jan 5, 2024
Xiphoseer
Xiphoseer previously approved these changes Jan 5, 2024
@EmosewaMC EmosewaMC marked this pull request as ready for review January 5, 2024 10:28
@EmosewaMC EmosewaMC changed the title fix: dont include game headers in common tests fix: make include paths consistent Jan 5, 2024
Copy link
Contributor

@Xiphoseer Xiphoseer left a 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"
Copy link
Contributor

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

find_package(ZLIB REQUIRED)

How is this case not potentially a system path? That package could be found anywhere.

@aronwk-aaron aronwk-aaron merged commit 2804dc3 into main Jan 5, 2024
4 checks passed
@aronwk-aaron aronwk-aaron deleted the fixheaders branch January 5, 2024 12:34
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.

4 participants