Moving headers to the glslang subdirectory breaks building shaderc and libplacebo #1959
Comments
|
@dj2 championed this; can you help here? We should probably be more conservative in moving things around. |
|
I'm guessing shaderc-2019.0 is old @dneto0 would probably know better. This change does work with Tip-of-Tree shaderc. I don't know what libplacebo is so I it's possible it needs to be updated to take the header move into account. I'm not sure how we could be more conservative here, as the SPIRV/ install folder directly conflicts with the spirv-tools spirv/ install folder. So, one of them had to move .... |
I fail to see how this is fixed in shaderc, for me it still fails for current git master google/shaderc@f16e793 when trying to build against external dependencies / glslang-7.13.3496 with the same error.
I've reported the issue upstream at https://code.videolan.org/videolan/libplacebo/issues/71. Generally it would be nice if such breaking changes could be communicated to the developers working on software using glslang before, so there are at least upstream patches/commits around that distributions can backport to work around the breakage, if not proper coordinated upstream releases. |
|
Ah using the system headers I see. We don't usually build that way so didn't notice that shaderc would fail to build. @johnkslang would you be OK with moving the SPIRV source directory into the glsang/ folder to make the install and non-install variants consistent? Otherwise, we can make shaderc use a higher level include folder, but that's not as nice (it would, essentially, have its third_party/ folder in the include path instead of the specific projects). On coordination, we don't know the downstream users of glslang so can't communicate with them directly. We usually do this by opening a PR and waiting to see if there are any complaints before committing. Shaderc other projects insulate themselves by pinning the known working version of downstream dependencies so you build with the required version. |
|
@dj2, I'd like to put back any removed files, until we know it is safe to remove them. Generally, I don't want to remove interfaces or files from where existing users already expect them, and agree this should not be done without adequate communication first. |
|
This will conflict with spirv-headers, potentially in weird ways as depending on the install order on a case-insensitive filesystem, you'll get either the spirv-headers headers or the glslang headers. I'm not sure how to handle that conflict if we put the SPIRV/ folder back where it was. The files aren't removed, just the install location was changed. The problem is, in the repo they exist at |
What was the bad thing that was happening before? What users were breaking? I understand it looks wrong, and should have been done a different way (still haven't looked an the history here), but that's a separate subject from how to migrate to something different without breaking consumers. See also #1919 for some recent history. |
|
I don't have a broken user. But, it's going to end up with spirv folders that are a) confusing and b) strange. Depending on if you have glslang you have certain headers in SPIRV/ but you also have them inside the unified folder. The SPIRV/ folder is GLSLang specific so should exist in side the glslang include folder. I don't know of any way to move headers that won't break downstream customers that depend on working at tip-of-tree ..... but the fix should just be updating their include path from The reason we don't see this in shaderc is because we don't use the install headers, we build with glslang source, so our |
|
You can revert the offending CL if desired, it shouldn't break anything. |
|
My 2 cents (as the author of one of the affected downstream libraries): This is an API break. You need to learn to start bumping the And no, I won't resort to pinning specific commits; and no, that's not a solution, either. (It stops being a solution the moment you need to bisect a regression in glslang, and it also stops being a solution the moment you are affected by a bug that would have been fixed in a newer version of glslang but your software is still pinned against older ones) |
|
Yes, this change had unintended side effects. We have a plan for how to migrate to the new path which is in place now. We are installing to both locations so you can migrate as needed and we will remove the old path in a few months. You can still git bisect with the changed path I'm not sure what the version has to do with doing a bisect? Is your suggestion for the Yes, if you pin dependencies you have to roll them forward for newer fixes. It also guards you against fluctuations in tip-of-tree and insulates your users from upstream changes. Either way, we're going to be removing this path in the future. We're install both for the time being so you can migrate to the new path and we will remove the new one in May. |
My suggestion for the #if GLSLANG_VERSION > whatever
#include <glslang/GlslangToSpv.h>
#else
#include <SPIRV/GlslangToSpv.h>
#endifNow I can build the same version of my library against newer and older glslang without requiring changes in my code. (Side note: Instead of checking for the API version, we could also explicitly test for existence of
If the history contains a commit which changes the API, and I'm trying to bisect glslang while keeping the downstream library that uses it at the same version, I need to rebuild that same downstream library against different versions of the glslang API. This requires some sort of conditional include in the downstream library. |
Simple work-around to KhronosGroup/glslang#1959 is to simply brute force `/usr/include/glslang` into our include path, that way the SPIRV includes will continue working as-is. (We also add a separate line to help with use cases like e.g. VLC where the location of the glslang headers are relative to the build prefix) We do it hackily by directly accessing the compiler flags because these directories may not necessarily exist, and meson doesn't like using `include_directories` with nonexistant paths.
7b0e236 found in the latest release breaks the build of at least shaderc-2019.0 and libplacebo-1.21.0.
shaderc-2019.0
libplacebo-1.21.0
The text was updated successfully, but these errors were encountered: