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

[HOLD] Add script to build and package libs in MS vcpkg #1261

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@DavidFair
Contributor

DavidFair commented Oct 15, 2017

This PR adds a new powershell script in the CMake folder (if there is a more appropriate place please let me know) that automatically updates / packages the required libs in a vcpkg.

It follows on from ideas in #1259 that @TheCycoONE was investigating.

When the script is run it does the following:

  • Clones the vcpkg repo (or updates it if it already exists) in the current folder
  • Bootstraps and builds vcpkg
  • Selects the correct libraries for CorsixTH and builds them locally
  • Packages the libraries in 7zip file and moves it to where the script was run
  • Gives the packages sane names.

As VS2017 is ABI compatible with 2015 I recommend we build dependencies in 2017 from now on and link both 2015/17 against these libraries. This should help us consolidate the various dependencies we currently have in CorsixTH/deps.

DavidFair added some commits Oct 15, 2017

Use CMake to pull in and use precompiled deps
This commit adds a new module which clones and automatically sets
the include and library paths for the precompiled dependencies.
This should allow MSVC to have an easier time building by default.
Linux users can opt in however they must choose the final arch
(i.e. x86 or x64) they intend to compile against. For this reason
Linux requires users to opt in by default
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 final pacakged
libraries are exported as 7zip files (.7z) ready to
be integrated with CMake.
@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 15, 2017

Member

I like having this scripted!

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Your script covers the build time dependencies for CorsixTH. There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg) and one which I haven't attempted to handle yet. luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

There is also wxwidgets needed for building AnimView

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork), because new releases will break our builds until we can fix compatibility.

The export could be optional, it's not necessary for local builds.

For building the lua documentation, it is useful to additionally copy lfs.dll and lpeg.dll into the vcpkg /tools directories (where lua.exe and luac.exe are placed)

Perhaps the script could write-out the cmake line to use the vcpkg, or a link to the embedded documentation: https://github.com/Microsoft/vcpkg/blob/master/docs/users/integration.md#cmake-toolchain-file-recommended-for-open-source-cmake-projects ?

Member

TheCycoONE commented Oct 15, 2017

I like having this scripted!

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Your script covers the build time dependencies for CorsixTH. There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg) and one which I haven't attempted to handle yet. luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

There is also wxwidgets needed for building AnimView

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork), because new releases will break our builds until we can fix compatibility.

The export could be optional, it's not necessary for local builds.

For building the lua documentation, it is useful to additionally copy lfs.dll and lpeg.dll into the vcpkg /tools directories (where lua.exe and luac.exe are placed)

Perhaps the script could write-out the cmake line to use the vcpkg, or a link to the embedded documentation: https://github.com/Microsoft/vcpkg/blob/master/docs/users/integration.md#cmake-toolchain-file-recommended-for-open-source-cmake-projects ?

@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Oct 16, 2017

Contributor

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Okay, I'll put this in scripts as build_files is picked up by .gitignore with build*

There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg)

I didn't see them listed in the wiki as dependencies. I'll add them too and update the wiki.

luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

It looks like development has stopped and nobody has an active fork. Does the vcpkg team have a policy on the state of a library before it is accepted?

There is also wxwidgets needed for building AnimView

The export could be optional, it's not necessary for local builds.

Originally my plan was to build all dependencies and distribute them pre-compiled like we currently do with the dependencies. However the final size of the .7z archive is 80MB so I prefer your approach of getting users to build locally rather than forcing them to download the exported libs.

I'll adapt the script so it integrates with CMake and allows us to specify if AnimView is being built. This should allow us to compile only what users need instead of everything.

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork)

II'm going to leave it pointing at the official repository and include checking out a specific commit. We can just change the commit we point to instead of having to pull the latest changes to test.

Perhaps the script could write-out the cmake line to use the vcpkg[...]

If CMake invokes the script we could easily glob the final path and handle everything within the build script.

My only other concern is CI servers, I'm reluctant to force them to rebuild the same dependencies each time. If AppVeyor/Travis allows us to preserve files between builds we could save the current commit we use for vpckg and export the dependencies. This way we can easily determine if we need to rebuild or just extract and reuse previously built ones?

Contributor

DavidFair commented Oct 16, 2017

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Okay, I'll put this in scripts as build_files is picked up by .gitignore with build*

There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg)

I didn't see them listed in the wiki as dependencies. I'll add them too and update the wiki.

luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

It looks like development has stopped and nobody has an active fork. Does the vcpkg team have a policy on the state of a library before it is accepted?

There is also wxwidgets needed for building AnimView

The export could be optional, it's not necessary for local builds.

Originally my plan was to build all dependencies and distribute them pre-compiled like we currently do with the dependencies. However the final size of the .7z archive is 80MB so I prefer your approach of getting users to build locally rather than forcing them to download the exported libs.

I'll adapt the script so it integrates with CMake and allows us to specify if AnimView is being built. This should allow us to compile only what users need instead of everything.

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork)

II'm going to leave it pointing at the official repository and include checking out a specific commit. We can just change the commit we point to instead of having to pull the latest changes to test.

Perhaps the script could write-out the cmake line to use the vcpkg[...]

If CMake invokes the script we could easily glob the final path and handle everything within the build script.

My only other concern is CI servers, I'm reluctant to force them to rebuild the same dependencies each time. If AppVeyor/Travis allows us to preserve files between builds we could save the current commit we use for vpckg and export the dependencies. This way we can easily determine if we need to rebuild or just extract and reuse previously built ones?

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 16, 2017

Member

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Okay, I'll put this in scripts as build_files is picked up by .gitignore with build*

Good

There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg)

I didn't see them listed in the wiki as dependencies. I'll add them too and update the wiki.

Thanks - it's on all of the wiki pages I checked. Which wiki page are you reading?

luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

It looks like development has stopped and nobody has an active fork. Does the vcpkg team have a policy on the state of a library before it is accepted?

The people that work on vcpkg prefer to use upstream releases with the minimal number of patches for compatibility (and they prefer those patches get merged upstream.) I'm not aware of another package on it in a situation like luasocket. As far as I know luasocket is not dead, but has received very little attention over the past number of years. I would like to drop luasocket in favour of implementing the functionality through libcurl, but that is out of scope for now.

There is also wxwidgets needed for building AnimView

The export could be optional, it's not necessary for local builds.

Sounds good.

Originally my plan was to build all dependencies and distribute them pre-compiled like we currently do with the dependencies. However the final size of the .7z archive is 80MB so I prefer your approach of getting users to build locally rather than forcing them to download the exported libs.

If we maintain our own vcpkg ports we could (optionally) dramatically reduce that by controlling the ffmpeg compile flags, similar to what I did with CorsixTH/deps

I'll adapt the script so it integrates with CMake and allows us to specify if AnimView is being built. This should allow us to compile only what users need instead of everything.

Good

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork)

II'm going to leave it pointing at the official repository and include checking out a specific commit. We can just change the commit we point to instead of having to pull the latest changes to test.

I'm feeling fairly strongly that we will be adding a CorsixTH\vcpkg fork so that we can maintain anything we need that deviates from upstream. It will be easy to swap though.

Perhaps the script could write-out the cmake line to use the vcpkg[...]

If CMake invokes the script we could easily glob the final path and handle everything within the build script.

I'm interested in seeing how you do this.

My only other concern is CI servers, I'm reluctant to force them to rebuild the same dependencies each time. If AppVeyor/Travis allows us to preserve files between builds we could save the current commit we use for vpckg and export the dependencies. This way we can easily determine if we need to rebuild or just extract and reuse previously built ones?

Both AppVeyor and Travis have the concept of dependency caching, so we can avoid rebuilding every time.

Member

TheCycoONE commented Oct 16, 2017

The cmake folder isn't appropriate - that should just be cmake modules. In my opinion 'scripts' is the best folder, an argument could be made for the 'build_files' folder as well.

Okay, I'll put this in scripts as build_files is picked up by .gitignore with build*

Good

There are a couple runtime dependencies which I have been successful in submitting to upstream vcpkg (luafilesystem and lpeg)

I didn't see them listed in the wiki as dependencies. I'll add them too and update the wiki.

Thanks - it's on all of the wiki pages I checked. Which wiki page are you reading?

luasocket is going to be tricky - there isn't a release (or release candidate) version that works, so we have to pick a semi-recent git commit instead; which I think that upstream wouldn't accept.

It looks like development has stopped and nobody has an active fork. Does the vcpkg team have a policy on the state of a library before it is accepted?

The people that work on vcpkg prefer to use upstream releases with the minimal number of patches for compatibility (and they prefer those patches get merged upstream.) I'm not aware of another package on it in a situation like luasocket. As far as I know luasocket is not dead, but has received very little attention over the past number of years. I would like to drop luasocket in favour of implementing the functionality through libcurl, but that is out of scope for now.

There is also wxwidgets needed for building AnimView

The export could be optional, it's not necessary for local builds.

Sounds good.

Originally my plan was to build all dependencies and distribute them pre-compiled like we currently do with the dependencies. However the final size of the .7z archive is 80MB so I prefer your approach of getting users to build locally rather than forcing them to download the exported libs.

If we maintain our own vcpkg ports we could (optionally) dramatically reduce that by controlling the ffmpeg compile flags, similar to what I did with CorsixTH/deps

I'll adapt the script so it integrates with CMake and allows us to specify if AnimView is being built. This should allow us to compile only what users need instead of everything.

Good

It is better to pull from a specific vcpkg commit or release; or from my fork (or a CorsixTH organization fork)

II'm going to leave it pointing at the official repository and include checking out a specific commit. We can just change the commit we point to instead of having to pull the latest changes to test.

I'm feeling fairly strongly that we will be adding a CorsixTH\vcpkg fork so that we can maintain anything we need that deviates from upstream. It will be easy to swap though.

Perhaps the script could write-out the cmake line to use the vcpkg[...]

If CMake invokes the script we could easily glob the final path and handle everything within the build script.

I'm interested in seeing how you do this.

My only other concern is CI servers, I'm reluctant to force them to rebuild the same dependencies each time. If AppVeyor/Travis allows us to preserve files between builds we could save the current commit we use for vpckg and export the dependencies. This way we can easily determine if we need to rebuild or just extract and reuse previously built ones?

Both AppVeyor and Travis have the concept of dependency caching, so we can avoid rebuilding every time.

Show outdated Hide outdated CMakeLists.txt
Show outdated Hide outdated CMakeLists.txt
@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Oct 16, 2017

Contributor

I've merged the PR from #1259 into this.

Here is a list of the work I've done since last time:

  • The script no longer has an option to export, we just build locally
  • I've integrated it into the CMake script fully so this is completed automatically
  • I looked into using a toolchain and went with using CMAKE_PREFIX_PATH (reasons below)

Regarding the toolchain to copy and paste my comment from the file:

    # 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 such as 
    # Visual Studio xx Win64 <= 64 bit build. If we use a toolchain this
    # always defaults to a 32 bit build.
    
    # Also we would need to restart the build at this point, delete the 
    # cache and invoke cmake again with the toolchain set if the user
    # specified their own generator as it is too late at this point.

I spent several hours looking for a workaround but it rapidly descends into dirty hacks. We can however use the CMAKE_PREFIX_PATH which performs the same functionality for finding libraries as we previously had.

I'm feeling fairly strongly that we will be adding a CorsixTH\vcpkg fork so that we can maintain anything we need that deviates from upstream. It will be easy to swap though.

Completely forgot about advantages of a fork such as patching our own dependencies. I'll change where we point to if you want to set-up a fork?

Contributor

DavidFair commented Oct 16, 2017

I've merged the PR from #1259 into this.

Here is a list of the work I've done since last time:

  • The script no longer has an option to export, we just build locally
  • I've integrated it into the CMake script fully so this is completed automatically
  • I looked into using a toolchain and went with using CMAKE_PREFIX_PATH (reasons below)

Regarding the toolchain to copy and paste my comment from the file:

    # 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 such as 
    # Visual Studio xx Win64 <= 64 bit build. If we use a toolchain this
    # always defaults to a 32 bit build.
    
    # Also we would need to restart the build at this point, delete the 
    # cache and invoke cmake again with the toolchain set if the user
    # specified their own generator as it is too late at this point.

I spent several hours looking for a workaround but it rapidly descends into dirty hacks. We can however use the CMAKE_PREFIX_PATH which performs the same functionality for finding libraries as we previously had.

I'm feeling fairly strongly that we will be adding a CorsixTH\vcpkg fork so that we can maintain anything we need that deviates from upstream. It will be easy to swap though.

Completely forgot about advantages of a fork such as patching our own dependencies. I'll change where we point to if you want to set-up a fork?

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 16, 2017

Member

Ok, there is now a CorsixTH/vcpkg. For the moment is a straight fork. I read your changes after writing my last response - CMAKE_PREFIX_PATH sounds like a good way to go.

Member

TheCycoONE commented Oct 16, 2017

Ok, there is now a CorsixTH/vcpkg. For the moment is a straight fork. I read your changes after writing my last response - CMAKE_PREFIX_PATH sounds like a good way to go.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 17, 2017

Member

The other thing that vcpkg toolchain does, which would be nice to have, is copy the dlls to the program output directory: https://github.com/Microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake#L126

We could do something more specific to CorsixTH that also copied lfs.dll and lpeg.dll.

Member

TheCycoONE commented Oct 17, 2017

The other thing that vcpkg toolchain does, which would be nice to have, is copy the dlls to the program output directory: https://github.com/Microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake#L126

We could do something more specific to CorsixTH that also copied lfs.dll and lpeg.dll.

@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Oct 17, 2017

Contributor

Okay, I'll add a section to the powershell script (as it has a list of libraries for MSVC) that copies the debug and release binaries to the relevant locations.

Is it worth me also adding a small section to the CMake script which copies the various files / folders we require into the build folder such as CorsixTH.lua? This will save us having to manually do it each time we have a fresh build folder.

I've also noticed in the Corsixth/CmakeLists.txt file we do not specify debug and optimized libraries for target_link_libraries. Currently we just link against release libraries regardless of whether we are in debug or release. This is something I'll look into in the future as the change should be relatively easy but beyond the scope of this PR.

Contributor

DavidFair commented Oct 17, 2017

Okay, I'll add a section to the powershell script (as it has a list of libraries for MSVC) that copies the debug and release binaries to the relevant locations.

Is it worth me also adding a small section to the CMake script which copies the various files / folders we require into the build folder such as CorsixTH.lua? This will save us having to manually do it each time we have a fresh build folder.

I've also noticed in the Corsixth/CmakeLists.txt file we do not specify debug and optimized libraries for target_link_libraries. Currently we just link against release libraries regardless of whether we are in debug or release. This is something I'll look into in the future as the change should be relatively easy but beyond the scope of this PR.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 17, 2017

Member

For now I suggest end users make CorsixTH in a subdirectory of the checkout path, which is why build* is ignored. The program correctly finds CorsixTH.lua and the lua directories in this configuration.

I think it's out of scope for this PR, but config.h.in should be given the destination path to CorsixTH.lua; and cmake install should copy them out to the appropriate path e.g. to the output directory on windows and ${prefix}/share/corsix-th on Linux.

Member

TheCycoONE commented Oct 17, 2017

For now I suggest end users make CorsixTH in a subdirectory of the checkout path, which is why build* is ignored. The program correctly finds CorsixTH.lua and the lua directories in this configuration.

I think it's out of scope for this PR, but config.h.in should be given the destination path to CorsixTH.lua; and cmake install should copy them out to the appropriate path e.g. to the output directory on windows and ${prefix}/share/corsix-th on Linux.

@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Oct 17, 2017

Contributor

Ahh I do my builds out of source hence my different perspective.

My original plan was to use the library names to guess the DLLs to copy over, however ffmpeg doesn't following this convention. Ultimately I took the kitchen sink approach by copying over all DLLs and the contents of the tools folder. These are placed into the correct Release or Debug build folders.

Contributor

DavidFair commented Oct 17, 2017

Ahh I do my builds out of source hence my different perspective.

My original plan was to use the library names to guess the DLLs to copy over, however ffmpeg doesn't following this convention. Ultimately I took the kitchen sink approach by copying over all DLLs and the contents of the tools folder. These are placed into the correct Release or Debug build folders.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 17, 2017

Member

Copying all dlls (and maybe all pdbs for debug builds?) from the bin folders seems line a fine approach to me.

I like the approach in the accepted answer for https://stackoverflow.com/questions/10671916/how-to-copy-dll-files-into-the-same-folder-as-the-executable-using-cmake since it puts it in the correct directory regardless of the project configuration.

Member

TheCycoONE commented Oct 17, 2017

Copying all dlls (and maybe all pdbs for debug builds?) from the bin folders seems line a fine approach to me.

I like the approach in the accepted answer for https://stackoverflow.com/questions/10671916/how-to-copy-dll-files-into-the-same-folder-as-the-executable-using-cmake since it puts it in the correct directory regardless of the project configuration.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 18, 2017

Member

Release build is working, the debug build is not. It seems that some libraries SDL2 and freetype in particular are given a 'd' suffix for their debug dlls, but CMake is using the release libs which don't match that name. It's annoying but we might be better off just respecting CMAKE_BUILD_TYPE up front instead of the CONFIG at build time.

I also have concerns about updating vcpkgs packages (iirc the old versions need to be manually removed, but we don't want to do that every time, so we'll have to check for updates), and maybe how to handle builds that require old versions (e.g. release tags), though I'm willing to ignore them for now.

Member

TheCycoONE commented Oct 18, 2017

Release build is working, the debug build is not. It seems that some libraries SDL2 and freetype in particular are given a 'd' suffix for their debug dlls, but CMake is using the release libs which don't match that name. It's annoying but we might be better off just respecting CMAKE_BUILD_TYPE up front instead of the CONFIG at build time.

I also have concerns about updating vcpkgs packages (iirc the old versions need to be manually removed, but we don't want to do that every time, so we'll have to check for updates), and maybe how to handle builds that require old versions (e.g. release tags), though I'm willing to ignore them for now.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 18, 2017

Member

With respect to my above comment, it seems debug builds are broken even with CMAKE_BUILD_TYPE=debug. In the vcpkg toolchain file, they put the debug path before the non-debug path on debug (and unspecified) builds: https://github.com/Microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake#L73

Member

TheCycoONE commented Oct 18, 2017

With respect to my above comment, it seems debug builds are broken even with CMAKE_BUILD_TYPE=debug. In the vcpkg toolchain file, they put the debug path before the non-debug path on debug (and unspecified) builds: https://github.com/Microsoft/vcpkg/blob/master/scripts/buildsystems/vcpkg.cmake#L73

@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Oct 18, 2017

Contributor

Release build is working, the debug build is not. It seems that some libraries SDL2 and freetype in particular are given a 'd' suffix for their debug dlls, but CMake is using the release libs which don't match that name. It's annoying but we might be better off just respecting CMAKE_BUILD_TYPE up front instead of the CONFIG at build time.

I've added the changes from the link you suggested. SDL2 was giving some issues, it either found the debug OR release. In the end I tweaked the find module to search for both the debug and release version and package them together in the same variable.
This now links correctly on my end, does it resolve the problems you were seeing?

I also have concerns about updating vcpkgs packages (iirc the old versions need to be manually removed, but we don't want to do that every time, so we'll have to check for updates), and maybe how to handle builds that require old versions (e.g. release tags), though I'm willing to ignore them for now.

The script adds the commit id for the last build to a file within vcpkg. Using this file we can see if we have checked out forwards or backwards. When we checkout backwards we run git clean -fx and build from scratch (I don't foresee this happening many times hence the nuclear approach).
In rare cases like this: Microsoft/vcpkg#501 where vpckg update couldn't determine the correct behaviour the user will have to clean or delete the vcpkg folder.
However I want to put the mechanism for that in when we have to otherwise it forces a clean rebuild each time.

I've also moved the VCPKG_COMMIT_SHA into the cache so users can see it. From the GUI it only appears under the advanced tab. This should allow us to test new commits without having to change the CMake file until were ready.

Contributor

DavidFair commented Oct 18, 2017

Release build is working, the debug build is not. It seems that some libraries SDL2 and freetype in particular are given a 'd' suffix for their debug dlls, but CMake is using the release libs which don't match that name. It's annoying but we might be better off just respecting CMAKE_BUILD_TYPE up front instead of the CONFIG at build time.

I've added the changes from the link you suggested. SDL2 was giving some issues, it either found the debug OR release. In the end I tweaked the find module to search for both the debug and release version and package them together in the same variable.
This now links correctly on my end, does it resolve the problems you were seeing?

I also have concerns about updating vcpkgs packages (iirc the old versions need to be manually removed, but we don't want to do that every time, so we'll have to check for updates), and maybe how to handle builds that require old versions (e.g. release tags), though I'm willing to ignore them for now.

The script adds the commit id for the last build to a file within vcpkg. Using this file we can see if we have checked out forwards or backwards. When we checkout backwards we run git clean -fx and build from scratch (I don't foresee this happening many times hence the nuclear approach).
In rare cases like this: Microsoft/vcpkg#501 where vpckg update couldn't determine the correct behaviour the user will have to clean or delete the vcpkg folder.
However I want to put the mechanism for that in when we have to otherwise it forces a clean rebuild each time.

I've also moved the VCPKG_COMMIT_SHA into the cache so users can see it. From the GUI it only appears under the advanced tab. This should allow us to test new commits without having to change the CMake file until were ready.

@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft Oct 19, 2017

Does the vcpkg team have a policy on the state of a library before it is accepted?

We want libraries that are usable by others :) For mature libraries, this means latest stable release, for immature libraries it's whatever the submitter wants. For unmaintained libraries, the caveat is that we will break it by updating its dependencies, and if nobody fixes it it'll get removed.

ras0219-msft commented Oct 19, 2017

Does the vcpkg team have a policy on the state of a library before it is accepted?

We want libraries that are usable by others :) For mature libraries, this means latest stable release, for immature libraries it's whatever the submitter wants. For unmaintained libraries, the caveat is that we will break it by updating its dependencies, and if nobody fixes it it'll get removed.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 19, 2017

Member

@ras0219-msft What's the version policy if a package targets a git commit instead of an upstream release?

Edit: disregard, I see tiny-dnn 2017-10-09-dd906fed8c8aff8dc837657c42f9d55f8b793b0e which combined with the comment from earlier should mean it's ok if later upstream makes a release and we follow their numbering.

Member

TheCycoONE commented Oct 19, 2017

@ras0219-msft What's the version policy if a package targets a git commit instead of an upstream release?

Edit: disregard, I see tiny-dnn 2017-10-09-dd906fed8c8aff8dc837657c42f9d55f8b793b0e which combined with the comment from earlier should mean it's ok if later upstream makes a release and we follow their numbering.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 20, 2017

Member

Getting close now. The release configuration runs properly. The debug configuration still depends on the release SDL2.dll both directly (as well as the debug SDL2d.dll), and indirectly due to an upstream issue: Microsoft/vcpkg#1951

Direct:

E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>dumpbin /dependents CorsixTH.exe
Microsoft (R) COFF/PE Dumper Version 14.00.24213.1
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file CorsixTH.exe

File Type: EXECUTABLE IMAGE

  Image has the following dependencies:

    SDL2.dll
    lua.dll
    SDL2_mixer.dll
    avformat-57.dll
    avcodec-57.dll
    avutil-55.dll
    swscale-4.dll
    swresample-2.dll
    freetyped.dll
    KERNEL32.dll
    MSVCP140D.dll
    VCRUNTIME140D.dll
    ucrtbased.dll
    SDL2d.dll

  Summary

        1000 .00cfg
        4000 .data
        5000 .idata
       2B000 .rdata
        7000 .reloc
       28000 .rsrc
       B9000 .text
        1000 .tls

Indirect:

E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>dumpbin /dependents SDL2_mixer.dll
Microsoft (R) COFF/PE Dumper Version 14.00.24213.1
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file SDL2_mixer.dll

File Type: DLL

  Image has the following dependencies:

    SDL2.dll
    smpeg2.dll
    flac.dll
    modplug.dll
    vorbisfile.dll
    WINMM.dll
    VCRUNTIME140D.dll
    ucrtbased.dll
    KERNEL32.dll

  Summary

        1000 .00cfg
        2000 .data
        2000 .idata
        3000 .rdata
        2000 .reloc
        1000 .rsrc
       1C000 .text
Member

TheCycoONE commented Oct 20, 2017

Getting close now. The release configuration runs properly. The debug configuration still depends on the release SDL2.dll both directly (as well as the debug SDL2d.dll), and indirectly due to an upstream issue: Microsoft/vcpkg#1951

Direct:

E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>dumpbin /dependents CorsixTH.exe
Microsoft (R) COFF/PE Dumper Version 14.00.24213.1
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file CorsixTH.exe

File Type: EXECUTABLE IMAGE

  Image has the following dependencies:

    SDL2.dll
    lua.dll
    SDL2_mixer.dll
    avformat-57.dll
    avcodec-57.dll
    avutil-55.dll
    swscale-4.dll
    swresample-2.dll
    freetyped.dll
    KERNEL32.dll
    MSVCP140D.dll
    VCRUNTIME140D.dll
    ucrtbased.dll
    SDL2d.dll

  Summary

        1000 .00cfg
        4000 .data
        5000 .idata
       2B000 .rdata
        7000 .reloc
       28000 .rsrc
       B9000 .text
        1000 .tls

Indirect:

E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>E:\GitHub\CorsixTH\build-localvcpkg\CorsixTH\Debug>dumpbin /dependents SDL2_mixer.dll
Microsoft (R) COFF/PE Dumper Version 14.00.24213.1
Copyright (C) Microsoft Corporation.  All rights reserved.


Dump of file SDL2_mixer.dll

File Type: DLL

  Image has the following dependencies:

    SDL2.dll
    smpeg2.dll
    flac.dll
    modplug.dll
    vorbisfile.dll
    WINMM.dll
    VCRUNTIME140D.dll
    ucrtbased.dll
    KERNEL32.dll

  Summary

        1000 .00cfg
        2000 .data
        2000 .idata
        3000 .rdata
        2000 .reloc
        1000 .rsrc
       1C000 .text
@ras0219-msft

This comment has been minimized.

Show comment
Hide comment
@ras0219-msft

ras0219-msft commented Oct 20, 2017

The sdl2 issue should be fixed in https://github.com/Microsoft/vcpkg/tree/deee8c17.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 20, 2017

Member

I've synced CorsixTH/vcpkg with the latest upstream so we should be able to test. I'll run it tonight if no one beats me too it.

@DavidFair
We will also need to consider how to handle the share folder resources. I'll put some thoughts on that later.

Member

TheCycoONE commented Oct 20, 2017

I've synced CorsixTH/vcpkg with the latest upstream so we should be able to test. I'll run it tonight if no one beats me too it.

@DavidFair
We will also need to consider how to handle the share folder resources. I'll put some thoughts on that later.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 22, 2017

Member

Sorry I didn't post here after I tested yesterday. There is still an issue upstream with the smpeg2 package, and as I mentioned, our own project appears to end up depending on both SDL2 and SDL2d directly in Debug. I don't know why, but if you have no idea either I will try to do some reading and play around. (Multi-config cmake is new to me)

Member

TheCycoONE commented Oct 22, 2017

Sorry I didn't post here after I tested yesterday. There is still an issue upstream with the smpeg2 package, and as I mentioned, our own project appears to end up depending on both SDL2 and SDL2d directly in Debug. I don't know why, but if you have no idea either I will try to do some reading and play around. (Multi-config cmake is new to me)

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 24, 2017

Member

Using vcpkg commit 23702360ce912b1827afe5559de936cc9a24cd0c (which I have pushed to CorsixTH/vcpkg), all transitive dependencies on SDL2 are fixed. We still have a direct dependency which I need to look into further.

Member

TheCycoONE commented Oct 24, 2017

Using vcpkg commit 23702360ce912b1827afe5559de936cc9a24cd0c (which I have pushed to CorsixTH/vcpkg), all transitive dependencies on SDL2 are fixed. We still have a direct dependency which I need to look into further.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 26, 2017

Member

Ok, I got it working.

The solution is to skip the FindSDL2 module all together (evidently Find* modules are somewhat of a deprecated concept in CMake, replaced by Config files).

What you need is the following three lines:

FIND_PACKAGE(SDL2 CONFIG REQUIRED)
TARGET_LINK_LIBRARIES(CorsixTH SDL2::SDL2)
TARGET_LINK_LIBRARIES(CorsixTH SDL2::SDL2main)

You also need to skip the normal Find SDL2 block. Since I don't know how prevalent SDL2 config modules are yet, it's probably best to do that conditionally on vcpkg usage.

Member

TheCycoONE commented Oct 26, 2017

Ok, I got it working.

The solution is to skip the FindSDL2 module all together (evidently Find* modules are somewhat of a deprecated concept in CMake, replaced by Config files).

What you need is the following three lines:

FIND_PACKAGE(SDL2 CONFIG REQUIRED)
TARGET_LINK_LIBRARIES(CorsixTH SDL2::SDL2)
TARGET_LINK_LIBRARIES(CorsixTH SDL2::SDL2main)

You also need to skip the normal Find SDL2 block. Since I don't know how prevalent SDL2 config modules are yet, it's probably best to do that conditionally on vcpkg usage.

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Oct 28, 2017

Member

@DavidFair are you able to continue on this?

Member

TheCycoONE commented Oct 28, 2017

@DavidFair are you able to continue on this?

@TheCycoONE TheCycoONE changed the title from Add script to build and package libs in MS vcpkg to [HOLD] Add script to build and package libs in MS vcpkg Nov 10, 2017

@DavidFair

This comment has been minimized.

Show comment
Hide comment
@DavidFair

DavidFair Nov 10, 2017

Contributor

@TheCycoONE Sorry I don't like leaving PRs in a half completed state but something came up on my end which I had to deal with.
Thanks for continuing with these changes in #1274, I'll close this PR which is now stale.

Contributor

DavidFair commented Nov 10, 2017

@TheCycoONE Sorry I don't like leaving PRs in a half completed state but something came up on my end which I had to deal with.
Thanks for continuing with these changes in #1274, I'll close this PR which is now stale.

@DavidFair DavidFair closed this Nov 10, 2017

@TheCycoONE

This comment has been minimized.

Show comment
Hide comment
@TheCycoONE

TheCycoONE Nov 10, 2017

Member

No problem, thank you for getting the ball rolling on this.

Member

TheCycoONE commented Nov 10, 2017

No problem, thank you for getting the ball rolling on this.

Alberth289346 added a commit that referenced this pull request Nov 11, 2017

Merge pull request #1274 from TheCycoONE/build_vcpkg
CMake deps handling - continuation of pr #1261
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment