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

Update third party dependencies to vcpkg #909

Closed
wants to merge 1 commit into from

Conversation

jherico
Copy link
Contributor

@jherico jherico commented Feb 12, 2024

Description

Addresses #893

Migrates most of the third party dependencies to using vcpkg transparently from CMake

General Checklist:

Please ensure the following points are checked:

  • My code follows the coding style
  • I have reviewed file licenses
  • I have commented any added functions (in line with Doxygen)
  • I have commented any code that could be hard to understand
  • My changes do not add any new compiler warnings
  • My changes do not add any new validation layer errors or warnings
  • I have used existing framework/helper functions where possible
  • My changes do not add any regressions
  • I have tested every sample to ensure everything runs correctly
  • This PR describes the scope and expected impact of the changes I am making

Note: The Samples CI runs a number of checks including:

  • I have updated the header Copyright to reflect the current year (CI build will fail if Copyright is out of date)
  • My changes build on Windows, Linux, macOS and Android. Otherwise I have documented any exceptions

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Feb 12, 2024

Thank you very much for putting so much work into this. Makes it a lot easier to see if this something we want take forward :)

But a few thoughts on this:

  • IMO all Khronos related third party dependencies should remain submodules
    • I'd also prefer VMA to stay as a submodule, as it may be closely tied to new features not public yet (and is written by a Khronos member company)
  • How do we make sure that your EZ-VCPKG script will be maintained in the future? Why is it required/used?
  • Documentation should be added for maintainers on how to update dependencies with vcpkg.
    • E.g. what (as a maintainer) do I have to change if I e.g. want to update to a fixed version of tinygltf from their github repo.
    • Or how do I fix a package to a given version/tag

I'm still a bit reluctant to this, as I fear this increases overhead for maintainers. Also not sure if this won't hurt us at some point when working on private, non-github samples. Developing samples in private for non-released functionality is already pretty tough, and I'm totally fine with using submodules.

I also prefer have the dependencies as submodules inside the samples folder than on the top level of my drive tbh. With the mix of submodules and a package manager I can now only see some of the third party dependencies from the project structure in my IDE, while others are implicitly available only by file search

To sum it up: I'm unsure if this will make things better for maintainers or people using our samples. Personally, I'd prefer it to stay the way it is.

Aside from that we never really had any negative feedback on our use of submodules.

P.S. : This doesn't build for me on Windows, but I guess that's because it's still a draft.

@jherico
Copy link
Contributor Author

jherico commented Feb 12, 2024

Thanks for the feedback @SaschaWillems

IMO all Khronos related third party dependencies should remain submodules

I've reverted the KTX dependency back to a submodule in my latest version. For some reason I was having issues with getting it to link properly with the vcpkg version, but that might be related to the fact that the vcpkg version appears to embed a subset of the astc codebase. I can also revert the VMA and the Spirv-Cross changes. I didn't move glslang to start with because it looked like some unusual preprocessor definitions were being set and I wanted to determine if they were being done for a specific reason related to the example behavior (although I feel like having a version of glslang in the examples repository that's configured in some unusual way compared to how most people would get it would actually be counterproductive).

How do we make sure that your EZ-VCPKG script will be maintained in the future? Why is it required/used?

The reason it's used is to make the addition of vcpkg transparent to both developers and consumers of the repository. You're correct in suggesting that adding another required tool to get the project to build is not desirable, and I wouldn't suggest any change that adds to the number of required downloads for a person wanting to build the samples. So the script basically bootstraps a version of the vcpkg tooling and places it into a directory (defaulting to ~/.ezvcpkg/<commit-id>) and then uses that copy of vcpkg to download and build the dependencies.

This has the benefit of creating an easily cacheable location outside the build directory for CI servers, and preventing the need to rebuild them when you clean your local project. I know that many of the platforms gain a big benefit from compile caching, but Windows / Visual Studio doesn't have any caching support, and that's a big audience that includes me.

As for maintenance, I can commit to assisting with any problems for as long as I... you know... live. But even if I get hit by a bus tomorrow, part of my contention is that the maintenance burden of using the ezvcpkg script should remain less than the burden of manually managing all the submodules and the build idiosyncrasies of those projects that vcpkg is providing.

Documentation should be added for maintainers on how to update dependencies with vcpkg.

That's a reasonable ask and I can work on something for that. I suspect there are 3 distinct scenarios that need to be dealt with

  • Someone wants to add a new dependency that already exists in vcpkg
  • Someone wants to add a new dependency that doesn't exist in vcpkg
  • Someone wants a very specific version of a package which is in vcpkg but not the version required

The first scenario is easy. You just add the name of the vcpkg port to the list of packages on this line:

set(VCPKG_PACKAGES catch2 cli11 fmt glm glslang imgui spdlog spirv-cross stb tinygltf vulkan-memory-allocator)

or if you want the package in only certain build configurations you'd do something more like this:

if ((NOT ANDROID) AND (NOT (VKB_WSI_SELECTION STREQUAL D2D)))
    list(APPEND VCPKG_PACKAGES glfw3)
endif()

You also have to add the find_package declaration, but vcpkg usually has guidance on that.

For the second scenario, you have two options. One is to make the effort to add the package to vcpkg, and the other is to incorporate it as a submodule, which is the same as the work you'd already have to do. The advantage of adding it to vcpkg is that everyone gains the benefit, not just this repository, plus the vcpkg guidelines ensure that the resulting artifact doesn't have the exact kind of issues that typically create headaches for people trying to embed something, like say multiple conflicting versions of json.hpp. Adding something to vcpkg does require understanding how their infrastructure works, but it's not any more difficult really than embedding something as a project in third_party. All it takes is a basic understanding of CMake syntax really.

The last scenario is the most difficult, but I also think it's the least likely. I understand the use case for keeping vulkan headers / etc as submodules because of the need to point at pre-release versions, but how likely is it that an example repository will need to point at an out of date package in vcpkg? Like, I could understand if this was a library and you needed to support using some specific version of fmt because downstream customers depended on it, but this is specifically an example repository.

However, assuming there was a compelling use case, the problem isn't intractable. There are a few possible approaches.

The first is just to try to find a commit in vcpkg that satisfies all your requirements. ezvcpkg lets you specify the commit by tag or commit hash, and so you can essentially go back in time very easily. However, this is a monolithic change, so while you might be able to find a specific version of fmt in the vcpkg history, you're stuck with the specific versions of all the other packages that are at the same commit.

The second possibility is to fork the main vcpkg repository and target the fork. When working on a fork you can easily set the dependencies (located in various ports/<package_name> folder) to specific versions from history and mix and match as desired. This is a little more maintenance intensive, but if you're doing it for just one package at a given time it's not a lot of work.

The last possibility is something called manifest mode, which is a feature vcpkg started supporting after I created ezvcpkg. It sounds like this is an optimal solution for targeting specific versions of packages, but my ezvcpkg script doesn't support it right now, and I'm not clear on how the syntax would work for targeting a specific versions for a specific package, but I'm studying up on it.

I also prefer have the dependencies as submodules inside the samples folder than on the top level of my drive tbh.

I get the sense you're talking about diving into the source files, is that right? While the lack of access to source files for vcpkg dependencies is one drawback of this approach, it's still trivially easy to navigate to any third party headers / symbols using any modern IDE. I'm able to, in MSVC 2022 for instance, hit ctrl-, and then type in any symbol or header name to quickly navigate to them...

image

Ideally, there really shouldn't reason for example developers or users to be diving into the implementation for things like fmt or imgui, but I suppose it's possible. It's less of an issue for header-only libraries like glm and vma.

@jherico
Copy link
Contributor Author

jherico commented Feb 12, 2024

You should be good to go for trying out the windows build now @SaschaWillems

Only the Android build is still giving me problems. I'm investigating that one.

@jherico
Copy link
Contributor Author

jherico commented Feb 13, 2024

ANNNNNNNNDROOOOOOOID!!!!!!!!!

Copy link
Contributor

@gary-sweet gary-sweet left a comment

Choose a reason for hiding this comment

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

This doesn't work (in it's current form at least) for me.

