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

[RDY] CMake deps handling - continuation of pr #1261 #1274

Merged
merged 4 commits into from Nov 11, 2017

Conversation

Projects
None yet
4 participants
@TheCycoONE
Member

TheCycoONE commented Nov 5, 2017

Compared to #1261 this PR:

  • Handles SDL2 on multiple configuration targets when using built in vcpkg, by using the provided sdl2 cmake config.
  • Doesn't modify FindSDL2.cmake (those modifications didn't work)
  • Copies lua files from the vcpkg/installed/**/share/lua/* directory
  • Checks out the correct commit when cloning vcpkg the first time
  • Checks out a newer vcpkg commit, containing luasockets and newer sdl packages
  • Fixes whitespace
  • Modifies appveyor to take advantage of these changes
@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Nov 6, 2017

I'd like to help you use the toolchain file if possible!

Note that CMAKE_TOOLCHAIN_FILE only appears to take effect during the first call to project(), so if you want to download the prebuilt libraries, you could do that first:

if(NOT CMAKE_TOOLCHAIN_FILE)
  # download stuff here
  set(CMAKE_TOOLCHAIN_FILE /downloaded/path/to/vcpkg.cmake CACHE STRING "")
endif()

cmake_minimum_required(VERSION 3.8)
project(foo CXX)

message(STATUS "VCPKG_TARGET_TRIPLET=${VCPKG_TARGET_TRIPLET}")

Also, the architecture of the Visual Studio generator can be controlled with -A x64.

ras0219-msft commented Nov 6, 2017

I'd like to help you use the toolchain file if possible!

Note that CMAKE_TOOLCHAIN_FILE only appears to take effect during the first call to project(), so if you want to download the prebuilt libraries, you could do that first:

if(NOT CMAKE_TOOLCHAIN_FILE)
  # download stuff here
  set(CMAKE_TOOLCHAIN_FILE /downloaded/path/to/vcpkg.cmake CACHE STRING "")
endif()

cmake_minimum_required(VERSION 3.8)
project(foo CXX)

message(STATUS "VCPKG_TARGET_TRIPLET=${VCPKG_TARGET_TRIPLET}")

Also, the architecture of the Visual Studio generator can be controlled with -A x64.

@TheCycoONE TheCycoONE changed the title from CMake deps handling - continuation of pr #1261 to [RFC] CMake deps handling - continuation of pr #1261 Nov 7, 2017

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 7, 2017

Member

@ras0219-msft I played for awhile tonight trying to use the CMAKE_TOOLCHAIN_FILE like you suggest, but so far haven't been able to turn it into a good experience.

I'll try again tomorrow, and document what I'm seeing then if I still can't get it to work.

Member

TheCycoONE commented Nov 7, 2017

@ras0219-msft I played for awhile tonight trying to use the CMAKE_TOOLCHAIN_FILE like you suggest, but so far haven't been able to turn it into a good experience.

I'll try again tomorrow, and document what I'm seeing then if I still can't get it to work.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 8, 2017

Member

@ras0219-msft The first problem I run into with that approach is that MSVC isn't set before PROJECT, so I don't know if we are in an appropriate environment to use VCPKG.

Member

TheCycoONE commented Nov 8, 2017

@ras0219-msft The first problem I run into with that approach is that MSVC isn't set before PROJECT, so I don't know if we are in an appropriate environment to use VCPKG.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 8, 2017

Member

@ras0219-msft Disregard the above - I've gotten considerably closer.

Now the issue I run into when I use the vcpkg toolchain is:

Building CorsixTH
CMake Error at vcpkg/scripts/buildsystems/vcpkg.cmake:159 (_find_package):
  Could not find a package configuration file provided by "SDL2" with any of
  the following names:

    SDL2Config.cmake
    sdl2-config.cmake

  Add the installation prefix of "SDL2" to CMAKE_PREFIX_PATH or set
  "SDL2_DIR" to a directory containing one of the above files.  If "SDL2"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CorsixTH/CMakeLists.txt:97 (FIND_PACKAGE)

Compared to the code I just committed, this was my changes:

diff --git a/CMake/VcpkgDeps.cmake b/CMake/VcpkgDeps.cmake
index 08933a19..1d778194 100644
--- a/CMake/VcpkgDeps.cmake
+++ b/CMake/VcpkgDeps.cmake
@@ -56,23 +56,4 @@ if(err_val)
       "\nIf this error persists try deleting the 'vcpkg' folder.\n")
 endif()

-# We cannot use a toolchain file at this point despite it being recommended by MS.
-# The arch is determined by the generator in use on Windows. For example
-# Visual Studio xx Win64 <= implies 64 bit build. If we use a toolchain this
-# always defaults to a 32 bit build.
-
-# If the user specified their own generator it is too late at this point.
-# We would need to restart CMake at this point, delete the
-# cache and invoke CMake again with the toolchain set.
-
-# For both of these reasons we will use CMAKE_PREFIX_PATH
-
-set(VCPKG_INSTALLED_PATH ${VCPKG_PARENT_DIR}/vcpkg/installed/${VCPKG_TARGET_TRIPLET})
-
-if(CMAKE_BUILD_TYPE MATCHES "^Debug$" OR NOT DEFINED CMAKE_BUILD_TYPE)
-  list(APPEND CMAKE_PREFIX_PATH ${VCPKG_INSTALLED_PATH}/debug)
-  list(APPEND CMAKE_LIBRARY_PATH ${VCPKG_INSTALLED_PATH}/debug/lib/manual-link)
-endif()
-
-list(APPEND CMAKE_PREFIX_PATH ${VCPKG_INSTALLED_PATH})
-list(APPEND CMAKE_LIBRARY_PATH ${_VCPKG_INSTALLED_DIR}/lib/manual-link)
+set(CMAKE_TOOLCHAIN_FILE ${VCPKG_PARENT_DIR}/vcpkg/scripts/buildsystems/vcpkg.cmake)
Member

TheCycoONE commented Nov 8, 2017

@ras0219-msft Disregard the above - I've gotten considerably closer.

Now the issue I run into when I use the vcpkg toolchain is:

Building CorsixTH
CMake Error at vcpkg/scripts/buildsystems/vcpkg.cmake:159 (_find_package):
  Could not find a package configuration file provided by "SDL2" with any of
  the following names:

    SDL2Config.cmake
    sdl2-config.cmake

  Add the installation prefix of "SDL2" to CMAKE_PREFIX_PATH or set
  "SDL2_DIR" to a directory containing one of the above files.  If "SDL2"
  provides a separate development package or SDK, be sure it has been
  installed.
Call Stack (most recent call first):
  CorsixTH/CMakeLists.txt:97 (FIND_PACKAGE)

Compared to the code I just committed, this was my changes:

diff --git a/CMake/VcpkgDeps.cmake b/CMake/VcpkgDeps.cmake
index 08933a19..1d778194 100644
--- a/CMake/VcpkgDeps.cmake
+++ b/CMake/VcpkgDeps.cmake
@@ -56,23 +56,4 @@ if(err_val)
       "\nIf this error persists try deleting the 'vcpkg' folder.\n")
 endif()

-# We cannot use a toolchain file at this point despite it being recommended by MS.
-# The arch is determined by the generator in use on Windows. For example
-# Visual Studio xx Win64 <= implies 64 bit build. If we use a toolchain this
-# always defaults to a 32 bit build.
-
-# If the user specified their own generator it is too late at this point.
-# We would need to restart CMake at this point, delete the
-# cache and invoke CMake again with the toolchain set.
-
-# For both of these reasons we will use CMAKE_PREFIX_PATH
-
-set(VCPKG_INSTALLED_PATH ${VCPKG_PARENT_DIR}/vcpkg/installed/${VCPKG_TARGET_TRIPLET})
-
-if(CMAKE_BUILD_TYPE MATCHES "^Debug$" OR NOT DEFINED CMAKE_BUILD_TYPE)
-  list(APPEND CMAKE_PREFIX_PATH ${VCPKG_INSTALLED_PATH}/debug)
-  list(APPEND CMAKE_LIBRARY_PATH ${VCPKG_INSTALLED_PATH}/debug/lib/manual-link)
-endif()
-
-list(APPEND CMAKE_PREFIX_PATH ${VCPKG_INSTALLED_PATH})
-list(APPEND CMAKE_LIBRARY_PATH ${_VCPKG_INSTALLED_DIR}/lib/manual-link)
+set(CMAKE_TOOLCHAIN_FILE ${VCPKG_PARENT_DIR}/vcpkg/scripts/buildsystems/vcpkg.cmake)
@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Nov 8, 2017

It looks like SDL2 doesn't provide a -config.cmake, so you'll probably need to write a FindSDL2.cmake script manually (which you probably needed anyway?).

There are a few floating around that might be interesting: https://github.com/search?utf8=%E2%9C%93&q=filename%3Afindsdl2.cmake&type=Code

ras0219-msft commented Nov 8, 2017

It looks like SDL2 doesn't provide a -config.cmake, so you'll probably need to write a FindSDL2.cmake script manually (which you probably needed anyway?).

There are a few floating around that might be interesting: https://github.com/search?utf8=%E2%9C%93&q=filename%3Afindsdl2.cmake&type=Code

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 8, 2017

Member

It turned out that the VCPKG_TRIPLET was ending up as just -windows.

in vcpkg.cmake, if VCPKG_TARGET_TRIPLET is specified it skips setting _VCPKG_TARGET_TRIPLET_ARCH, then later in the file it sets VCPKG_TARGET_TRIPLET again. I don't know how this is normally handled when you use -DVCPKG_TARGET_TRIPLET, but when I was setting it in VcpkgDeps.cmake the ARCH was never set and then removed.

By avoiding setting VCPKG_TARGET_TRIPLET, it lets the toolchain detect it and set it properly.

Member

TheCycoONE commented Nov 8, 2017

It turned out that the VCPKG_TRIPLET was ending up as just -windows.

in vcpkg.cmake, if VCPKG_TARGET_TRIPLET is specified it skips setting _VCPKG_TARGET_TRIPLET_ARCH, then later in the file it sets VCPKG_TARGET_TRIPLET again. I don't know how this is normally handled when you use -DVCPKG_TARGET_TRIPLET, but when I was setting it in VcpkgDeps.cmake the ARCH was never set and then removed.

By avoiding setting VCPKG_TARGET_TRIPLET, it lets the toolchain detect it and set it properly.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 8, 2017

Member

AppVeyor is broken because I haven't put lfs.dll in the lua cpath for lua.exe

Member

TheCycoONE commented Nov 8, 2017

AppVeyor is broken because I haven't put lfs.dll in the lua cpath for lua.exe

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 10, 2017

Member

At @Alberth289346's request I've made the indentation for the two new CMake files internally consistent.

If no one else has anything to add, I'll squash these up and move this PR back to RDY tomorrow.

Member

TheCycoONE commented Nov 10, 2017

At @Alberth289346's request I've made the indentation for the two new CMake files internally consistent.

If no one else has anything to add, I'll squash these up and move this PR back to RDY tomorrow.

TheCycoONE and others added some commits Nov 7, 2017

Bump minimum CMake version to 3.2
Our TravisCI environment is using 3.2, so we are not actively testing on any
older version.
Use CMake to pull in and use precompiled deps for Linux
This commit adds a new module which clones and automatically sets
the include and library paths for the precompiled dependencies.

Linux users can opt in however they must choose the final arch
(i.e. x86 or x64) they intend to compile against.
Add script to build and package libs in MS vcpkg
This commit adds a new script which can be executed on
a Windows machine to build and package the libraries
for CorsixTH. A new folder called vcpkg will be created
in the same folder as the script. The script is
integrated into CMake and run by default when
targetting MSVC.

@TheCycoONE TheCycoONE changed the title from [RFC] CMake deps handling - continuation of pr #1261 to [RDY] CMake deps handling - continuation of pr #1261 Nov 11, 2017

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 11, 2017

Member

All squished up and ready to go.

Member

TheCycoONE commented Nov 11, 2017

All squished up and ready to go.

@Alberth289346 Alberth289346 merged commit 46aefba into CorsixTH:master Nov 11, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment