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

CMake: Use VERSION/SOVERSION for all shared libs, make glslang-default-resource-limits STATIC #2419

Merged
merged 2 commits into from
May 25, 2023

Conversation

akien-mga
Copy link
Contributor

The first patch is taken from the Debian and Fedora packaging (https://salsa.debian.org/xorg-team/vulkan/glslang/-/blob/debian-unstable/debian/patches/glslang-default-resource-limits_staticlib.patch). I did not double check if it's correct for glslang-default-resource-limits to be static, but it does seem sensible.

The second patch applies the new shared library versioning from libglslang.so to the other shared libraries built with glslang:

add_library(glslang ${LIB_TYPE} ${BISON_GLSLParser_OUTPUT_SOURCE} ${GLSLANG_SOURCES} ${GLSLANG_HEADERS})
set_target_properties(glslang PROPERTIES
FOLDER glslang
POSITION_INDEPENDENT_CODE ON
VERSION "${GLSLANG_VERSION}"
SOVERSION "${GLSLANG_VERSION_MAJOR}")

The output of this PR is (on Linux, with -DBUILD_SHARED_LIBS=ON):

$ ls -l /usr/lib64/lib{glslang,HLSL,SPIRV,SPVRemapper}.so*
lrwxrwxrwx 1 root root      16 Oct 12 11:29 /usr/lib64/libglslang.so -> libglslang.so.11*
lrwxrwxrwx 1 root root      20 Oct 12 11:29 /usr/lib64/libglslang.so.11 -> libglslang.so.11.0.0*
-rwxr-xr-x 1 root root 2238616 Oct 12 11:29 /usr/lib64/libglslang.so.11.0.0*
lrwxrwxrwx 1 root root      13 Oct 12 11:29 /usr/lib64/libHLSL.so -> libHLSL.so.11*
lrwxrwxrwx 1 root root      17 Oct 12 11:29 /usr/lib64/libHLSL.so.11 -> libHLSL.so.11.0.0*
-rwxr-xr-x 1 root root   14656 Oct 12 11:29 /usr/lib64/libHLSL.so.11.0.0*
lrwxrwxrwx 1 root root      14 Oct 12 11:29 /usr/lib64/libSPIRV.so -> libSPIRV.so.11*
lrwxrwxrwx 1 root root      18 Oct 12 11:29 /usr/lib64/libSPIRV.so.11 -> libSPIRV.so.11.0.0*
-rwxr-xr-x 1 root root  549392 Oct 12 11:29 /usr/lib64/libSPIRV.so.11.0.0*
lrwxrwxrwx 1 root root      20 Oct 12 11:29 /usr/lib64/libSPVRemapper.so -> libSPVRemapper.so.11*
lrwxrwxrwx 1 root root      24 Oct 12 11:29 /usr/lib64/libSPVRemapper.so.11 -> libSPVRemapper.so.11.0.0*
-rwxr-xr-x 1 root root  181280 Oct 12 11:29 /usr/lib64/libSPVRemapper.so.11.0.0*

CC @tjaalton @airlied to review as this will impact Debian and Fedora packaging (positively AFAIK your packaging policies :)).

@ben-clayton
Copy link
Contributor

Hi @akien-mga,

Related bug: #2283

I'm not entirely sure about versioning the libraries aside from glslang - the reason being there's no clearly defined public interface for these libraries. If we're going to take semantic versioning seriously, then we'll need to ensure that the major version is bumped whenever we make a non-backwards compatible change to any public API. As the libraries libHLSL, libSPIRV, libSPVRemapper have no public defined API, everything is treated as public, and so any changes to these libraries may be considered breaking.

It's my plan to add presubmit checks to verify that the API / ABI is backwards compatible when landing changes without a major version bump.

I've been scratching my head over how to best deal with these libs, and your expertise would be helpful here. I see a couple of options:

  • (a) we force these to all be static, and maybe remove them from distro packages. This might upset some developers, but they'll probably be equally upset whenever they update their packages and find a bunch of build failures. As far as I'm aware these libraries were considered 'private' by the authors and were never really expected to be included in distro packages in the first place.
  • (b) we take the effort in classifying a public API for each library. This is a whole bunch of work, and will likely end up in breaking changes due to code refactoring. The public API will likely miss symbols currently in use, which might upset others again.

I had been thinking of (a) and then maybe someday (b). Happy to hear other thoughts and opinions though!

@arcady-lunarg
Copy link
Contributor

The change to use VERSION/SOVERSION has already been done in a different PR, but the change to make glslang-default-resource-limits STATIC has not and I think still has value, as does the refactoring to use set_target_properties. I'm going to rebase this and try to get it merged.

@arcady-lunarg arcady-lunarg force-pushed the libs-shared-soversion branch from 642f31c to c2714ae Compare May 23, 2023 22:43
@arcady-lunarg arcady-lunarg added the kokoro:run Trigger Google bot runs label May 23, 2023
@kokoro-team kokoro-team removed the kokoro:run Trigger Google bot runs label May 23, 2023
Copy link
Contributor

@jeremy-lunarg jeremy-lunarg left a comment

Choose a reason for hiding this comment

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

LGTM

@arcady-lunarg arcady-lunarg merged commit 48a467b into KhronosGroup:main May 25, 2023
@akien-mga
Copy link
Contributor Author

Thanks for picking this up! :)

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.

5 participants