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.

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

Fix the link order of libglslang and libHLSL #463

Closed
wants to merge 1 commit into from

Conversation

haasn
Copy link

@haasn haasn commented May 29, 2018

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.

@googlebot
Copy link
Collaborator

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. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual 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
@haasn
Copy link
Author

haasn commented May 29, 2018

Additionally, I found another error in the linker scripts: libshaderc_util also needs to depend on SPIRV-Tools. I've included this fix in the commit.

@@ -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.

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.

@dneto0
Copy link
Collaborator

dneto0 commented Jun 11, 2018

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.

@haasn
Copy link
Author

haasn commented Jun 16, 2018

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.

@dneto0
Copy link
Collaborator

dneto0 commented Jul 11, 2018

I'd like @AWoloszyn 's opinion on how to better support building against already-installed support libraries.

@AWoloszyn
Copy link
Contributor

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.
My understanding of how one should generally handle external dependencies for CMake, is with find_package.

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:
https://stackoverflow.com/questions/46861504/cmake-transitive-dependency-is-not-found-via-find-package

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.

@haasn
Copy link
Author

haasn commented Nov 19, 2018

My understanding of how one should generally handle external dependencies for CMake, is with find_package.

The better way to handle external dependencies in general would be with pkg-config

@AWoloszyn
Copy link
Contributor

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.

@dneto0
Copy link
Collaborator

dneto0 commented Nov 28, 2018

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
And https://cmake.org/cmake/help/v3.7/module/FindVulkan.html

@dneto0
Copy link
Collaborator

dneto0 commented Mar 8, 2019

this has gone stale. closing.

@dneto0 dneto0 closed this Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants