search

google / shaderc

A collection of tools, libraries, and tests for Vulkan shader compilation.
Other
1.86k stars 363 forks source link

Don't use generator expressions #1415

Closed kasper93 closed 5 months ago

kasper93 commented 6 months ago

While generator expression is more concise, it is avoidable in this case. This change fixes compatibility with Meson, which currently does not support generator expressions and would incorrectly enable install. While this shouldn't be critical, it exposes other compatibility issues. In short, by changing this basic expression, I'm able to build shaderc with Meson wrap successfully.

dneto0 commented 5 months ago

Sorry, Meson is not one of our supported build systems. I counted 99 uses of CMake generator expressions in my Shaderc tree (including dependencies) I don't think it's viable to back out.

I noticed in your upstream example that you turned off Shaderc tests. https://github.com/mesonbuild/meson/issues/13214 I encourage folks to run the tests. It catches bugs. (I once caught a bad cross-compiler in the Android NDK because it miscompiled some Glslang code, and I caught it with a Shaderc test).

kasper93 commented 5 months ago

Sorry, Meson is not one of our supported build systems. I counted 99 uses of CMake generator expressions in my Shaderc tree (including dependencies) I don't think it's viable to back out.

Understandable. The point here was that Meson seems to support generator expressions that are passed down to compilation, as they were intended use-case for those in CMake. It seems to just not parse correctly those simple configuration time conditionals. Either way, thank you for looking at this PR.

I noticed in your upstream example that you turned off Shaderc tests. mesonbuild/meson#13214 I encourage folks to run the tests. It catches bugs. (I once caught a bad cross-compiler in the Android NDK because it miscompiled some Glslang code, and I caught it with a Shaderc test).

I fully agree. This was done to minimize test surface, by disabling as much as possible. Also I don't think Meson can convert ctest to own tests, so that's another problem.