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

Update CMake scripts #1365

Merged
merged 14 commits into from
May 28, 2018
Merged

Update CMake scripts #1365

merged 14 commits into from
May 28, 2018

Conversation

Ghabry
Copy link
Member

@Ghabry Ghabry commented May 15, 2018

Requires the liblcf cmake PR to work.

This updates all the CMake stuff to use targets.

Works for Windows, Linux and MacOSX, including static builds.

Under Windows I have a vcpkg issue: The opus vcxproj file compiles a sample c++ file in which means the main is overwritten when linking static m(. Must be fixed in vcpkg repo.

Simplified the code a bit by making Wildmidi always ON, doesn't matter if SDL2 or SDL2_Mixer is used. Still no love for OpenAL yet ^^'.

Added a new magical function player_find_package to reduce code duplication.

Copy link
Member

@carstene1ns carstene1ns left a comment

Choose a reason for hiding this comment

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

Not tested yet, but looks nice and clean.

CMakeLists.txt Outdated

project(EasyRPG_Player CXX C)
set(PACKAGE_VERSION "0.5.3")
project(EasyRPG_Player VERSION 0.5.3 LANGUAGES CXX C)
Copy link
Member

@carstene1ns carstene1ns May 15, 2018

Choose a reason for hiding this comment

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

Should be safe to remove C here.

Copy link
Member

Choose a reason for hiding this comment

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

ping

CMakeLists.txt Outdated

# Source Files
set(PLAYER_SRCS
add_library(${PROJECT_NAME}_Static
Copy link
Member

Choose a reason for hiding this comment

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

Does cmake still not support having different kinds of things (executable, library) with the same name? 👎 Maybe change the name to player_aux or something.
Also, this might be a shared library without explicit setting STATIC, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

is a static library by default because BUILD_SHARED_LIBS is OFF by default (strange default). But you are right, must force STATIC here.

Is not possible to have two targets with the same name but you can cheat:
https://gist.github.com/jlgerber/eafc4ee2b9954e27dd2bb009496b1b03#file-cmakelists-txt-L13
Will do this to get rid of _Static.

Copy link
Member

Choose a reason for hiding this comment

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

nice, you are already using half of the cheat currently 👍

CMakeLists.txt Outdated
else()
message(STATUS "Resampler: None")
message(STATUS "Resampler: None")
endif()
Copy link
Member

Choose a reason for hiding this comment

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

Maybe align everything with spaces (like the rest)?

Copy link
Member Author

@Ghabry Ghabry May 15, 2018

Choose a reason for hiding this comment

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

This was all really well aligned until fdela came and changed the Ogg playback string to Ogg vorbis playback :P.
Already played around with some alignments (and no alignments). The problem with no spacing at all is that it's harder to read IMHO.
But this change is obivously wrong, is wrong space to tab...... oO

Copy link
Member

Choose a reason for hiding this comment

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

pong

@Ghabry
Copy link
Member Author

Ghabry commented May 18, 2018

good news: Even without vcpkg we can already switch to CMake completely.
I just need to rename pixman to pixman-1, then it works with my hand-rolled buildsystem.
Well and fix some linker errors ;).

@ras0219-msft
Copy link

The opus issue should be fixed in vcpkg master!

@Ghabry
Copy link
Member Author

Ghabry commented May 18, 2018

@ras0219-msft
yepp this is what I planned to do 👍

@carstene1ns
Copy link
Member

Good, waiting for the (hopefully final) rebase.

@Ghabry
Copy link
Member Author

Ghabry commented May 18, 2018

oooh, I understood this wrong. The opus issue was already solved in the vcpkg package repo. Awesome :D

Ghabry added 4 commits May 19, 2018 20:46
…pport before CMake 3.11.

Handle the circular dependency on Harfbuzz in FindFreetype because the INTERFACE linkage is not supported before CMake 3.11.
@Ghabry
Copy link
Member Author

Ghabry commented May 21, 2018

Okay, that's enough (annoying) CMake work for now.

Copy link
Member

@carstene1ns carstene1ns left a comment

Choose a reason for hiding this comment

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

Works as expected.

@carstene1ns carstene1ns merged commit a1ce052 into EasyRPG:master May 28, 2018
@Ghabry Ghabry deleted the cmake branch September 27, 2018 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants