Skip to content

Use CMake and support clients using CMake#5

Merged
TylerGlaiel merged 2 commits into
TylerGlaiel:mainfrom
friendlyanon:cmake
Jul 12, 2023
Merged

Use CMake and support clients using CMake#5
TylerGlaiel merged 2 commits into
TylerGlaiel:mainfrom
friendlyanon:cmake

Conversation

@friendlyanon
Copy link
Copy Markdown
Contributor

Fixes #3 and some compiler warnings. Moved WIN32_LEAN_AND_MEAN to the build system.

I have also seen someone wanting to add Linux support, which should be easy by just extending the if().

For anyone wanting to add tests to this, you'll just have to add these lines at the end:

include(CTest)
if(BUILD_TESTING)
  add_executable(CrashlogsTests test.cpp)
  target_link_libraries(CrashlogsTests PRIVATE Crashlogs)
  add_test(NAME CrashlogsTests COMMAND CrashlogsTests)
endif()

Lines 12-16 make it possible for users to consume this project like so:

find_package(Crashlogs REQUIRED)
target_link_libraries(Dog PRIVATE Crashlogs::Crashlogs)
#include <crashlogs.h>

...

Vendoring via add_subdirectory (or anything that wraps it) is NOT supported. That would require even more code for no good reason.

@friendlyanon
Copy link
Copy Markdown
Contributor Author

Oops, sticky shift key.

@TylerGlaiel
Copy link
Copy Markdown
Owner

TylerGlaiel commented Jul 6, 2023

leave WIN32_LEAN_AND_MEAN in the file and I'll add it (reasoning: cmake should be completely optional)

_CRT_SECURE_NO_WARNINGS still has to be left in the build system,
because the localtime function is declared via <string>, which comes
from the header, but it is a Windows specific definition that should not
be present in the header file.
@friendlyanon
Copy link
Copy Markdown
Contributor Author

Would also be worth seeing where #4 goes. I could just rebase and adapt changes from that instead of having to do 2 PRs.

@TylerGlaiel
Copy link
Copy Markdown
Owner

sure, cmake is entirely irrelevant to me so if you wanna wait till mac/linux support is in to add it then let me know. im not gonna maintain cmakelists.txt myself, someone will have to update it when new platforms are added

@friendlyanon
Copy link
Copy Markdown
Contributor Author

Seems the other PR went dormant. You could accept this then I can review CMake related changes in the other PR if it ever gets picked back up.

@TylerGlaiel TylerGlaiel merged commit 4e61afa into TylerGlaiel:main Jul 12, 2023
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.

Interest in Adding a Build System?

2 participants