[WIP] Attemp to fix https://github.com/KhronosGroup/SPIRV-Tools/issues/3909 #4569
Conversation
|
Continuing discussion from #3909 (comment)
This PR removes targets that were identified as in-use. See #3626 (comment) and #3626 (comment). I'm sure there are many more uses in non-public codebases. There are linux packages shipping
The fact there's a
I do not believe that changing default visibility to public is a good idea. Making all the symbols public can impact binary size and performance. As soon as all symbols are public, they'll get used, and you won't be able to hide them again without breaking downstream code. |
you fixed that with unexpected behaviour. The creation of the file this can be expanded to package maintainers. the right now behaviour (without this PR) is explained in this issue #4417, wich is cleary not expected, and i hope this PR fix it
this is because the current behaviour is incorrect. and also, the downstrem project with use the fixed behaviour, wich use or
maybe i need chage the things i made in the part of hide or unhide symbols maybe need much love, but with greetings |
|
I don't disagree that your changes bring the CMake configurations to a cleaner, and more expected / idiomatic output. We have distribution packages out in the wild that ship with As there are packages containing this file, there will be numerous binaries that will depend on
How many downstream projects? This PR affects projects that depend directly on SPIRV-Tools CMake rules, and those that depend on the linux distribution packages. You seem confident that this is a small cost for a small number of projects. I'm less confident about this. I know this will impact a number of Google projects, which I can likely handle, but the changes to symbol visibility may cause additional unexpected problems. Maybe this major breaking change is the right thing to do. |
sorry if my comments sounds redundant or "strange". my apologyze. my intention is be clear due my lack know of the language
yea, and that name convention is not expected. and depends how is builded can be incorrect notes about the tings in the main CMakeList.txt ok
then why hide the symbol when build if enable shared libs, and why is full exposed here? i dont understand the logic on this. when build static lib, the it's not better expose the symbols in both libraries in any case? (or hide the expecific symbol you want to hide (i think i miss something here) unfuctional lib. the projects needs to use i try to modify the content of the PR for re-add some deleted logic about visibility and not work at all due a lack of some symbols in changes: diff --git a/source/CMakeLists.txt b/source/CMakeLists.txt
index cf5fe48d..56699e57 100644
--- a/source/CMakeLists.txt
+++ b/source/CMakeLists.txt
@@ -373,6 +373,14 @@ add_library(${SPIRV_TOOLS} ${SPIRV_TOOLS_LIBRARY_TYPE} ${SPIRV_SOURCES})
spirv_tools_default_target_options(${SPIRV_TOOLS})
set(SPIRV_TOOLS_TARGETS ${SPIRV_TOOLS})
+if(BUILD_SHARED_LIBS)
+ set_target_properties(${SPIRV_TOOLS} PROPERTIES CXX_VISIBILITY_PRESET hidden)
+ target_compile_definitions(${SPIRV_TOOLS}
+ PRIVATE SPIRV_TOOLS_IMPLEMENTATION
+ PUBLIC SPIRV_TOOLS_SHAREDLIB
+ )
+endif()
+
if("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux")
find_library(LIBRT rt)
if(LIBRT)
(END)[ 92%] Linking CXX executable spirv-val
/usr/bin/ld: CMakeFiles/spirv-val.dir/val/val.cpp.o: in function `print_usage(char*)':
val.cpp:(.text+0x26): undefined reference to `spvTargetEnvList[abi:cxx11](int, int)'
/usr/bin/ld: CMakeFiles/spirv-val.dir/val/val.cpp.o: in function `main':
val.cpp:(.text.startup+0x102): undefined reference to `spvParseUniversalLimitsOptions(char const*, spv_validator_limit*)'
/usr/bin/ld: val.cpp:(.text.startup+0x4b7): undefined reference to `spvtools::SpirvTools::SpirvTools(spv_target_env)'
/usr/bin/ld: val.cpp:(.text.startup+0x4ef): undefined reference to `spvtools::SpirvTools::SetMessageConsumer(std::function<void (spv_message_level_t, char const*, spv_position_t const&, char const*)>)'
/usr/bin/ld: val.cpp:(.text.startup+0x524): undefined reference to `spvtools::SpirvTools::Validate(unsigned int const*, unsigned long, spv_validator_options_t*) const'
/usr/bin/ld: val.cpp:(.text.startup+0x535): undefined reference to `spvtools::SpirvTools::~SpirvTools()'
collect2: error: ld returned 1 exit status
make[2]: *** [tools/CMakeFiles/spirv-val.dir/build.make:115: tools/spirv-val] Error 1
make[1]: *** [CMakeFiles/Makefile2:1203: tools/CMakeFiles/spirv-val.dir/all] Error 2
make: *** [Makefile:146: all] Error 2something i can't see here SPIRV-Tools/tools/CMakeLists.txt Lines 43 to 68 in b469957 if i understand rigth, these programs needs full visibility, but then build as shared hidden it (or partial), need to be exposed all? or this programs when build, static or shared, is linked always with static libs?
can use the new convention with a simply rebuild. is possible can exist a wild projects with use the libraries directly instead of use
until now. all projects wich use this linbrary right now needs, or simply rebuild or change the build system (i assume none on this changes modify any part of the core) and about package maintainers: @lordheavy, can you take a little look on this and share your opinion? (archlinux SPIRV-Tools (and Vulkan stack) packages maintainer) feel to free invoke other distribution maintainers greetings |
|
i have done a V3. build ok only static, only shared and both. also pases each ctest tested with GLSLANG and VKD3D in local. thge first one fails about missing symbols when build as shared. for that i enabled the visibility on the the list of the V3 changes is:
TODO:
__ point 4 need acomplished by devs. i'am completely out of this scope |
- Build static unconditionally. but is only used for link all `spirv-foo` programs. because all of them (or partial) need all symbols which is only provided by static libs (until the point 4 get fixed)(*). for install it. see below
This can increase the size a bit.
- Now exist two defines witch control what type of library is installed/builded*:
- `BUILD_SHARED_LIBS` -> This control if you want build shared libs. and if is builded, install it when invoke `make install`
- `SPIRV_INSTALL_STATIC_LIBS` -> This not control the build static library (because is ON unconditionality), but can control the installation when invoke `make install`
I do this because some distros needs only static libs, other only shared libs, other both, or other simply what install only the programs
--- Table of individual use:
- `BUILD_SHARED_LIBS=OFF` or not set -> Disable build and installation of shared libraries
- `BUILD_SHARED_LIBS=ON` -> Enable build and install shared libs
- `SPIRV_INSTALL_STATIC_LIBS=OFF` -> Disable the installation the libraries when invoke `make install`
- `SPIRV_INSTALL_STATIC_LIBS=ON` or not set -> Option ON by default. Enable the installation when invoke `make install`
--- Table of conjuntion use:
- `BUILD_SHARED_LIBS=ON` and `SPIRV_INSTALL_STATIC_LIBS=ON` -> Build and install the wrole project. include programs, static and shared libs.
- `BUILD_SHARED_LIBS=OFF` and `SPIRV_INSTALL_STATIC_LIBS=ON` -> Build the programs and static libs, and install the static libs and the programs when invoke `make install` (Default).
- `BUILD_SHARED_LIBS=ON` and `SPIRV_INSTALL_STATIC_LIBS=OFF` -> build the programs, shared and static libs, and only install the programs and the shared libs when invoke `make install`.
- `BUILD_SHARED_LIBS=OFF` and `SPIRV_INSTALL_STATIC_LIBS=OFF` -> build the programs and the static libs, but not install when invoke `make install`
In all cases, always install the headers
- The generation of the `.cmake` only make one flavor, but depend of the options is used, include shared, static, or both. when enable only static lib, the `.cmake` files point to `static` libs, when build only shared libs, points to `shared` libs,
in this case, static and shared uses the both cases, uses the same TARGET (`libSPIRV-Tools>foo<`)
If enabled both, the TARGET of the static libs is untouched (`libSPIRV-Tools>foo<`), but the shared libs add the surfix `-shared` (`libSPIRV-Tools>foo<-shared`)
-- This need HARD test. this need a CMakeLists.txt test case wich can get the info when use static libs or use shared libs. and test if can linked with other things.
I have tested with GLSLANG, and seems work as expected. in a VKD3D project (uses pkg-config), i need remove the `-shared` in the configure.am file for found the correct library.. build ok and linked the libraries as expected (*)
- The files `libSPIRV-Tools-shared.so` and `SPIRV-Tools-shared.pc` is completely gone. no need anymore.
- By design, the common `SPIRV-Tools.pc` can be used in both build modes. the `pkg-config` stack can handle the information of static libs with `--static` flag when exist static libs in the library path, and normal use when is used as shared lib.
Maybe need ajust for include this information (if exist in the project) into `SPIRV-Tools.pc.in` and setup in the `CMakeFiles.txt`.
- (*)The visiblility of the shared/static modules is untouched. BUT the `libSPIRV-Tools.so` (static `libSPIRV-Tools.a` is always visible) is turn to visible. needs a internal rework (point 4). i can't link with any library (tested with GLSLANG or VKD3D). Always fails to link about missing symbols.
Needs fix by dev.
TODO:
- Fix point (2) and point (3)
- Point (4) is out of my scope. i'm not coder
sl1pkn07
force-pushed
the
master
branch
5 times, most recently
from
22c35d2 to
82141d8
Compare
-DBUILD_SHARED_LIBS=ONTODO:
Fix point (2) and point (3)
Point (4) is out of my scope. i'm not coder