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.

Sign up for GitHub

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

Jump to bottom

Fix the link order of libglslang and libHLSL #463

Closed
wants to merge 1 commit into from
Closed

Fix the link order of libglslang and libHLSL #463

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion glslc/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ add_library(glslc STATIC
shaderc_default_compile_options(glslc)
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})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work as-is for me already, but I suppose I can add the extra link to be on the safe side.

target_link_libraries(glslc PRIVATE shaderc_util shaderc)

add_executable(glslc_exe src/main.cc)
Expand Down
4 changes: 2 additions & 2 deletions libshaderc_util/CMakeLists.txt
View file
Open in desktop
Expand Up @@ -34,8 +34,8 @@ endif(SHADERC_ENABLE_INSTALL)

find_package(Threads)
target_link_libraries(shaderc_util PRIVATE
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})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


shaderc_add_tests(
TEST_PREFIX shaderc_util
Expand Down