search

vsg-dev / VulkanSceneGraph

Vulkan & C++17 based Scene Graph Project
http://www.vulkanscenegraph.org
MIT License
1.32k stars 212 forks source link

Remove CONFIG mode specification from find_package(glslang) #1272

Closed nolankramer closed 2 months ago

nolankramer commented 2 months ago

Description

Removes the hard-coded CONFIG mode from https://github.com/vsg-dev/VulkanSceneGraph/blob/b64dbb2cb70236071eae8687e0b0ec9e134acd2f/CMakeLists.txt#L47

Why

This allows projects to more easily use vsg as a code dependency (MODULE mode). Projects can still use it as an installed dependency (CONFIG mode) since CMake gracefully falls-back to CONFIG mode in the event MODULE mode fails. (see https://cmake.org/cmake/help/latest/command/find_package.html#id7)

This allows developers to easily use both modes.

Note that by default CMake starts in MODULE mode (which can easily be overridden if users set CMAKE_FIND_PACKAGE_PREFER_CONFIG ON).

Fixes # https://github.com/vsg-dev/VulkanSceneGraph/issues/1271

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

I have a private project where I tested this as working.

Checklist:

robertosfield commented 2 months ago

We had to add the CONFIG to make sure that compatible versions of glslang were picked up - glslang has be distributed with varying degrees of broken CMake support, it's been a case of wack a mole trying to keep things working across all platform and build combinations - it's while for a while I had to move the build into the core VSG.

I don't know whether it's possible to just drop the CONFIG right now and still have it work for the most users as sometimes old 3rd party packaging can get fixed/replaced and resolve the issues we had to workaround.

What platforms/build combinations have to tested against? What platforms did you have issue that necessitated the change?

nolankramer commented 2 months ago

I ran into this on Windows.

It also seems related to (if not the exact same issue) https://github.com/vsg-dev/vsgFramework/issues/15.

There's other options we could explore - such as passing in options to toggle CONFIG. But, I've worked with CMake for awhile, and typically CONFIG is not set so that maximum compatibility with user configuration is attained.

robertosfield commented 2 months ago

I've used CMake since it's early days, and it's the glslang project is the first one to be so inconsistently packaged that we've had to fallback to using the CONFIG hint to make sure a modern version of the packaging is used.

What error do you see with the current use of CONFIG? What version of glslang do you have installed and how do you install it?

AnyOldName3 commented 2 months ago

There's no Findglslang.cmake module provided by CMake itself or by the VSG, so module mode will never end up being used. Getting rid of CONFIG in the find_package call doesn't magically create the finder module file. With or without this PR, module mode is not going to work.

It sounds like you're conflating module mode (where CMake looks for and uses a Find<PackageName>.cmake file when find_package is called) with CMake's new FetchContent redirection mode (where CMake, if it's failed to find a package with a config file or a finder module, will use FetchContent declarations for a package to clone the source and build it as part of the outer project). That mode also won't work due to problems on glslang's end (e.g. headers are available via different include paths if it's FetchContented) and because, due to those problems, we've got no FetchContent declarations explaining to CMake how to fetch glslang.

The linked vsgFramework issue is because, despite what it says in the readme, vsgFramework doesn't install any projects, it just builds them, and it's the install step that generates the CMake config files. For whatever reason (e.g. the cmake_minimum_required(VERSION 3.14) call is putting CMake into 3.14 backwards compatibility mode and disabling FetchContent redirection, or nothing's been done to enable it) when the config files aren't found, FetchContent redirection mode isn't stepping in to save the day. Even if it did, though, the VSG wouldn't actually build because of the aforementioned include path problem with glslang, which needs fixing upstream. The CONFIG argument isn't what's breaking this as FetchContent is documented to create config files for any libraries it fetches that will satisfy find_package calls in CONFIG mode.

All this taken into account, this PR is a red herring. There might well be a real problem that it makes go away (it's not clear from your posts that doing this has made anything work that didn't before - you've claimed it makes module mode possible, but that can't be right as there's no Findglslang.cmake file, so no module for module mode to use), but if it does, it's by hiding a problem rather than fixing it.

Having tried FetchContenting glslang and the VSG locally, I can confirm that it does indeed disable the shader compilation feature due to not finding glslang. Looking at a log of file system calls with procmon, I see that CMake checks to see if FetchContent has created the config file for glslang in CMAKE_FIND_PACKAGE_REDIRECTS_DIR a few times, but it never actually creates it. That would explain it failing to notice that glslang had been FetchContented, but this is either an upstream glslang or CMake problem or something caused by not passing the right flags to FetchContent. It's not a VSG problem. If I do the same test with this PR branch, it still can't find glslang in module mode as there's still no finder module provided by CMake or the VSG. Anyway, even if it did then configure the VSG with shader compilation support, it would then fail at build time due the to the upstream include path problem.

AnyOldName3 commented 2 months ago

As another data point, I tried the same test but with OVERRIDE_FIND_PACKAGE set in the FetchContent declaration for glslang, which appears to be the magic phrase to make find_package find things provided by FetchContent. With and without this PR, VSG is configured with shader compilation support (without this PR, it just uses the generated config file, and with this PR, it tries to find a finder module, then fails and falls back to the generated config file), but this fails part way through as neither glslang or spirv-tools make themselves installable when CMake detects it's a nested project (e.g. if FetchContent is used), and CMake won't let you install something whose dependencies aren't installable. Off the top of my head, I think glslang has flags that can be set to toggle this behaviour, but it'd be a waste of time to keep fiddling with this experiment as glslang still sets incorrect include paths when included via FetchContent, so it'll still die at build time even if it configures successfully.

nolankramer commented 2 months ago

Thanks for your detailed analysis.

Let me be more clear on my local setup, so that you can draw conclusions from that. One thing I want to point out is that as an end user I was unable to get vsgFramework even configuring without many of the steps I'll list below.

Here's what I had to do:

  1. I'm using CPM. CPM is a thin wrapper around FetchContent. CPM creates Find.cmake files for each dependency, and relies on Module mode and some internal functions to find the dependency. This is why I want to be able to use Module mode. CPM is now very popular.
  2. Nearly all of the vsg dependencies - SPIRV-Headers, SPIRV-Tools, glslang - do not emit import libraries (.lib) for the libraries they build (Windows-only of course). I had to force them to build statically with BUILD_SHARED_LIBS OFF.
  3. Because the above dependencies are static, vsg cannot be built as a static library - as static libraries require install() to export dependencies as well. So I had to flip BUILD_SHARED_LIBS ON for vsg.

Turning off CONFIG mode enables (1.) to work.

Even if this PR is a "red herring" as you say, I'm glad it's at-least serving as a catalyst for us to discuss these CMake issues. Hopefully through this discussion, we can land on some concrete steps (including addressing glslang upstream), to make vsg easier to build against in a variety of configurations.

I want to point out that pulling and building dependencies as in-source is an important and valuable configuration. It allows developers to easily upgrade dependencies and control the build process. It also centralizes development configuration, and typically cuts down on the number of installed packages and local setup steps a user needs - which is important when trying to optimize team efforts.

I think we can move this discussion into https://github.com/vsg-dev/VulkanSceneGraph/discussions/1199, or we can create a new discussion to address the wider issue of build configurations (static, shared, in-source, installed dependencies, cross-platform, etc).