Our environment requires our own cross-compiling toolchain, and from what I've (briefly) seen, this doesn't appear to be using it. It seems to be building for the host build machine, not the target.

Here's how far it gets:

[100%] Built target vulkan
-- EZVCPKG Checking out commit 2024.01.12
-- EZVCPKG Bootstrapping vcpkg binary
-- EZVCPKG vcpkg bootstrap command: /build/gsweet/EXVCPKG_TMP/2024.01.12/bootstrap-vcpkg.sh
-- EZVCPKG vcpkg working dir: /build/gsweet/EXVCPKG_TMP/2024.01.12
-- EZVCPKG Building/Verifying packages catch2 cli11 fmt glm glslang imgui spdlog spirv-cross stb tinygltf vulkan-memory-allocator
-- Running /build/gsweet/EXVCPKG_TMP/2024.01.12/vcpkg --vcpkg-root /build/gsweet/EXVCPKG_TMP/2024.01.12 install --triplet x64-linux catch2 cli11 fmt glm glslang imgui spdlog spirv-cross stb tinygltf vulkan-memory-allocator
-- EZVCPKG done
-- CMAKE_PREFIX_PATH /build/gsweet/builds/7712_C0_64;/build/gsweet/EXVCPKG_TMP/2024.01.12/installed/x64-linux;/build/gsweet/EXVCPKG_TMP/2024.01.12/installed/x64-linux/share
-- CMAKE_TOOLCHAIN_FILE /build/gsweet/vc5_git/v3dv3/cmake/toolchains/bcg-aarch64-toolchain.cmake
CMake Error at CMakeLists.txt:53 (find_package):
  Could not find a package configuration file provided by
  "VulkanMemoryAllocator" with any of the following names:

    VulkanMemoryAllocatorConfig.cmake
    vulkanmemoryallocator-config.cmake

  Add the installation prefix of "VulkanMemoryAllocator" to CMAKE_PREFIX_PATH
  or set "VulkanMemoryAllocator_DIR" to a directory containing one of the
  above files.  If "VulkanMemoryAllocator" provides a separate development
  package or SDK, be sure it has been installed.

The default location of EZVCPKG_BASEDIR being the home folder also caused initial issues, as our home folders are mounted on a data center and are very slow.

I'm also don't really want a bunch of extra code and sources being pulled outside of the main source / build folders.

I'm not entirely sure what problem you're trying to fix with this PR, but at first sight it doesn't seem particularly conducive to supporting the wide range of devices that Vulkan is targeting. Adding extra layers of build complexity, when submodules already work well, seems a little like a solution looking for a problem.

@SaschaWillems
Copy link
Collaborator

SaschaWillems commented Feb 14, 2024

I'm sure Gary is not the only one running into this issue. This repo is used by several Khronos related IHVs and ISVs, some with very specific requirements and I wouldn't want to break anything for them. Not all of them are active at github or allowed to talk about how they use stuff in public. I don't like to repeat myself, but I'm fine with our current build system.

@jherico
Copy link
Contributor Author

jherico commented Feb 14, 2024

@gary-sweet thanks for the feedback and the testing.

tl;dr: I think that the issues currently facing this PR are tractable, and I think that the use of vcpkg to grab dependencies (even just the non-Khronos dependencies) would be an overall reduction in maintenance burden (see the size of the third party CMakeLists.txt and the existence of the VKBP_DISABLE_WARNINGS macro) and also benefit the target audience of people trying to learn how to use Vulkan.

However, I get the sense that it's becoming a point of irritation, so I'm content to drop it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This is relevant to the build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants