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

Added CMake support for Visual Studio/Xcode/Android generator #1594

Merged
merged 27 commits into from
Oct 10, 2016
Merged

Added CMake support for Visual Studio/Xcode/Android generator #1594

merged 27 commits into from
Oct 10, 2016

Conversation

chaoticbob
Copy link
Contributor

Updated Windows platform specific CMake related files to build on Windows. Corrected some of the additions of preprocessor definitions. Updated ci_make_app macro to generate the right settings for apps. Tested against Cube, CubeMapping, ShadowMapping, and Extrude on Visual Studio 2013 and Visual Studio 2015.

@chaoticbob chaoticbob changed the title Added CMake support for Visual Studio generator on Windows Added CMake support for Visual Studio/Xcode/Android generator Oct 3, 2016
@@ -1,6 +1,15 @@
cmake_minimum_required( VERSION 3.0 FATAL_ERROR )
set( CMAKE_VERBOSE_MAKEFILE ON )

# Configure the Android toolchain before the project start
if( CINDER_TARGET )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be a part of configure.cmake, which sets up the CINDER_TARGET along with other important variables used on all platforms.

But perhaps the cinderAndreoidToolchain.cmake must be included before the call to project( cinder )? If that is the case, I think that the project() call can be moved to after the include( configure.cmake ).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The compiler checks seem to happen when you project() is called. I think on most platform this is fine, but on Android since we have to do a manual toolchain setup, it needs to happen before project(). Certainly not married to this solution, so if we can move stuff around to make it more sane, would be great.

Copy link
Contributor

Choose a reason for hiding this comment

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

I seems that the proper ( in terms of CMake ) way to do this would be using CMake's toolchain functionality. See here and here. This functionality would also be interesting to explore in the case of Linux / RPi .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, forgot about the toolchain feature. I'll move this to a toolchain file approach over the weekend.

