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

Refactor FindHistory.cmake #6654

Merged

Conversation

bencsikandrei
Copy link
Contributor

@bencsikandrei bencsikandrei commented Apr 29, 2022

Hello!

I was super excited to look into wesnoth! Amazing work!

While I was getting to know the project I've read through some of the cmake finders and thought I may help with some modernization.

I've made FindHistory.cmake use lower caps keywords, added an IMPORTED target for it and some documentation that looks like the standard cmake docs.

Please let me know if this kind of stuff is interesting to you as I'll probably also look into the other cmake files during my journey.

Thanks and keep up the great work!
Andrei

EDIT: my editor was configured with spaces. I noticed all other cmake finders use tabs. I'll update accordingly

@github-actions github-actions bot added Building Build-time issues. CMake Issues involving the CMake build system. labels Apr 29, 2022
* use all lower case keywords for consistency
* add target for libhistory: History::History
@Pentarctagon Pentarctagon merged commit 5796ae5 into wesnoth:master Apr 29, 2022
@Pentarctagon
Copy link
Member

Thanks! And as a heads up if you're going to continue looking at other files, the reason there are some differences between Windows/MSVC for how libraries are added is we use vcpkg for our Windows builds which makes things like SDL2::SDL2 available (which are present otherwise).

@bencsikandrei bencsikandrei deleted the refactor_find_history_cmake branch May 1, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Building Build-time issues. CMake Issues involving the CMake build system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants