KhronosGroup / KTX-Software

KTX (Khronos Texture) Library and Tools
Other
854 stars 226 forks source link

using libKTX on Linux when linking report errors on Threads::Threads #831

Closed Raffaello closed 7 months ago

Raffaello commented 7 months ago

I have tried to use the RPM 4.3-beta modified from this PR #830 in a project.

when compiliing it is reporting that cannot find Threads::Threads

I have tried to change the PUBLIC here: https://github.com/KhronosGroup/KTX-Software/blob/main/CMakeLists.txt#L649

to PRIVATE

and that issue seems resolved.

Raffaello commented 7 months ago

@MarkCallow let me know if i should do the change and i will do in the PR #830 if it is ok for you.

Raffaello commented 7 months ago

both Fedora and Ubuntu report the error with PUBLIC, i will commit the change in #830

MarkCallow commented 7 months ago

Are you linking to a static or shared libktx?

What is reporting it cannot find Threads::Threads? That should not passed to the linker. That is a CMake target created by find_package(Threads REQUIRED). The CMake generator should be converting that to the actual library provided by that target.

If using a static libktx the app will need to link with the threads and dl libraries so in that case I think these things still need to be public.

Please remove the fix from PR #830 and open a new PR. I want to merge the fix for #827 and am as yet unsure about the fix you are proposing for this issue.

Raffaello commented 7 months ago

reverted the change in #830 .

i am linking to shared: using KTX::ktx in target_link_libraries.:

target_link_libraries(${PROJECT_NAME} PUBLIC
    KTX::ktx
)

The error is:

[cmake] CMake Error at /usr/lib/cmake/ktx/KtxTargets.cmake:56 (set_target_properties):
[cmake]   The link interface of target "KTX::ktx" contains:
[cmake] 
[cmake]     Threads::Threads
[cmake] 
[cmake]   but the target was not found.  Possible reasons include:
[cmake] 
[cmake]     * There is a typo in the target name.
[cmake]     * A find_package call is missing for an IMPORTED target.
[cmake]     * An ALIAS target is missing.
[cmake] 
[cmake] Call Stack (most recent call first):
[cmake]   /usr/lib/cmake/ktx/KtxConfig.cmake:7 (include)
[cmake]   CMakeLists.txt:24 (find_package)
[cmake] 

The file KtxTargets.cmake at line 56 has this statement:

set_target_properties(KTX::ktx PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "\$<\$<CONFIG:Debug>:_DEBUG;DEBUG>;KTX_FEATURE_KTX1;KTX_FEATURE_KTX2;KTX_FEATURE_WRITE"
  INTERFACE_COMPILE_FEATURES "c_std_99;cxx_std_11"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "dl;Threads::Threads"
)

if that would be the case with static, maybe a cmake "if(static) private else public" could do the trick eventually...

MarkCallow commented 7 months ago

Thanks for the additional info. So you are using a shared library, correct?

Two things are unclear to me:

  1. Does CMake handle interfaces differently when the target is a static library vs a shared library. E.g, does it make pricate interfaces public.
  2. Does CMake generate a different KtxTargets.cmake when the target is a static library.

Once we get answers we can decide the proper fix. What is clear is that when using a static library the application has to link with whatever libraries the static library is dependent on. There may be some tool chains that hide that need from the application programmer.

Raffaello commented 7 months ago
  1. when compiling with PRIVATE the same statement at KtxTargets:56 is:

    set_target_properties(KTX::ktx PROPERTIES
    INTERFACE_COMPILE_DEFINITIONS "\$<\$<CONFIG:Debug>:_DEBUG;DEBUG>;KTX_FEATURE_KTX1;KTX_FEATURE_KTX2;KTX_FEATURE_WRITE"
    INTERFACE_COMPILE_FEATURES "c_std_99;cxx_std_11"
    INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
    )

    it misses the last line INTERFACE_LINK_LIBRARIES "dl;Threads::Threads".

  2. i don't think CMake is smart enough yet on that. What it does is with PUBLIC having those as transitive dependencies, with PRIVATE those dependency are only linked and required for the library itself. This scenario should open the fact that a shared library should be most likely PRIVATE and a static library most likely PUBLIC (because the static library will be embedded in the "exe" and therefore the "exe" requires should/must also include the required dependencies of the library that is statically linking to)

I guess that is the theory, but not 100% sure on it, I didn't check anything on a KTX-static yet. i found a small thread on that PUBLIC vs PRIVATE discussion: https://discourse.cmake.org/t/the-usage-of-target-include-directories-vs-target-link-libraries/7914/10 that might enlight a little bit more on it.

Raffaello commented 7 months ago

I think a possible patch could be (draft):

elseif(LINUX)
        find_package(Threads REQUIRED)
        if(KTX_FEATURE_STATIC_LIBRARY)
            target_link_libraries(
                ${target}
            PUBLIC
                dl
                Threads::Threads
            )
        else()
                target_link_libraries(
                ${target}
            PRIVATE
                dl
                Threads::Threads
            )
        endif()
    endif()

but not sure if Threads::Threads for e.g. must be included in an app not using it directly, but only because is transitive... :thinking:

MarkCallow commented 7 months ago

A Happy New Year to you @Raffaello.

Thank you for the info and suggestion. I do not think the link you provided really answers the question. I did find another link, https://cmake.cmake.narkive.com/t0Dq2KUO/difference-between-private-and-public-with-target-link-libraries with a definitive answer. Follow this permalink to the definitive answer. It says that CMake treats PRIVATE in target_link_libraries as PUBLIC when target is a static library. If true, your original fix would be fine. For confirmation I'd like to find some actual CMake documentation that says this. It makes sense that this would be the behaviour.

Reading on through the thread the author of the message at the permalink says he tested with CMake 3.5.2 and it worked as he describes. @Raffaello please open a PR with your original fix of changing PUBLIC to PRIVATE.

Raffaello commented 7 months ago

@MarkCallow Happy New Year to you too! 🥳

So based on that peramlink, it will PRIVATE.

ah, ok. I will do. 👍