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

Generate pkg-config file during install builds #3371

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasonliu--
Copy link

@jasonliu-- jasonliu-- commented Oct 21, 2023

This commit causes CMake to generate a pkg-config file when ENABLE_GLSLANG_INSTALL is enabled. This allows software projects that use pkg-config (and not CMake) to locate external dependencies (e.g., Godot 4.x), to find and properly link to a pre-built glslang package.

Closes #1715.

This commit causes CMake to generate a pkg-config file when `ENABLE_GLSLANG_INSTALL` is enabled. This allows software projects that use pkg-config (and not CMake) to locate external dependencies (e.g., Godot 4.x), to find and properly link to a pre-built glslang package.

Closes KhronosGroup#1715.
@arcady-lunarg arcady-lunarg added the kokoro:run Trigger Google bot runs label Oct 25, 2023
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label Oct 25, 2023
@arcady-lunarg
Copy link
Contributor

Unfortunately the pkg-config file that this produces doesn't actually work, when I try to build with pkg-config I get undefined errors because libglslang.a isn't actually self-contained and you generally will need to link against at least libSPIRV.a as well. Same goes for shared library builds I think. We're working on unifying the two into one single library file though, please see #3320.

@jasonliu--
Copy link
Author

jasonliu-- commented Oct 26, 2023

Hi @arcady-lunarg, so are you saying that the pkg-config file doesn't work in general as a concept, or are you saying that it's missing some pieces, i.e. also needing to link against libSPIRV.{a,dylib} as well? Because if it's the latter, could you let me know what are the minimum libraries that are needed? Just libglslang and libSPIRV? What about the other libs, libHLSL and libSPVRemapper? And how about static-only libraries, such as libGenericCodeGen.a, libMachineIndependent.a, libOGLCompiler.a, libOSDependent.a, libglslang-default-resource-limits.a; are those all required to link statically against glslang? I could easily add them to the Libs: line in the pkg-config file, that's no problem.

@arcady-lunarg
Copy link
Contributor

The pkg-config file is fine as a concept, but your implementation of it is broken. I suggest waiting a little while for me to finish the work of rearranging the libraries a bit and then your pkg-config file will actually be correct.

@jasonliu--
Copy link
Author

Sure, waiting is not a problem right now.

Most of the downstream software that we currently have packaged in MacPorts are building and running fine linking against glslang because they all use CMake to build. Godot is the only downstream project that must use pkg-config because they use scons as their build system. In addition, the devs are still in the process of integrating glslang, so they are currently cherry-picking certain pieces from your source code, and compiling them during the build as internal dependencies.

My wanting to add the pkg-config file was mostly just in preparation for when the Godot devs fully integrate all of the features of glslang that they want to use into their codebase, at which point in time, assuming that they haven't added too many custom patches of their own which they never contribute upstream, it will be possible to simply link Godot 4.x to a pre-built glslang dylib by using the pkg-config file. Hopefully by then you will have already been able to rearrange the glslang libraries to have an API/ABI that is completely available using just libglslang as a single entry point. Until then, I think it's perfectly reasonable to hold off on merging this PR.

If after you have rearranged the libraries, you think that I need to change the pkg-config file in some way (maybe there are some static-only libraries that have to remain separate for some reason), please let me know, I can add them to a Libs.private: line in the pkg-config file so that the additional static libs only get linked when a downstream project uses the pkg-config --static option.

@juan-lunarg
Copy link
Contributor

  • This doesn't properly handle BUILD_SHARED_LIBS.

Libs.private is only needed when BUILD_SHARED_LIBS is OFF.

  • Doesn't handle absolute installation locations which can be provided to CMake:

See: KhronosGroup/SPIRV-Headers#282

  • Testing?

Both vulkan-loader and spirv-headers currently test the generated .pc file to some extent. If the testing doesn't happen here it should happen during SDK testing since adding this will mean verifying another deliverable.

@jasonliu--
Copy link
Author

@juan-lunarg , do you believe that the same technique that is used in the issue you referenced would also work here for glslang?

Both vulkan-loader and spirv-headers currently test the generated .pc file to some extent. If the testing doesn't happen here it should happen during SDK testing since adding this will mean verifying another deliverable.

I'm willing to add similar tests to this PR, I wasn't aware that other projects were doing these tests.

@juan-lunarg
Copy link
Contributor

@juan-lunarg , do you believe that the same technique that is used in the issue you referenced would also work here for glslang?

Yes but keep in mind that is a header only library. You can look at the loader code as well for reference.

NOTE: The loader's package config is simpler since the loader can only be installed as a SHARED library so there isn't a perfect example for you to look at.

I'm willing to add similar tests to this PR, I wasn't aware that other projects were doing these tests.

👍🏾

@juan-lunarg
Copy link
Contributor

@juan-lunarg , do you believe that the same technique that is used in the issue you referenced would also work here for glslang?

This repo actually summarizes the issue nicely
https://github.com/jtojnar/cmake-snips#assuming-cmake_install_dir-is-relative-path

file(WRITE "${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc.in" [=[
prefix="@CMAKE_INSTALL_PREFIX@"
exec_prefix="${prefix}"
libdir="${prefix}/lib"
Copy link

Choose a reason for hiding this comment

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

Please use ${prefix}/@CMAKE_INSTALL_LIBDIR@ to avoid hardcoding 'lib'.

prefix="@CMAKE_INSTALL_PREFIX@"
exec_prefix="${prefix}"
libdir="${prefix}/lib"
includedir="${prefix}/include"
Copy link

Choose a reason for hiding this comment

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

Please use ${prefix}/@CMAKE_INSTALL_INCLUDEDIR@ for the same reason.

garybuhrmaster referenced this pull request in MythTV/mythtv Sep 1, 2024
Fedora 39 fixed the problem where it couldn't parse the glslang.pc
file at all because of leading spaces, but it introduced a new problem
because it still requires linking against libraries that are no longer
part of the distribution.  Change the first fix to only apply to
Fedora 38 or earlier, and add a fix for the new problem.

View this diff using the "-w" flag.
@arcady-lunarg
Copy link
Contributor

Now that glslang has been mostly consolidated to one library file, I think it might be time to pick this back up.

christophecvr added a commit to christophecvr/macports-ports that referenced this pull request Oct 17, 2024
  See: https://trac.macports.org/ticket/71081
  set python to version 3.12
  Adapted patch create-pkgconfig-file.diff with last remarks on :
    KhronosGroup/glslang#3371
  Added extra debug info to see all the enabled options in config log.
christophecvr added a commit to christophecvr/macports-ports that referenced this pull request Oct 18, 2024
  See: https://trac.macports.org/ticket/71081
  Set python to version 3.12
  Added the lates remark on:
  KhronosGroup/glslang#3371
  to patchfile.
judaew pushed a commit to macports/macports-ports that referenced this pull request Oct 18, 2024
  See: https://trac.macports.org/ticket/71081
  Set python to version 3.12
  Added the lates remark on:
  KhronosGroup/glslang#3371
  to patchfile.
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.

Add pkgconfig file in the sources
5 participants