Library targets are a mess

[ben-clayton]

Pulling together the following issues:

• Support shared libs "properly" #3214 - Support shared libs "properly"
• BUILD_SHARED_LIBS output libSPIRV-Tools.a instead of libSPIRV-Tools.so #3626 - BUILD_SHARED_LIBS output libSPIRV-Tools.a instead of libSPIRV-Tools.so
• MSVC CMake build fails with BUILD_SHARED_LIBS=1 #3482 - MSVC CMake build fails with BUILD_SHARED_LIBS=1
• No library versions in soname #3046 - No library versions in soname

To summarize the above:

BUILD_SHARED_LIBS is not properly respected, and distro packages are now broken

This is mostly my fault - #3490 fixed Windows builds that had issues building .dlls (#3482), but these changes have caused distro package problems (#3626). Some background:

Prior to #3490, we had "just" the SPIRV_TOOLS targets ${SPIRV_TOOLS} and ${SPIRV_TOOLS}-shared:

• ${SPIRV_TOOLS} was either static or shared based on BUILD_SHARED_LIBS and had all symbols visible as default (symbols not annotated with SPIRV_TOOLS_EXPORT were also exposed)
• ${SPIRV_TOOLS}-shared was (and still is) always shared and hides non-pubic-API symbols (only SPIRV_TOOLS_EXPORT symbols are visible).

Both libSPIRV-Tools.so and libSPIRV-Tools-shared.so appear to be included in distro packages.

With #3490:

• ${SPIRV_TOOLS} was renamed to ${SPIRV_TOOLS}-static and was forced to always build as static.
• ${SPIRV_TOOLS} is now an alias to ${SPIRV_TOOLS}-static or ${SPIRV_TOOLS}-shared based on BUILD_SHARED_LIBS.
• Targets that previously depended on ${SPIRV_TOOLS} now depend on ${SPIRV_TOOLS}-static.
• Other library targets always build as static.

While the changes work transparently when using SPIRV-Tools as a sub-project of another CMake project, it breaks the install package, as these were expecting all libraries to be generated as shared when BUILD_SHARED_LIBS=1.

I can only speculate that the reason we originally had the ${SPIRV_TOOLS} and ${SPIRV_TOOLS}-shared targets was due to symbol visibility - there are a number of targets in this project that depend on ${SPIRV_TOOLS} and use non-public APIs. This is discussed here.

Libraries are not versioned

None of the libraries are versioned, there's no use of versioned SONAMEs. Discussed in detail here.
Consequently, updating to a new version of SPIRV-Tools may break projects in a number of exciting ways.

---

I propose the following:

1. We replace the ${SPIRV_TOOLS}, ${SPIRV_TOOLS}-shared and ${SPIRV_TOOLS}-static targets with a single ${SPIRV_TOOLS} target that respects BUILD_SHARED_LIBS

The ${SPIRV_TOOLS} target will hide non-public APIs by default (same for static as shared).

Non public symbols in use by other SPIRV-Tools targets will need to be moved out to other static utility libraries (which will not be included in the install output) that can be privately linked by both ${SPIRV_TOOLS} and other dependee targets.

As the ${SPIRV_TOOLS}-static target has not been hiding private symbols, there's a good chance that external projects are relying on these private APIs. These will have to be fixed, or we need to promote more of the library from private to public.

This will also require fixing up downstream projects that explicitly use ${SPIRV_TOOLS}-static (examples: clspv, dxc) and ${SPIRV_TOOLS}-shared.

Distros will likely have to remove the libSPIRV-Tools-shared.so library from their packages.

2. We start versioning our shared libraries with VERSION and SOVERSION

The version numbers will be derived from the CHANGES file, and so will always be kept in sync with the source.

3. We start enforcing semantic versioning rules

We enforce a major version bump whenever there's a breaking API / ABI change to the public API.
We enforce a minor version bump whenever there's backwards-compatible, but new public APIs added.

I have been working on tooling for detecting and enforcing this as part of presubmits.

4. We classify the public API for SPIRV-Tools[-link,-opt,-reduce]

The ${SPIRV_TOOLS} target attempts to classify public from non-public API using the SPIRV_TOOLS_EXPORT macro.

In order to version the other libraries, we'll need to limit the symbol visibility to well defined public API, otherwise any change to these targets will technically break API / ABI compatibility.

If no public API can be found, we should question the inclusion of these libraries in package distributions.

---

I'm aware that these proposals will likely cause breakages for downstream projects, but I believe the long-term benefits outweigh the short term pain.

Thoughts and comments welcome.

[orbea]

@ben-clayton I would suggest that both shared and static libs could be individually enabled or disabled so that packages could be created with only shared libs, only static libs or both.

[expipiplus1]

This is important for build systems which aren't able to build the shared lib, for example nixpkgs's pkgsStatic: NixOS/nixpkgs#103197

[barcharcraz]

@ben-clayton I would suggest that both shared and static libs could be individually enabled or disabled so that packages could be created with only shared libs, only static libs or both.

This doesn't really work in cmake very well. I would suggest just looking at BUILD_SHARED_LIBS and building shared libraries if it's set and static otherwise (just call add_library without a library type and you get this behavior). For the symbol issue you should use "object" libraries, either directly or to build seperate internal static libraries that have the symbols you need. And these should NOT be installed.

Alternatively you should just export all the symbols you need to export and document which ones are "stable" and which ones aren't. You could also have seperate shared libs with the "internal" symbols that are linked to by the libs providing the "stable" symbols, but that's probably more complicated than necessary and it doesn't seem like your project really wants to commit to the stability requirements to make in place upgrades possible (it's a huge commitment).

I may prepare a patch to do this but it's likely to be somewhat disruptive to the whole vulkan-sdk cluster of dependencies.

[sl1pkn07]

Thoughts and comments welcome.

sincerely as user, all you points is how should It have been things from the start. and a year has passed and none of the point is move.

this must do have done in the next day when the error opened. be cause affect a lot of projects and distributions

exist any ETA programed for fix this?

greetings

[sl1pkn07]

#4569

please test

i do it in local and do ctest in both mode. the test passed without problems

for run ctest in shared lib. is need ad the paths of the generated .so to LD_LIBRARY_PATH, like

  LD_LIBRARY_PATH="$(pwd)/source:$(pwd)/source/link:$(pwd)/source/lint:$(pwd)/source/opt:$(pwd)/source/reduce" \
  ctest --output-on-failure

greetings

[ben-clayton]

Hi @sl1pkn07,

sincerely as user, all you points is how should It have been things from the start. and a year has passed and none of the point is move.
this must do have done in the next day when the error opened. be cause affect a lot of projects and distributions

I attempted to find a clean solution to these problems, but found that there are numerous downstream projects depending on the current build rules and behavior. The minor changes I attempted to improve the situation broke people's workflows, and people got grumpy (understandably).

I came to the conclusion that the only sensible approach to fixing this is to make a v2 branch, and make significant breaking changes there. Unfortunately my priorities have changed over the past year, so I'm unlikely to be able to drive this effort.

I'll review your PR, but I'd be extremely cautious of making significant build rule / linker changes to the main branch. There are a lot of projects that depend on SPIRV-Tools, and have (probably undesirably) come to depend on the broken behavior of this library.

I don't want to discourage any efforts to fix this, but I'd still strongly encourage large changes to be made in different branch.

[sl1pkn07]

For that i put the PR as draft

About downstream projects, I'm completely sure rigth now have a workrounds for fixing the actual behaviour, and the devs want to remove it as soon as possible.

This is a chance to do that correctly.

Greetings