Fix the link order of libglslang and libHLSL#463
Conversation
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here (e.g. What to do if you already signed the CLAIndividual signers
Corporate signers
|
libglslang depends on libHLSL, so the latter needs to be specified last. This fixes an issue when trying to build shaderc against system-wide versions of libglslang/libHLSL, rather than the in-tree versions from third_party. Additionally, libshaderc_util also depends on SPIRV-Tools
|
Additionally, I found another error in the linker scripts: |
| target_include_directories(glslc PUBLIC ${glslang_SOURCE_DIR}) | ||
| target_link_libraries(glslc PRIVATE glslang OSDependent OGLCompiler | ||
| HLSL glslang SPIRV ${CMAKE_THREAD_LIBS_INIT}) | ||
| glslang SPIRV HLSL ${CMAKE_THREAD_LIBS_INIT}) |
There was a problem hiding this comment.
glslang was already listed before and after HLSL.
I don't know the details but that's usually necessary when there's a circular dependency between the two. Are you sure that HLSL itself doesn't depend on glslang?
We may require a second mention of HLSL, something like:
glslang ... HLSL .. glslang .. HLSL
?
There was a problem hiding this comment.
It seems to work as-is for me already, but I suppose I can add the extra link to be on the safe side.
| glslang OSDependent OGLCompiler HLSL glslang SPIRV | ||
| SPIRV-Tools-opt ${CMAKE_THREAD_LIBS_INIT}) | ||
| glslang OSDependent OGLCompiler glslang HLSL SPIRV | ||
| SPIRV-Tools-opt SPIRV-Tools ${CMAKE_THREAD_LIBS_INIT}) |
There was a problem hiding this comment.
CMake already has a "PUBLIC" dependency from SPIRV-Tools-opt to SPIRV-Tools. So it seems this is not necessary?
What error are you trying to fix here?
There was a problem hiding this comment.
Correct me if I'm wrong, but those dependencies only do anything if you actually build SPIRV-Tools-opt / SPIRV-Tools using your own cmake scripts, from third_party. But in practice, I am not building libshaderc against the third_party versions - I am building them against the system-wide SPIRV-Tools installation. (As the commit message points out)
The concrete issue being solved here is sjnewbury/gentoo-gpu#22, which prevents libshaderc from working on Gentoo. The gentoo packaging policy forbids the use of vendored third_party-style libraries in favor of system-wide versions of those libraries wherever avoidable, and this is such a case.
I can either ship the work-around from this commit as part of the gentoo package, or I can try submitting it upstream - which is what I tried here.
|
Please rebase your changes against latest master to rerun the bots. Also, please sign the contributor's license agreement (CLA). That's necessary before we can accept your CL. |
If I give you free license to take over authorship of the commit, does that mean I don't have to sign the CLA? I looked at the relevant links but was confused as to what exactly you want me to do. I can copy/paste some statement, but I'm not about to download and fill in some form for a trivial commit. |
|
I'd like @AWoloszyn 's opinion on how to better support building against already-installed support libraries. |
Sorry about not getting around to this earlier, it fell off the end of my inbox. In order for us to handle the correct dependencies from SPIRV-Tools-opt -> SPIRV-Tools we will have to export a CMake config file that describes the transitive dep. See: Then users would not be required to know this transitive dependency. Ideally something like this should be done in Glslang as well, so that users do not have to replicate the strange circular dependency behavior. |
The better way to handle external dependencies in general would be with pkg-config |
|
Correct me if I am wrong, but pkg-config is not supported (properly) on Windows, so we would have to maintain a second setup for deps for Windows as well. With the cmake-based solution at least it should work everywhere. |
Doing something like LunarG for FindVulkan seems the right way forward. See https://github.com/Kitware/CMake/blob/master/Modules/FindVulkan.cmake |
|
this has gone stale. closing. |
…8ebe e7294a8ebe Add headers for SPV_NV_linear_swept_spheres. (google#483) 003bcf4e0d Add headers for SPV_NV_cluster_acceleration_structure. (google#484) 43764cc756 updates IntegerFunctions2INTEL to remove Shader capability dependency (google#481) 767e901c98 Add SPV_NV_cooperative_vector (google#482) 2b2e05e088 grammar and header changes for SPV_INTEL_subgroup_matrix_multiply_accumulate (google#471) 0659679d96 Add a source language for Rust (google#472) 9ca0e67b5e grammar and header changes for SPV_INTEL_2d_block_io (google#470) a380cd2543 Fix OpAsmTargetINTEL operand (google#468) 3f17b2af67 [SPIRV] Add generator magic number (google#467) 36d5e2ddaa Add provisional key to grammar (google#464) 45b314049d Add NonSemanticShaderDebugInfo100.h to bazel build. (google#466) 2ce05a6f79 Remove trailing whitespace (google#465) 996c728cf7 add basic utility code testing for cpp, cpp11, and c (google#461) cb6b2c32db Fix on header generator for c++11, regenerated products (google#463) 22c4d1b1e9 Add SPV_NV_cooperative_matrix2 and SPV_NV_tensor_addressing (google#458) 252dc2df08 Add nuvk's spirv emitter. (google#454) 50bc4debdc VkspReflection non-sematic: remove literals for Ids (google#453) 07ddb1c0f1 Update SPV_AMDX_shader_enqueue (google#452) d92cf88c37 Add "aliases" fields to the grammar and remove duplicated (google#447) a62b032007 Add SPV_EXT_arithmetic_fence (google#450) ec59c77a3b Reserve SPIR-V enums for MediaTek (google#451) 0413bc33fa Add SPV_EXT_optnone (google#449) git-subtree-dir: third_party/spirv-headers git-subtree-split: e7294a8ebed84f8c5bd3686c68dbe12a4e65b644
libglslang depends on libHLSL, so the latter needs to be specified last.
This fixes an issue when trying to build shaderc against system-wide
versions of libglslang/libHLSL, rather than the in-tree versions from
third_party.