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

CMake Configuration can fail when building as a code dependency due to find_package(glslang CONFIG) #1271

Open
nolankramer opened this issue Sep 1, 2024 · 15 comments

Comments

@nolankramer
Copy link

Describe the bug

When including vsg as a code dependency in another project (as a submodule or FetchContent) AND the encompassing project is using glslang as a code dependency - vsg can fail to configure because the find_package(glslang) command is forced into CONFIG mode:
See

find_package(glslang ${GLSLANG_MIN_VERSION} CONFIG)

To Reproduce
Steps to reproduce the behavior:

  1. Create an encompassing folder that has a CMakeLists.txt
  2. Use FetchContent to retrieve and configure glslang and vsg

Expected behavior
CONFIG should be removed - as CMake can gracefully fallback to CONFIG mode if MODULE mode fails.

@AnyOldName3
Copy link
Contributor

AnyOldName3 commented Sep 3, 2024

There's been a bunch of discussion about this on a PR, so I'll quote it all here:

Robert:

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?

Nolan:

I ran into this on Windows.

It also seems related to (if not the exact same issue) vsg-dev/vsgFramework#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.

Robert:

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?

Me:

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.

Also me:

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.

Nolan:

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 #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).

@AnyOldName3
Copy link
Contributor

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.

vsgFramework is currently broken. We made the decision not to support glslang via FetchContent because of upstream problems that make it a pain, and because glslang's maintainers recommend the build/install/find approach as the only reliable one, so aren't going to prioritise fixing anything. I did some work to fix upstream problems, but at least one of them would need major changes, so they're unlikely to accept it.

CPM creates Find.cmake files for each dependency, and relies on Module mode and some internal functions to find the dependency.

That would explain the difference in behaviour. I didn't know CPM did this as all of its examples are for libraries that don't have any dependencies they don't bundle. Looking into it, it doesn't sound like it's an entirely finished feature, so I think it's a decently safe bet that CPM could start also creating the config files like plain FetchContent would in recent versions of CMake, or register itself as a dependency provider to intercept the find_package call even in config mode.

Without CPM, it's considered bad practice to use find_package without CONFIG if you've not provided your own finder module, so it's not a good thing if CPM's making it a requirement, and should really be resolved on their end.

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.

I just tried a DLL build of glslang, and it seems pretty broken. I've reported it here KhronosGroup/glslang#3716. The import libraries aren't supposed to be missing, and it even breaks several of glslang's own targets, but presumably, you've not tried building those.

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.

This is one of the upstream problems I mentioned. If things are working properly, then you need install to work properly for your dependencies if they're DLLs/.sos, too, otherwise the shared libraries will be missing at runtime. You can get away with this being broken on your dev machine as typically you wouldn't be installing your application and would instead run it right from the build directory.

This was something that was broken on purpose by glslang with KhronosGroup/glslang#3439 by an actual LunarG employee. KhronosGroup/glslang#3508 fixes it (or maybe only partially fixes it - it was months ago that I looked into it) if you explicitly pass in an option, so you might want to try that.

Anyway, I'm able to build VSG as a static or dynamic library and link it with static glslang just fine when glslang is built, installed and find_package(CONFIG)ed, because that's not horribly broken upstream.

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.

You need to tell glslang's maintainers this, as all the difficulties stem from them disagreeing, and making a big mess. Fixing that mess would require days of development and then based on my experience of getting PRs to glslang merged, weeks or months of politicking to maybe get the changes merged or maybe get them rejected. I did try the make everything nice upstream so FetchContent is feasible approach, but had to give up as it was eating too much valuable time. There are ways to work around lots of the problems, and I did manage to get fixes merged for some of the others, but it even if we tolerated all the necessary ugliness in this repo to do that, it could all fall apart when glslang makes another change that breaks this workflow.

Summary

A minor CPM issue plus glslang doesn't support being used with FetchContent so we can't support using it via FetchContent.

As a temporary solution, you should be able to ExternalProject_Add glslang, and use FetchContent for everything else, as that should let you build and install glslang to your build directory before other projects that use it get configured.

@nolankramer
Copy link
Author

Thank so much for going down this rabbit hole with me @AnyOldName3! I really appreciate the patience as I get situated with this issue.

I'm not one for politicking either - I try to avoid it like the plague. I really prefer just being able to get things done, instead of trying to change people's point-of-view. That's why I appreciate you having the patience to explain this all to me.

Seeing the issue as it is now - this all makes sense: If the upstream dependency doesn't work with FetchContent, then any code dependency build that relies on the feature will be horribly broken, and there's nothing that we could permanently do to address it, unless all dependencies agree to hold up their end of the compatibility bargain.

Also - I double-checked my local build - you're right about no symbols being exported. That seems like a serious issue as well! It makes sense now that the import .libs are not being generated. It also explains why building as a static library worked, since those don't rely on a dynamic linker.


I'll try the workaround with ExternalProject_Add for my own project.

I don't think I'm in a position to contribute much to the discussion in glslang - however I'm happy to add another voice of discontent on their repo since, to me, it's clearly non-supportive of a useful workflow when it could be if it were just built with it in-mind.

@nolankramer
Copy link
Author

For anyone following along - @AnyOldName3's open issue in glslang is here: KhronosGroup/glslang#3509

@robertosfield
Copy link
Collaborator

Thanks for all the discussion guys. glslang has been painful dependency to have :-|

If there is an alternative for compiling high level shaders into SPIR-V comes into existence that is easy to integrate I'll very happily adopt it.

@nolankramer
Copy link
Author

nolankramer commented Sep 8, 2024

I've ended up working around the issue using this cmake code:

include(FetchContent)

set(FETCHCONTENT_TRY_FIND_PACKAGE_MODE NEVER)

# Download the source
FetchContent_Declare(glslang
    GIT_REPOSITORY "https://github.com/KhronosGroup/glslang.git"
    GIT_TAG "main"
    GIT_PROGRESS TRUE
    FIND_PACKAGE_ARGS
)
FetchContent_Populate(glslang)
message("glslang_SOURCE_DIR=${glslang_SOURCE_DIR}")
message("glslang_BINARY_DIR=${glslang_BINARY_DIR}")

message("[glslang-wrapper message]: Running update_glslang_sources.py...")
execute_process(
    COMMAND python update_glslang_sources.py
    WORKING_DIRECTORY ${glslang_SOURCE_DIR}
)

message("[glslang-wrapper message]: Configuring glslang...")
# Configure it
execute_process(
    COMMAND ${CMAKE_COMMAND} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DBUILD_SHARED_LIBS=OFF -DGLSLANG_ENABLE_INSTALL=ON -DGLSLANG_TESTS=OFF ${glslang_SOURCE_DIR}
    WORKING_DIRECTORY ${glslang_BINARY_DIR}
)
message("[glslang-wrapper message]: Building glslang...")
# Build it
execute_process(
    COMMAND ${CMAKE_COMMAND} --build . -j16 --config ${CMAKE_BUILD_TYPE}
    WORKING_DIRECTORY ${glslang_BINARY_DIR}
)
message("[glslang-wrapper message]: Installing glslang to ${CMAKE_CURRENT_BINARY_DIR}/glslang-install")
# Install it
execute_process(
    COMMAND ${CMAKE_COMMAND} --install . --prefix ${CMAKE_CURRENT_BINARY_DIR}/glslang-install --config ${CMAKE_BUILD_TYPE}
    WORKING_DIRECTORY ${glslang_BINARY_DIR}
)

# Set paths
set(SPIRV-Tools_DIR ${CMAKE_CURRENT_BINARY_DIR}/glslang-install/SPIRV-Tools/cmake CACHE PATH "" FORCE)
set(SPIRV-Tools-opt_DIR ${CMAKE_CURRENT_BINARY_DIR}/glslang-install/SPIRV-Tools-opt/cmake CACHE PATH "" FORCE)
set(glslang_DIR ${CMAKE_CURRENT_BINARY_DIR}/glslang-install/lib/cmake/glslang CACHE PATH "" FORCE)
message("* glslang_DIR=${glslang_DIR}")

It's not pretty. It essentially downloads, configures, builds, and installs glslang at parent project configure time - then sets glslang_DIR to the install directory.

It's probably robust enough to use in vsg-Framework. It might need some error checking, cleanup, and checks to speed up cached builds. But that's up to you guys. We might want to run some tests to ensure robustness.

It could probably also be made more slick by using an externally-sourced SPIRV-tools.

@AnyOldName3
Copy link
Contributor

If you're doing everything at configure time, then ExternalProject is a more appropriate tool than FetchContent https://cmake.org/cmake/help/latest/module/ExternalProject.html.

@robertosfield
Copy link
Collaborator

I'm open to revisiting the use of FetchContent in vsgFramework, originally I was concerned about supporting older versions of CMake that widely used linux distro's were still on, but it's now 2024 not 2018 so we can probably relax the CMake version constraint. We'd need to check what CMake versions are available on the main platforms, then use that as a starting place.

To get things started, the laptop I'm on now has Kubuntu 24.04 which has CMake 3.28.3.

@robertosfield
Copy link
Collaborator

My Kubuntu 22.04 systems has CMake 3.22.1

@robertosfield
Copy link
Collaborator

Looking online for older Ubuntu versions:

Ubunutu 20.04 has cmake 3.16.3
Ubunutu 18.04 has cmake 3.10.2

It's more important that the VSG itself can still work with older CMake versions - the required version presently sits at 3.7. It would interesting to see what other older distros have as we might be able to set a more modern CMake as minimum.

For vsgFramework it's more of a niche vsgFramework, requires 3.14, but we potentially could set it 3.16.3 so still work on the Ubuntu 20.04 era OS's. I don't know what that might gives us in terms of additional CMake capabilities. If we set 22.04 era as a minimum for vsgFramework then we can use CMake 3.22.1 as a base.

@AnyOldName3
Copy link
Contributor

ExternalProject_Add, which is the more suitable tool for building dependencies during the configure stage, was already in CMake 3.0, so no change is needed to version requirements to make a less dodgy version of the vsgFramework patch work.

@nolankramer
Copy link
Author

nolankramer commented Sep 9, 2024

I patched the above code together quickly over the weekend. I didn't see an immediate way to set-up a dependency relationship between an ExternalProject_Add glslang and a FetchContent_Declare VulkanSceneGraph.

If there's a better way, I'm happy to update the code!

@AnyOldName3
Copy link
Contributor

After the ExternalProject_Add's install stage has run, you'll have a directory with glslang installed to it just like with the FetchContent-with-extra-steps approach above. I don't remember whether the default install path is automatically added to CMAKE_PREFIX_PATH or whether you'll need to manually add it/set glslang_DIR, but it's no worse than what you've already tried.

@nolankramer
Copy link
Author

@robertosfield How far back is vsg required to hold compatibility for Linux and CMake?

@robertosfield
Copy link
Collaborator

@robertosfield How far back is vsg required to hold compatibility for Linux and CMake?

That's the very question I'm asking myself by listing what different OS versions support which CMake versions. I don't want to orphan support for users stuck on older OS's but if the number of affected users is small and they have a path for installing a more modern CMake version then we can potentially just ask them to upgrade.

The VSG community is like all open source communities - you don't get to find out all the different combinations that users are using the software until they raise a flag a problem saying they've come across a problem. You can ask for feedback and this can help, but these calls on go so far in reaching end users. It's only after you've made a decision and changed something do you get to find out that causes a problem for some users.

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

No branches or pull requests

3 participants