# Force multiprocess compilation
add_compile_options( /MP )
# Add necessary platform libraries
list( APPEND PLATFORM_LIBRARIES "zlib.lib;shlwapi.lib;OpenGL32.lib" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

On OS X, we handled this as part of libcinder's target_link_libraries() (here) Its pretty nice this way because then anyone who does a find_package( cinder ) will get linked to the required libraries, even if they aren't using ci_make_app() (like if they already have a different cmake setup).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try moving it to target_link_libraries - I can't remember right now if there was a specific reason for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this to a more explicit location: 76f8781

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated this to be deps on lib cinder: c47e1de

# Add lib dirs
cmake_policy( PUSH )
cmake_policy( SET CMP0015 OLD )
link_directories( "${CINDER_PATH}/${CINDER_LIB_DIRECTORY}" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in general @PetrosKataras was advocating that we use the target_* variants as much as possible. Is there a way to do this with link_directories() too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked around for a bit and could not find a way. Happy to change if you guys have a suggestion.

Copy link
Contributor

@PetrosKataras PetrosKataras Oct 4, 2016

Choose a reason for hiding this comment

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

There is no target_* variant for link_directories but what I am actually wondering is if this is needed at all. In theory ( and in practice as it happens for Linux / OS X for example ) the previous call to find_package( cinder ) together with the target_link_libraries( myapp cinder ) that follows should take care of finding and linking with Cinder without the need for manually specifying link directories.. But maybe this is not the case for the Visual Studio generator?

Copy link
Contributor Author

@chaoticbob chaoticbob Oct 4, 2016

Choose a reason for hiding this comment

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

CMake has historically had issues with the rules imposed by MSBuild and build systems like Jam that it has to do some weird jumping around. For instance the output path or the ignoring specific library. We have to override these and or put them in as forced flags. link_directories make it easier to deal with things like this: libboost_filesystem-vc120-mt-sgd-1_60.lib. This leverages the auto linking feature in Visual C++ and doesn't lock you into a specific build of Boost - especially one that's tagged with a version number. In the near future we won't have to deal with this because we won't have a dependency on Boost. But for now, short of attempting to detect the version of Boost, it's easier to let Visual C++ handle it for you by using link_directories.

Copy link
Contributor Author

@chaoticbob chaoticbob Oct 4, 2016

Choose a reason for hiding this comment

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

Just noticed that some static libs on Windows can be added into the cinder.lib and we don't have to explicitly add them to the app target. It's not clear to me how these are propagated or inherited.

Let me try moving some of this around to see if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, moved what I could so find_package( cinder ) picks up on the deps.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't have a strong opinion regarding link_directories as long as we can keep the usage of it just for edge cases like this one. I just look forward to moving away from Boost as a dependency..

Copy link
Contributor Author

@chaoticbob chaoticbob Oct 5, 2016

Choose a reason for hiding this comment

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

We don't really have an option on Windows for this. Unless we want to glob and parse the Boost libraries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I actually implemented the globbing and it worked. But then I realized that it would make it really difficult for the CMake generated lib to interact with the handmade project files. So it seems like link_directories() is the most reasonable thing to do for now.

@@ -24,11 +24,12 @@ target_include_directories( cinder SYSTEM BEFORE INTERFACE ${CINDER_INCLUDE_SYST
target_include_directories( cinder BEFORE PRIVATE ${CINDER_INCLUDE_USER_PRIVATE} )
target_include_directories( cinder SYSTEM BEFORE PRIVATE ${CINDER_INCLUDE_SYSTEM_PRIVATE} )

target_link_libraries( cinder PUBLIC ${CINDER_LIBS_DEPENDS} )
target_link_libraries( cinder PUBLIC ${CINDER_LIBS_DEPENDS} )
target_link_libraries( cinder LINK_PRIVATE Ws2_32.lib )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line should be removed since a) it would 'leak' to non-windows platforms and b) it has been integrated in platform_msw.cmake as part of CINDER_LIBS_DEPENDS here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that was apart of my experimenting to find some better way to propagate the link flags. It's been removed now.

set( CINDER_ANDROID_NDK_ARCH "armeabi-v7a" CACHE STRING "Android NDK target architecture." )

set( CINDER_TARGET_SUBFOLDER "android-${CINDER_ANDROID_NDK_PLATFORM/CINDER_ANDROID_NDK_ARCH" )
#set( CINDER_ANDROID_NDK_PLATFORM 21 CACHE STRING "Android NDK Platform version number." )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't we want to expose these variables to the cmake cache somehow?

Also, I do like that they are in configure.cmake as this file is shared by both libcinder and app builds.


target_compile_definitions( cinder PUBLIC ${CINDER_DEFINES} )

# Visual Studio and Xcode generators adds a ${CMAKE_BUILD_TYPE} to the ARCHIVE
# and LIBRARY directories. Override the directories so, ${CMAKE_BUILD_TYPE} doesn't double up.
if( CINDER_MSW )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this live in platform_msw.cmake?

STATIC_LIBRARY_FLAGS_MINSIZEREL "${CINDER_STATIC_LIBS_FLAGS_RELEASE} ${CINDER_STATIC_LIBS_DEPENDS_RELEASE}"
STATIC_LIBRARY_FLAGS_RELWITHDEBINFO "${CINDER_STATIC_LIBS_FLAGS_RELEASE} ${CINDER_STATIC_LIBS_DEPENDS_RELEASE}"
)
elseif( CINDER_MAC )
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this live in platform_macosx.cmake?

endif()

add_executable( ${ARG_APP_NAME} MACOSX_BUNDLE WIN32 ${ARG_SOURCES} ${ICON_PATH} ${ARG_RESOURCES} )

target_include_directories( ${ARG_APP_NAME} PUBLIC ${ARG_INCLUDES} )
target_link_libraries( ${ARG_APP_NAME} cinder ${ARG_LIBRARIES} )

# Ignore Specific Default Libraries
if( MSVC )
set_target_properties( ${ARG_APP_NAME} PROPERTIES LINK_FLAGS "/NODEFAULTLIB:LIBCMT /NODEFAULTLIB:LIBCPMT" )
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we set this on the cinder target, won't they get picked up as the app depends on that package? That way we can keep them grouped together.

set( CINDER_STATIC_LIBS_DEPENDS "${MACOS_SUBFOLDER}/libboost_filesystem.a ${MACOS_SUBFOLDER}/libboost_system.a ${MACOS_SUBFOLDER}/libz.a" )

if( NOT ( "Xcode" STREQUAL "${CMAKE_GENERATOR}" ) )
if(NOT CMAKE_LIBTOOL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add comment as to why we need to use libtool instead of ar / ranlib (partially because that's what xcode does).

@richardeakin
Copy link
Collaborator

Merging this as it is getting long and the remaining notes are mostly clean up, can be done afterwards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants