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

[vcpkg-configure-cmake] Use ninja to configure in parallel #2712

Merged
merged 1 commit into from
Feb 8, 2018

Conversation

ras0219-msft
Copy link
Contributor

@ras0219-msft ras0219-msft commented Feb 1, 2018

Proposed implementation of #2368

This uses ninja to run cmake configure in parallel, when it is available.

I ran some performance testing around installing freeimage,libpng,openexr,tiff, and zlib.

  • Run 1 without: 10.03 minutes
  • Run 2 without: 10.06 minutes
  • Run 3 with: 7.467 minutes
  • Run 4 with: 7.230 minutes

So, 25% speed increase overall! :D Granted, these are all CMake-based and none are header-only, but still ;p

Requirements before merging: I plan to do a full catalog build ensuring no regressions (will take a few days) as well as testing spaces in the vcpkg path (still a bad idea, but we'll support it as much as we can).

+@tobiaskohlbau, @janisozaur

@ras0219-msft ras0219-msft added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Feb 1, 2018
@janisozaur
Copy link
Contributor

janisozaur commented Feb 2, 2018

Thanks for implementing that!

I'm certainly eager to try it out, it may provide just enough headroom for our AppVeyor job to complete.

It looks like this concerns only non-UWP targets, is that correct? What is the problem with having UWP targets improved with ninja here? It's just running the configure command, not changing the CMake generator, which seems to be the other issue.

What do the advertised times contain? Is that wall-clock time of vcpkg install freeimage libpng openexr tiff zlib? If you're going to rebuild the full catalogue, may I suggest you collect (and publish) times for each of packages/steps, so it is known where too look for optimisations next?

file(MAKE_DIRECTORY ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/vcpkg-parallel-configure)
file(WRITE ${CURRENT_BUILDTREES_DIR}/${TARGET_TRIPLET}-rel/vcpkg-parallel-configure/build.ninja "${_contents}")

message(STATUS "Configuring ${TARGET_TRIPLET} in parallel")
Copy link
Contributor

Choose a reason for hiding this comment

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

While this can easily be the case for any modern desktop system, it's easy to envision a single-CPU machines (e.g. CIs*) where ninja may default to running just one job and skipping any parallelism. In such cases this message may be misleading and maybe rephrasing it a little would state the intentions better.

* https://www.visualstudio.com/team-services/continuous-integration/

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've removed the "in parallel" to make the message less misleading. Thanks!

@ras0219-msft ras0219-msft force-pushed the ninja-parallel-configure branch from 6c1d80c to d09e983 Compare February 2, 2018 19:15
@ras0219-msft
Copy link
Contributor Author

ras0219-msft commented Feb 2, 2018

What do the advertised times contain? Is that wall-clock time of …

Yep, this is the time that vcpkg spits out after running vcpkg install freeimage (partially -- I ping-ponged back and forth with remove zlib and install freeimage) which is total wall-clock time.

It looks like this concerns only non-UWP targets, is that correct?

No, the intention was to include UWP targets. NOT VCPKG_CMAKE_SYSTEM_NAME matches targeting windows desktop and VCPKG_CMAKE_SYSTEM_NAME STREQUAL "WindowsStore" matches UWP. This was really to match Windows hosts, so I've changed it to be clearer: CMAKE_HOST_SYSTEM_NAME STREQUAL "Windows".

If you're going to rebuild the full catalogue, may I suggest you collect (and publish) times for each of packages/steps

That's a good idea, but more complicated in practice. I think there's not enough overlap here to warrant tying them together, so we should pursue it with a separate issue (we rebuild the entire catalog regularly, so there's ample opportunity to get this data in the future).

@tobiaskohlbau
Copy link
Contributor

tobiaskohlbau commented Feb 3, 2018

I've checked out the branch and can confirm it works on my machine. Can't tell anything about time improvements as I did not run a larger test. I will try to get detail timings for the same libraries as provided in the issues.

I only have one concern which is about logging I liked the way it was done before with two different files for debug and release. I've thought about piping the cmd /c …. output into the right files within ninja execution. But I could imagine that's a problem for cmake to catch if there exists errors within one of the config steps. I'm ok with concatenating the output together but would like to see a way to split them up as before. As it would have consistent namings and sheme for "parallel" and "non parallel" builds.

@janisozaur
Copy link
Contributor

@ras0219-msft I'm trying to run this on AppVeyor, but it fails to find ninja. Even after I explicitly call vcpkg_find_acquire_program(NINJA) and I see in the logs it gets downloaded, it doesn't get found. I can probably work around that myself, but given how you already take care of getting ninja for packages that need it, it would a welcome improvement, especially for automating CI

@alexkaratarakis alexkaratarakis force-pushed the ninja-parallel-configure branch from d09e983 to 5632619 Compare February 5, 2018 23:52
@ras0219-msft
Copy link
Contributor Author

@janisozaur Thanks for the bug report! We found the issue and it should be resolved -- let us know if it's still a problem!

@janisozaur
Copy link
Contributor

@ras0219-msft wow, that's impressive, trying it out on AppVeyor brought the build times for our libraries down to just 31 minutes in total. Looking forward to have this merged then. Just to give some context: the last build didn't finish in time before the 60 minute cut-off.

Libraries in question: curl:x86-windows discord-rpc:x86-windows freetype:x86-windows jansson:x86-windows libpng:x86-windows libzip:x86-windows openssl:x86-windows sdl2:x86-windows speexdsp:x86-windows zlib:x86-windows

@ras0219-msft ras0219-msft merged commit 7102569 into master Feb 8, 2018
@ras0219-msft
Copy link
Contributor Author

Thanks everyone!

@alexkaratarakis alexkaratarakis deleted the ninja-parallel-configure branch February 8, 2018 04:08
ras0219-msft pushed a commit that referenced this pull request Feb 17, 2018
* Add libgta

Libgta is a portable library that implements the Generic Tagged Array (GTA)
file format.

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* [libgta] turn off document build and doxygen dependency

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* [libgta] add build-dependency

- bzip2, zlib and liblzma

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* [libgta] remove dlls when static build

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* Fix typo

* [libgta] update CONTROL

add lf at end of file.

* libmupdf version bump

* [qt5-base] Use system freetype.

* [opencv] Fix UWP and ARM

* [hdf5] Always use config mode for HDF5

* [opencv] Fixup -- apply patch added in previous commit

* [abseil] Add *.inc files. Fixes #2718

* [abseil] Update to 2018-2-5

* Fix for Issue #2729

Allows PowerShell to change the name of the downloaded directory correctly.

* vcpkgRemoveItem: nullcheck

* [cpprestsdk] Update to 2.10.2

* Adding Torch's TH library (#2737)

Adding Torch's TH library

* [flatbuffers] Fixes #2735

* sobjectizer updated to v.5.5.21

* [nghttp2] Enable static builds

* [nghttp2]: update to 1.30.0 (#2739)

* [nghttp2]: update to 1.30.0

* [nghttp2] Enable static builds

* Add epsilon library port

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* [vcpkg-configure-cmake] Use ninja to configure in parallel (#2712)

* [curl] Add nghttp2.lib to dependencies of curl

* [vtk] Fix breaking change in find_package(HDF5)

* Merge findFileRecursivelyUp into VcpkgPowershellUtils

* [vcpkgInvokeCommandClean] Don't use -encodedCommand.

Instead, use -Command with the appropriate number of escaped quotes
(which ended up being 3)

* [blosc] Update to 1.13.5

static-install-fix.patch no longer required (upstream contains the patched version).
Resolves build error with VS 15.6 in static builds

* [cgal] Avoid using absolute paths in cmake config file

* Update CHANGELOG and bump version to v0.0.104

* [qt5-base][qtdeploy] Deploy plugin dependencies to the executable's folder.

* Add libgeotiff port

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* [libgeotiff] fix install directory for cmake configurations

Signed-off-by: Hiroshi Miura <miurahr@linux.com>

* fix tiff detection error when static build

* [libgeotiff] remove installed dlls when static build

* [libgeotiff] update description

* [chipmunk] 7.0.2 initial.

* [recast] 1.5.1 initial.

* [tinydir] 1.2.3 initial.

* [mman] git-f5ff813 initial.

* Update Catch to 2.1.2 (#2763)

* [vcpkg-ci] Delete intermediate build folders even on unsuccesful builds

* [folly] Workaround bug for VS 15.6

* [pcre] fix space issue and add mirror (fix #2751)

* [realsense2] Update to v2.10.0

Update realsense2 port to librealsense v2.10.0.

* [libevent] Fix generated libevent targets files

* fixed typo in warning message (#2773)

* [blaze] update to Blaze 3.3

* [lmdb] Fix possible whitespace problem

* Fix date issue

* add qt5-quickcontrols2

* add qt5-quickcontrols port

* add qt5-graphicaleffects

* change compile order to debug first, fix #2767 (#2785)

* [liblzma] Add usage information

* [ffmpeg] Allow static builds of ffmpeg (#2783)

ffmpeg creates static .a libraries, so change the suffix to .lib

* [ffmpeg] Bump version for PR #2783

* [aubio] Update to handle static FFMPEG

* Copy local dependencies for library targets (#2787)

* Fix for issue #2786

* [vcpkg-cmake-toolchain] Only applocal dependencies for shared libraries

* [uriparser] Update to 0.8.5

* [vcpkg] Implement Default-Features (#2697)

* [vcpkg] Add Default-Feature to make_status_pgh utility function

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Parse "Default-Features" as dependencies and add test for parsing

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Document some methods and structures

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Add install_default_features_test

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Change install_default_features_test to not have preinstalled package

* [vcpkg] Test install behaviour of default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Implement default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Test default features upgrade behavior

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Implement upgrade with default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Test behaviour of upgrade with default features in dependencies

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Make upgrade install new default features

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Move collecting of packages for which to prevent defaults

Further down the line to create_feature_install_plan.

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Fix core missing from default features and potential inf loop

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Rename, fix and move some tests

Signed-off-by: Squareys <squareys@googlemail.com>

* Updated boost license to 1.66.0 (#2795)

I was a bit confused to see a reference to "1.65.1" while I was installing 1.66.0, but it turns out this URL is just not updated. This updates it.

* [vcpkg-find-acquire-program] Add mirror for NASM. Fixes #2777.

* update cuda requirement to 9.0, fixes #2791 (#2802)

* update cuda requirement to 9.0, fixes #2791

* [cuda] Restore sample version blob

* [vcpkg] http_proxy and https_proxy should be lowercase (#2815)

Pacman of Msys understands only lowercase environment variables
http_proxy and https_proxy.

* [vcpkg] Add find/find_installed/is_installed for FeatureSpec

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Fix build command for packages that depend of features

Signed-off-by: Squareys <squareys@googlemail.com>

* [liblo] Initial port (#2821)

* [portaudio] Added ASIO support to build

* Update libpng to 1.6.34

* [liblo] Initial port

* Revert "Update libpng to 1.6.34"

This reverts commit ede0bb9.

* Revert "[liblo] Initial port"

This reverts commit bb819eb.

* [liblo] Initial port

* [liblo] Use vcpkg_from_github() and vcpkg_fixup_cmake_targets()

* [liblo] Fix SHA512

* [openvr] update to 1.0.13 (#2809)

* [ebml] Initial port. (#2812)

* [vcpkg] Fix bug with missing dependencies introduced in #2697 (#2819)

When a package dependency was not found (has no source control file),
install would exit with "Value was null" when trying to install its default
features, as the dependency would be marked erroneously as found in this
case.

Signed-off-by: Squareys <squareys@googlemail.com>

* [vcpkg] Avoid using s::status_known() -- it does not do what you think it does

* [jansson] Update to 2.11 (#2820)

* [aws-sdk-cpp] update to 1.3.58 (#2810)

* [mkl] Add port MKL (#2806)

* [corrade,magnum,-plugins,-extras,-integration] Update to latest and support feature packages (#2687)

[corrade,magnum,-plugins,-extras,-integration] Update to latest and support feature packages

* [jbig2dec][libmupdf] Extract jbig2dec, remove remaining vendored 3rdparty
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants