vsg-dev / VulkanSceneGraph

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

glslang min_request_version does not accept 15.0.0 #1326

Open psi29a opened 2 days ago

psi29a commented 2 days ago

Describe the bug Looks like cmake is not parsing 15.0.0 correctly and claims that it does not meet the min requirement of 14.

To Reproduce Steps to reproduce the behavior:

Debian Sid

https://packages.debian.org/source/sid/glslang

cmake .

CMake Warning at CMakeLists.txt:47 (find_package):
  Could not find a configuration file for package "glslang" that is
  compatible with requested version "14".

  The following configuration files were considered but not accepted:

    /usr/lib/x86_64-linux-gnu/cmake/glslang/glslang-config.cmake, version: 15.0.0
    /lib/x86_64-linux-gnu/cmake/glslang/glslang-config.cmake, version: 15.0.0

CMake Warning at CMakeLists.txt:52 (message):
  glslang not found.  ShaderCompile support disabled.

Expected behavior Should configure and find glslang

Screenshots If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

Additional context I'm packaging VSG upstream to Debian

psi29a commented 2 days ago

Change the value to "14.0.0" does not resolve the issue either.

CMake Warning at CMakeLists.txt:47 (find_package):
  Could not find a configuration file for package "glslang" that is
  compatible with requested version "14.0.0".

  The following configuration files were considered but not accepted:

    /usr/lib/x86_64-linux-gnu/cmake/glslang/glslang-config.cmake, version: 15.0.0
    /lib/x86_64-linux-gnu/cmake/glslang/glslang-config.cmake, version: 15.0.0
robertosfield commented 2 days ago

Could the glslang-config be buggy and not accepting support for older versions? Or is 15 not officially compatible with 14?

I wonder if removing the version check in the find_package line and then checking the version if it's found might work.

AnyOldName3 commented 2 days ago

As we discussed last time this came up, glslang's generated CMake config (specifically glslang-config-version.cmake) is set up to deny backwards compatibility if there's a major version change. There are options that let us specify a maximum version, too, but they're ignored if the major version differs for the minimum and maximum we set, so they won't save us here.

I don't think they set things up like that intentionally, it's just how CMake defaults to doing things, and they seem to expect that most projects using glslang use a Windows-style workflow where you clone and build the specific version of all dependencies that you want, instead of the Linux-style workflow, letting the system package manager provide your dependencies and having to deal with whichever versions it decides to give you. If we report it to them, it'll probably sit around unsolved like most of their CMake problems, but they'd probably accept a well-reasoned PR. I think I looked into it and decided it wasn't trivial, so would require at least a little bit of thought, so put it off for another day.

In the meantime, just set GLSLANG_MIN_VERSION to 15 by setting -DGLSLANG_MIN_VERSION=15 on the command line during configure, and everything should be fine.

psi29a commented 1 day ago

Alrighty, we resolved it and package is here: https://mentors.debian.net/package/vulkanscenegraph/ Awaiting lawyer review from FTPMasters and uploading.

Once there, I'll package up the rest: vsgXchange and examples.

Feel free to close this.

robertosfield commented 1 day ago

What was the solution? To use Chris' suggestion?

It'd be good to have a general solution of the box so won't close it for now.

psi29a commented 1 day ago

For the Debian package we just patched the GLSLANG_MIN_VERSION to 15 from 14. It's less work for us to carry this patch and drop when no longer necessary. We want to keep our d/rules clean, so no need to set a define.

https://salsa.debian.org/games-team/vulkanscenegraph/-/blob/master/debian/patches/001-glslang-min-version.patch?ref_type=heads