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

Make it available as vcpkg package #40

Open
Cambloid opened this issue Apr 5, 2019 · 21 comments
Open

Make it available as vcpkg package #40

Cambloid opened this issue Apr 5, 2019 · 21 comments

Comments

@Cambloid
Copy link

Cambloid commented Apr 5, 2019

Make it available through vcpkg

@mathisloge
Copy link

I do want to take on this but I would need some feedback first, before I'm going down the path until the end. I think it would take a bit of time and there isn't a sure functional outcome.

Now comes my real question:

  1. Are you @DiligentGraphics @TheMostDiligent open to such PRs?
  2. what is the real minimum cmake version? Can we bump it up? I think using a new version of cmake is pretty easy today.
  3. I could make it white space compatible in the same run, as I need to touch many places with paths.

@TheMostDiligent
Copy link
Contributor

@mathisloge Thanks for looking into this.
First of all, few notes about why things are the way they currently are: Diligent depends on multiple third-party projects, which we integrate as submodules. These projects evolve in their own way and we update them periodically, fixing potential build and run-time issues. Though not very often, both kind of problems do happen periodically and consistently. By referencing specific commits of each dependency we make sure that the code base is always in good shape, that everything builds and runs properly and that very customer uses the exact same code base.

As to vcpkg, I am very open to adding the project to the registry, but I can't say I have a complete understanding of how vcpkg references dependencies and what exactly needs to be done to make this work - can you clarify this (or send a link to the documentation as I was not able to find a good one)? I looked at your changes and I can't immediately tell why they are necessary.

So the answer to your first question is yes, unless this requires major changes to CMake, which will result in inconsistent dependencies and major build breakage risks.

what is the real minimum cmake version?

Depending which platform you build on and for, and which components you enable, CMake requirements may vary. While it is easy to update CMake on Windows, Android Studio, for example, bundles CMake and it is usually quite out-of-date. We are trying to keep CMake requirements low when possible.

I could make it white space compatible in the same run, as I need to touch many places with paths.

If you have a PR that makes it whitespace-compatible - please submit it. But please keep the PR scope narrow to not mix unrelated changes in one commit.

@mathisloge
Copy link

@TheMostDiligent thanks for the fast response.

or send a link to the documentation as I was not able to find a good one)

unfortunately there is currently no good documentation

can you clarify this

In the world of vcpkg, the goal and the requirement is that all libraries build together. That is, if a library has dependency A and Diligent also has dependency A, both must build together with the same version in the catalogue. As a result, every dependency is considered an external dependency and must be based on the finding mechanism of cmake. Submodules are not allowed. In particular, with shared libraries, submodules can cause problems if libraries bring in their own and both use different versions of the dependency A.

So the answer to your first question is yes, unless this requires major changes to CMake, which will result in inconsistent dependencies and major build breakage risks.

if Diligent is consumed in the current way, i.e. without vcpkg, the submodules are still used. Therefore, nothing should change here, except for the way the include paths are introduced into Diligent.
If Diligent is consumed with vcpkg, Diligent will definitely build. But with the versions that are included in the catalogue of vcpkg. Before a library is added or updated in vcpkg, it must build successfully (at least with windows, linux and macos) .

@TheMostDiligent
Copy link
Contributor

As a result, every dependency is considered an external dependency and must be based on the finding mechanism of cmake.

Is my understanding correct that basically vcpkg is a catalogue of different libraries at specific versions. If one library depends on another, the dependency must be present in the catalogue and its version is defined by vcpkg?

If Diligent is consumed with vcpkg, Diligent will definitely build.

It may build at this very moment, but like I said in the beginning, there is no guarantee it will consistently work in the future is there is no way to control the version of dependency packages.

Before a library is added or updated in vcpkg, it must build successfully (at least with windows, linux and macos)

Does Microsoft team do that? Is there a continuous process that makes ensures that a library still builds after it has been addded?

@mathisloge
Copy link

mathisloge commented Mar 29, 2022

Is my understanding correct that basically vcpkg is a catalogue of different libraries at specific versions. If one library depends on another, the dependency must be present in the catalogue and its version is defined by vcpkg?

yes thats correct. If a dependency is not in the catalogue, one has to add it.

It may build at this very moment, but like I said in the beginning, there is no guarantee it will consistently work in the future

Diligent is pinpointed to a specifc version. If a dependency of Diligent is updated and breaks the build, the person who updates the dependency must also make sure, that Diligent builds again. Typically this would result in a PR for Diligent to fix those problems. See microsoft/vcpkg#19665 or microsoft/vcpkg#16494 for PRs that had to fix a lot of issues in other projects before updating.

is there is no way to control the version of dependency packages.

not in the main catalogue. that would break the whole design decision of "everything can be build together with the current catalogue you have". There is versioning for the final application that consumes vcpkg https://github.com/microsoft/vcpkg/blob/master/docs/specifications/versioning.md

Does Microsoft team do that? Is there a continuous process that makes ensures that a library still builds after it has been addded?

the microsoft team reviews every single PR. CIs are building every PR with windows-x86, windows-x64, windows-x64-static, x64-uwp, arm64, arm-uwp, x64-osx and x64-linux. Before a update is merged, all CIs have to be green. As said above, if dependencies gets updated, all projects are build which depend on that dependency. If there are still runtime issues, those will be fixed with a new PRs when reported from users. It is commonly to redirect users from diligent issues to vcpkg issues, if they have dependency issues that are related to vcpkg.

@mathisloge
Copy link

just as a side note: it could be a longer journey, but I guess that it is worth it. e.g. for mapnik I needed one and a half year until it was added in vcpkg microsoft/vcpkg#14628 and microsoft/vcpkg#18849 but it also contributed a lot to the original repository (mapnik/mapnik#4191 , mapnik/mapnik#4286) just to mention a few.

@TheMostDiligent
Copy link
Contributor

TheMostDiligent commented Mar 29, 2022

OK, things are a bit more clearer now. Since code stability and quality is paramount for us, we can't switch the main build system to the one where dependencies are not controlled by us.

My current thinking to make vcpkg work is this: change CMake files to check if every 3rd party target already exists (it is already done in a couple of places). This way, when building with vcpkg, a special CMake script will import all targets or initialize them as necessary before running Diligent CMake files, and Diligent will use these existing targets instead of using its submodules. This should make the system flexible enough to support vcpkg (as well as other scenarios where dependencies are provided by the app) and also to keep the default build approach intact.

What do you think?

@mathisloge
Copy link

mathisloge commented Mar 29, 2022

sound good. I've planned that, too.
current working branch: DiligentGraphics/DiligentCore@master...mathisloge:master

I've added a CMake option DILIGENT_USE_BUNDLED_DEPENDENCIES which defaults to ON.
So when DILIGENT_USE_BUNDLED_DEPENDENCIES=ON the submodules are used. if DILIGENT_USE_BUNDLED_DEPENDENCIES=OFF a cmake file is included, which has all those find_package calls etc.

Don't know how fast I'm progressing this and the next week but I would estimate that I'm having a working build by the end of april.

vcpkg working branch: https://github.com/mathisloge/vcpkg/tree/diligent

@TheMostDiligent
Copy link
Contributor

I've added a CMake option DILIGENT_USE_BUNDLED_DEPENDENCIES which defaults to ON.
So when DILIGENT_USE_BUNDLED_DEPENDENCIES=ON the submodules are used. if DILIGENT_USE_BUNDLED_DEPENDENCIES=OFF a cmake file is included, which has all those find_package calls etc.

Good, that sounds like a good plan. I want to make it more granular - to check every single project if it is already present in the system. This way an application may provide its own version of any specific third-party dependency.

@mathisloge
Copy link

@TheMostDiligent sounds good.

I will do that. Starting with core.

@TheMostDiligent
Copy link
Contributor

@mathisloge I updated core, tools and samples to allow their dependencies to be provided externally. If required targets already exist, Diligent will use those instead of the bundled - nothing extra needs to be done.

In my test I put all submodule dependencies into one folder ThirdParty:

args
glfw
glslang
googlestest
imgui
nuklear
SPIRV-Cross
SPIRV-Headers
SPIRV-Tools
volk
Vulkan-headers
xxHash

CMake file for this folder:

set(SPIRV-Headers_SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/SPIRV-Headers")
add_subdirectory(SPIRV-Tools)
add_subdirectory(glslang)
add_subdirectory(SPIRV-Cross)
set(gtest_force_shared_crt ON CACHE BOOL "Use shared (DLL) run-time lib even when Google Test is built as static lib.")
add_subdirectory(googletest)
add_subdirectory(glfw)

option(BUILD_SHARED_LIBS "Build shared library" OFF)
add_subdirectory(xxHash/cmake_unofficial)

Some of the dependencies do not create targets. For them a *_DIR variable should be defined. At the top level CMake:

add_subdirectory(ThirdParty)

set(DILIGENT_VULKAN_HEADERS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/Vulkan-Headers")
set(DILIGENT_SPIRV_HEADERS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/SPIRV-Headers")
set(DILIGENT_VOLK_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/volk")
set(DILIGENT_ARGS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/args")
set(DILIGENT_DEAR_IMGUI_PATH "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/imgui")
set(DILIGENT_NUKLEAR_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/nuklear")

Oveall, this allows all third-party dependencies to be defined outside of the Diligent CMake file and it will transparently pick them up.

There are few other dependencies that are checked in into the tree:

Core:

  • glew - uses custom CMake as there is none provided by the library
  • GPUOpenShaderUtils - single source file + header directly included by the project it is used by
  • stb - only stb_image_write.h, no CMake

Tools:

  • imGuIZMO.quat - single source + header, used directly, no CMake
  • json - single header, used directly, no CMake
  • libjpeg-9a - custom CMake. no known official GitHub repo
  • libtiff - custom CMake. no known official GitHub repo
  • lpng - custom CMake. no known official GitHub repo1.6.17
  • stb - single header, used directly, no CMake
  • tinygltf - single header, used directly, no CMake
  • zlib-1.2.8 - Custom CMake

@mathisloge
Copy link

@TheMostDiligent oh great. I will look into it and see if there are some bits not working as expected with vcpkg.

about volk:
volk has a target called volk::volk_headers wich will get imported with a find_package(volk CONFIG). The current used volk submodule also has this. Instead of using some cmake path variable:
IMHO it is nicer to use the target variant instead of using include paths. (as upstream distributes such targets). The main benifit is that implicit things like this:
DiligentGraphics/DiligentCore@63efbd5#diff-108a96c5e1891d251d9aa21fe5d96e15f3a5d1d6be158e8cc42dfa15413ee889L107

if(PLATFORM_LINUX)
        target_link_libraries(Diligent-GPUTestFramework
        PUBLIC
            dl # Required by Volk
            xcb
        )
    endif()

aren't needed. dl could just be replaced by volk::volk_headers

in ThirdParty/CMakeLists.txt:

set(VOLK_PULL_IN_VULKAN OFF)
add_subdirectory(volk)
set_target_properties(volk PROPERTIES EXCLUDE_FROM_ALL 1 EXCLUDE_FROM_DEFAULT_BUILD 1) # do not build volk library target - we only need the header target

All changes for volk: DiligentGraphics/DiligentCore@63efbd5

Regarding the other listed targets:

Core:

  • glew FindGLEW GLEW::GLEW available since cmake 3.1

Tools:

  • libjpeg-9a FindJPEG find_package(JPEG) JPEG::JPEG target since cmake 3.12 might be to recent
  • libtiff FindTIFF find_package(TIFF) TIFF::TIFF target available since cmake version 3.5.
  • lpng FindPNG find_package(PNG) PNG::PNG target available since cmake version 3.5.
  • zlib FindZLIB target ZLIB::ZLIB available since 3.1

So to support those find_package calls I'm proposing something I'm usually using:

option(USE_EXTERNAL_XXXXX OFF)

if(USE_EXTERNAL_XXXXX)
   find_package(XXXX [CONFIG] REQUIRED)
else() 

add_library(myCustomLibOfXXXX ....)
add_library(XXXX::XXXX ALIAS myCustomLibOfXXXX)
endif()


add_executable(DiligentCore ...)

target_link_libraries(DiligentCore  ... XXXX::XXXX)

@TheMostDiligent
Copy link
Contributor

@mathisloge I committed a bunch of updates:

  • Core: use volk and GLEW::GLEW
  • Tools: use JPEG::JPEG, TIFF::TIFF, PNG::PNG, and ZLIB::ZLIB

So to support those find_package calls I'm proposing something I'm usually using:
option(USE_EXTERNAL_XXXXX OFF)

CMake files now first try to check if the target already exists. If so, they use the existing target, otherwise add required subdirectories and create targets as necessary. As a result, all external decencies can now be loaded outside of Diligent's CMake files and will be automatically picked up. No need for USE_EXTERNAL_XXXXX options.

The way it should work now is that all required external packages are found with find_package or what is appropriate, and then add_subdirectory(DiligentEngine) is called - all existing targets should transparently be used, e.g.:

find_package(GLEW)
find_package(Vulkan)
find_package(JPEG)
add_subdirectory(glslang)
add_subdirectory(volk)
...
add_subdirectory(DiligentEngine)

I also think that there should be two options: to install DiligentCore only, and the whole project.

@mathisloge
Copy link

I also think that there should be two options: to install DiligentCore only, and the whole project.

for vcpkg I need to make seperate targets, since it is discouraged to add submodules to ports. so there will be
diligent-core, diligent-tools, diligent-fx

Maybe we could also add a optional file include where one can hook in a cmake script with all the find_package things?
Currently I would need to add a git patch applied to the repos to add such a file.

e.g.:
after the first project(...) call in each subproject (DiligentCore, DiligentTools, etc.)

if(EXISTS "${DILIGENT_FIND_EXTERNAL_LIBS}")
  include("${DILIGENT_FIND_EXTERNAL_LIBS}")
endif()

@TheMostDiligent
Copy link
Contributor

for vcpkg I need to make seperate targets, since it is discouraged to add submodules to ports. so there will be
diligent-core, diligent-tools, diligent-fx

Are they going to be separate packages? Only core is self-consistent and can be built on its own. Other modules depend on core and each other.

Maybe we could also add a optional file include where one can hook in a cmake script with all the find_package things?

Yes, I think that every submodule can have a script to load its required dependencies.

if(EXISTS "${DILIGENT_FIND_EXTERNAL_LIBS}")

Where this file will be supplied from? I mean, why would it not exist? I would expect something like

if(DILIGENT_FIND_EXTERNAL_LIBS)
  include("find_external_libs.cmake")
endif()

BTW, how are you debugging the set up before proposing a change to vcpkg?

@mathisloge
Copy link

mathisloge commented Mar 31, 2022

Are they going to be separate packages

yes. diligent-tools will have a dependency on diligent-core. see https://github.com/microsoft/vcpkg/blob/master/docs/maintainers/manifest-files.md#dependencies for more info about vcpkg dependencies.

Where this file will be supplied from? I mean, why would it not exist? I would expect something like

IIUC you want people to enable to find some external packages and some submodule packages. By enabling users to specify a custom script file somewhere in the project or on disk, they could add the paths or find_package calls as they liked. for vpckg purposes it would be this way:

  • download github source zip
  • extract
  • copy the cmake script for find_package calls into the source dir
  • configure project with -DDILIGENT_FIND_EXTERNAL_LIBS=find_vcpkg_deps.cmake
  • build & install

BTW, how are you debugging the set up before proposing a change to vcpkg?

I will setup a test project which tries to find the core lib, tools etc. and for runtime test copying the samples.
It will be a external repository which will have a simple project setup with a vcpkg.json file. It will be public and then should also be used when proposing an update to vcpkg.

propably I will need to add a cmake wrapper to find all diligent libs

@TheMostDiligent
Copy link
Contributor

IIUC you want people to enable to find some external packages and some submodule packages

What I don't understand in case of vcpkg is who is responsible for downloading these decencies? Should we assume that if we specify these dependencies, then they all are present when our CMake runs?

I created a test project where all third-party libs were in a separate folder and everything built and ran fine.

Here is the CMake file for my folder with the libs:

add_subdirectory(glew)

add_subdirectory(Vulkan-Headers)
add_subdirectory(SPIRV-Headers)
add_subdirectory(SPIRV-Tools)
add_subdirectory(SPIRV-Cross)
add_subdirectory(glslang)
add_subdirectory(volk)

set(gtest_force_shared_crt ON CACHE BOOL "Use shared (DLL) run-time lib even when Google Test is built as static lib.")
add_subdirectory(googletest)

add_subdirectory(glfw)

option(BUILD_SHARED_LIBS "Build shared library" OFF)
add_subdirectory(xxHash/cmake_unofficial)

add_subdirectory(libjpeg-9a)
add_subdirectory(libtiff)
add_subdirectory(lpng-1.6.17)
add_subdirectory(zlib-1.2.8)

set(DILIGENT_ARGS_DIR "${CMAKE_CURRENT_SOURCE_DIR}/args" CACHE PATH "" FORCE)
set(DILIGENT_DEAR_IMGUI_PATH "${CMAKE_CURRENT_SOURCE_DIR}/imgui" CACHE PATH "" FORCE)
set(DILIGENT_NUKLEAR_DIR "${CMAKE_CURRENT_SOURCE_DIR}/nuklear" CACHE PATH "" FORCE)

With the remaining path variables I think it should be possible to remove them too, if really necessary.
The only really required setting is
set(gtest_force_shared_crt ON CACHE BOOL "Use shared (DLL) run-time lib even when Google Test is built as static lib.")
otherwise tests will not build. Though there may be no need to build tests at all.

So everything should be good to go.

@mathisloge
Copy link

What I don't understand in case of vcpkg is who is responsible for downloading these decencies? Should we assume that if we specify these dependencies, then they all are present when our CMake runs?

In short: yes, all needed dependencies are installed when running the build step of diligent-xxx
Long form:

vcpkg has a catalogue of many libraries, each entry is called port.
Each port is represented as a directory with a minimum of two files:

  1. vcpkg.json: used by the vcpkg-tool to determine the name, version, features, dependencies and some other fields like license etc.
  2. portfile.cmake: will be invoked at the cmake configure step to instruct vcpkg how to build the library. The portfile sets the download location with the git ref and a hash, in case the contents of the ref changes. in the next step needed build options will be passed to invoke the cmake configure step of the library. Next the build and install step for debug and release configuration will be invoked.

So if my project called XY now wants to consume diligent-core, the vcpkg-tool will look at its vcpkg.json and scans it dependencies. Then it will look at the dependencies of the dependencies and so on. It will construct a build graph to build all deps in the correct order.

Hope it is a bit clearer now.

The thing with vcpkg is that it won't take anything specified variables in our projects CMakeLists.txt to build any dependencies.

@TheMostDiligent
Copy link
Contributor

Yes, that makes it clearer - thanks.

in the next step needed build options will be passed to invoke the cmake configure step of the library

Is it possible to also configure the dependent projects? Diligent sets some required options for third-party libs and also disables unnecessary targets to reduce clutter.

The thing with vcpkg is that it won't take anything specified variables in our projects CMakeLists.txt to build any dependencies.

Or did you mean that third-party libs can't be configured?

Next the build and install step for debug and release configuration will be invoked.

I actually now think that it may not make a lot of sense to install the engine itself. While core has a very defined API and produces dynamic/static libs that can be easily used, other modules are mostly indented to be used as source. So diligent-core may be the only port

@mathisloge
Copy link

Is it possible to also configure the dependent projects? Diligent sets some required options for third-party libs and also disables unnecessary targets to reduce clutter.

no. The way it works is that each port defines a set of possible features. Imagine Port A B and C
A has features a, aa, aaa
B has features b, bb, bbb

C needs A[aaa] and B[bbb]
B[bbb] needs B[bb]
B[bb] needs A[a]

vcpkg will instruct the buildsystems to build the following configuration: A[a, aaa], B[bb, bbb] and C

So there is only build what is needed to build everything that was requested.

configure the dependent projects?

if that were possible, the goal of vcpkg of building everything together with everything would be impossible to achieve.

Or did you mean that third-party libs can't be configured?

yes, sorry. those can only be configured via features (defined in a vcpkg.json)

I actually now think that it may not make a lot of sense to install the engine itself. While core has a very defined API and produces dynamic/static libs that can be easily used, other modules are mostly indented to be used as source. So diligent-core may be the only port

I will first implement diligent-core. And then looking for the other tools/libraries.

but as I said at the beginning, I don't have much time this week and next week. So I don't expect any results until the end of april.

@MilanDierick
Copy link

@mathisloge Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants