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

CMake configuration for Visual Studio #42

Merged
merged 30 commits into from
Feb 26, 2018

Conversation

thomthom
Copy link
Contributor

@thomthom thomthom commented Feb 24, 2018

This PR sets up a CMake project to generate a Visual Studio solution. It require at least CMake 3.8.

To generate a solution:

mkdir build
cd build
CMake .. -G "Visual Studio 15 2017 Win64"

If making adjustments to the CMake configuration, update the VS solution by right-clicking the ALL_BUILD project and build only that.

It creates a solution with the following projects:

  • GoogleTest as a static lib
  • SketchUpAPICpp as a static lib
  • SketchUpAPITests as a console app linking to GoogleTest and SketchUpAPICpp

I also nuked the old manual VS project I created along with the Xcode project.

The main CMake configuration is in root of the project CMakeLists.txt. This refer to various other .cmake files under a cmake directory. I created a .cmake per VS project.

It also adds a third-party directory for GoogleTest and SLAPI. I did not move the existing SLAPI .framework files. Leaving this for the XCode work of the CMake configuration.

After this PR we need to tweak it to allow for generation of Xcode projects.

@thomthom thomthom mentioned this pull request Feb 24, 2018
@jimfoltz
Copy link
Contributor

We are using release 1.8.0 of GoogleTest which is a year and a half old compared to the latest master branch, which builds with no warnings and no need for special config. Just checkout the master branch in git if you'd like to use a more recent although presumably less stable version.

@thomthom
Copy link
Contributor Author

thomthom commented Feb 24, 2018 via email

@thomthom
Copy link
Contributor Author

thomthom commented Feb 24, 2018 via email

@thomthom
Copy link
Contributor Author

Looks like I deleted some files right before making the pull request.

I'll fix that tomorrow.

@thomthom
Copy link
Contributor Author

Fix the branch - the files wasn't missing, but .gitignore excluded them so I didn't notice while I was building on my machine.

@thomthom
Copy link
Contributor Author

Tommy, any specific reasons for these files to be ignored? Usually easier to just ignore the build directories.

# Prerequisites
*.d

# Compiled Object files
*.slo
*.lo
*.o
*.obj

# Precompiled Headers
*.gch
*.pch

# Compiled Dynamic libraries
*.so
*.dylib

# Fortran module files
*.mod
*.smod

# Compiled Static libraries
*.lai
*.la
*.a

# Executables
*.exe
*.out
*.app

@jimfoltz
Copy link
Contributor

jimfoltz commented Feb 25, 2018

There should be an option to skip building googletest and the tests in the CMakeLists.txt file

Then the tests and googletest should only be built when this option is set. This way consumers of the wrapper library only get the library without also getting the tests and google tests in their projects.

Proposed:

option(DEVELOPER "Set this when developing the wrapper library itself" OFF)
# ...
include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/SketchUpAPICpp.cmake)
if(DEVELOPER)
	include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/GoogleTest.cmake)
	include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/SketchUpAPITests.cmake)
endif(DEVELOPER)

Here is how I am adding the wrapper library from my program in a separate folder:

include_directories("../Wrapper/include" "${SKETCHUP_HEADERS}")
# CppWrapper
add_subdirectory(../Wrapper ${CMAKE_CURRENT_BINARY_DIR}/Wrapper)
add_executable(skpinfo
	src/main.cpp
	src/output.hpp
	src/print_dictionary.hpp
)
target_link_libraries(skpinfo SketchUpAPICpp ${SKETCHUP_LIBRARY})

# namespace and TR1-only machinery are deprecated and will be REMOVED. You can
# define _SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING to acknowledge that you
# have received this warning.
add_definitions(-D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want this to apply to all of the folowing targets, or just GoogleTest? It should be appended to the GoogleTest.cmake file and made so it only acts on GoogleTest.

...
add_library(GoogleTest STATIC third-party/googletest/googletest/src/gtest-all.cc)
target_compile_definitions(GoogleTest PRIVATE -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING)
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It could probably be omitted for the wrapper lib, but the Tests project also needs it as including the GoogleTest headers cause warnings also there.

@thomthom
Copy link
Contributor Author

There should be an option to skip building googletest and the tests in the CMakeLists.txt file

Sounds good to me.

@thomthom
Copy link
Contributor Author

There should be an option to skip building googletest and the tests in the CMakeLists.txt file
On second thought, is it really needed? The Visual Studio solution will generate separate projects for GoogleTest and the test project. One can easily build only the Wrapper lib.

Similary with generated Xcode projects from CMake. How much use is it really to make CMake not generate build targets for the tests and GoogleTest?

@thomthom
Copy link
Contributor Author

(Btw, we can probably sort that out in a separate PR. Better to just get this large PR commited before it gets stale, IMO.)

@TommyKaneko TommyKaneko merged commit 0395119 into TommyKaneko:master Feb 26, 2018
@TommyKaneko
Copy link
Owner

There you go, merged. Hope nothing breaks.

@jimfoltz
Copy link
Contributor

Similary with generated Xcode projects from CMake. How much use is it really to make CMake not generate build targets for the tests and GoogleTest?

It’s useful when someone just wants to use the library and not hack on it.

@jimfoltz
Copy link
Contributor

The way the cmake files are written, any project using the library also gets the tests and google test as sub projects.

@thomthom
Copy link
Contributor Author

I'm not sure I fully understand what you mean by using the library in this context. If you pull the repo as-is, you use cmake to generate the VS solution - and if you only care for the Wrapper lib (the static library) then you compile only that project. And you can link that directly.

Is this related to using add_subdirectory from another project's CMake files? (I'm completely green to CMake workflows.)

@TommyKaneko
Copy link
Owner

I have to admit I'm a little lost on CMake, so not sure how much I can contribute. I have got as far as figuring out that this:
https://www.johnlamp.net/cmake-tutorial-2-ide-integration.html#section-MacOsX

... does not generate an Xcode project. Am slightly missing my old Xcode file at the moment. Would like a steer on how to get Xcode working with this.

@jimfoltz
Copy link
Contributor

It's OK as is. Using include() instead of add_subdirectory() means everything is global and applies to all targets instead of being able to localize includes and definitions, and options for each target. We can tweak it as we go.

I didn't mean to suggest cmake replace the current Xcode project or VS solution, I only meant it would be a nice option to have. I was looking at it from the point of view of a consumer of the library. The main purpose of having a cmake file from this perspective was not to make it easy to develop the library itself, but to make it easy to use the library.

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.

3 participants