Fix an issue with the generated KtxTargets.cmake #325

UX3D-becher · 2020-10-08T14:09:09Z

Fixes issue #326

In this pull request we changed the problematic dependencies of the ktx target in the root CMakeLists.txt from PUBLIC to PRIVATE and fixed configuration errors in other cmake files. These errors mainly stem from the use of the INTERFACE_INCLUDE_DIRECTORIES property of the ktx that after the above change does not contain the private include paths any more.

CLAassistant · 2020-10-08T14:09:14Z

All committers have signed the CLA.

UX3D-nopper · 2020-10-08T14:11:44Z

@MarkCallow Can u pls review?

MarkCallow · 2020-10-08T21:16:24Z

Please create an issue. Note that there is a large change coming to the CMake files to add a libktx_read that can only read KTX files. I Plan to merge that PR (#324) first. You will have to rebase this after the merge if, as I expect it to be, the bug is still present.

MarkCallow · 2020-10-09T00:06:44Z

CMakeLists.txt

@@ -195,7 +195,7 @@ target_include_directories(
 PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
    $<INSTALL_INTERFACE:include>
-
+PRIVATE
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lib/basisu/transcoder>
    $<INSTALL_INTERFACE:lib/basisu/transcoder>


I wonder if this line and all similar are really necessary. I've requested a review from @atteneder.

While this changes are not necessary, I agree it makes sense to strictly differentiate between public and private(internal) headers.

I am referring to the INSTALL_INTERFACE line. lib/basisu/transcoder is not installed. It is purely internal to the libktx. Hence my question.

There shouldn't be any need to actually make and install a transcoder library. If shared libktx is being used I don't think anybody cares if the whole libktx is loaded into memory. If a static library is being used and only the transcoder is referenced, the linker will link only the transcoder functions into the application.

MarkCallow · 2020-10-09T00:10:55Z

CMakeLists.txt

@@ -329,7 +329,7 @@ set(KTX_BUILD_DIR "${CMAKE_BINARY_DIR}")

 if(WIN32)
    target_link_libraries(
-        ktx
+        ktx PRIVATE


This and related lines will have to change after PR #324 is merged as, if the built library is a static library (an option added by PR #324), libzstd will be required by the application.

Wasn't aware of this. I'd need to test.

libktx has references to functions in libzstd. If you are building a shared library these will cause libzstd to be linked into the shared libktx. If static, the app needs to link with libzstd to satisfy the references.

MarkCallow · 2020-10-09T00:13:42Z

CMakeLists.txt

@@ -396,7 +401,7 @@ if(KTX_FEATURE_VULKAN)
    )
    target_include_directories(
        ktx
-    PUBLIC
+    PRIVATE
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lib/dfdutils>
        $<INSTALL_INTERFACE:lib/dfdutils>


This is also going to be needed by the app when linking with a static library which is a problem as there is currently no separate release of the dfdutils library.

Do users of KTX need to use dfdutils (dfd.h) directly commonly?

A clean solution could be to create a CMake config for dfdutils as well and link to its CMake target.
Users who need dfdutils directly could direclty refer to it.

Quite possibly, if the app wishes to examine the DFD. At present only a small part of the information it contains is made available through the libktx API.

I agree we should make a CMake config for it, as you suggest @atteneder. dfdutils is included via git subrepo from https://github.com/KhronosGroup/dfdutils. I need to push some recent additions and fixes back there and we should add the CMake config there.

atteneder · 2020-10-10T21:40:51Z

CMakeLists.txt

@@ -195,7 +195,7 @@ target_include_directories(
 PUBLIC
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
    $<INSTALL_INTERFACE:include>
-
+PRIVATE
    $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lib/basisu/transcoder>
    $<INSTALL_INTERFACE:lib/basisu/transcoder>


While this changes are not necessary, I agree it makes sense to strictly differentiate between public and private(internal) headers.

atteneder · 2020-10-10T21:43:20Z

CMakeLists.txt

@@ -329,7 +329,7 @@ set(KTX_BUILD_DIR "${CMAKE_BINARY_DIR}")

 if(WIN32)
    target_link_libraries(
-        ktx
+        ktx PRIVATE


Wasn't aware of this. I'd need to test.

atteneder · 2020-10-10T21:55:43Z

CMakeLists.txt

@@ -396,7 +401,7 @@ if(KTX_FEATURE_VULKAN)
    )
    target_include_directories(
        ktx
-    PUBLIC
+    PRIVATE
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/lib/dfdutils>
        $<INSTALL_INTERFACE:lib/dfdutils>


Do users of KTX need to use dfdutils (dfd.h) directly commonly?

A clean solution could be to create a CMake config for dfdutils as well and link to its CMake target.
Users who need dfdutils directly could direclty refer to it.

atteneder · 2020-10-10T21:56:49Z

interface/basisu_c_binding/CMakeLists.txt

@@ -22,7 +22,7 @@ target_include_directories(
 PUBLIC
    inc
 PRIVATE
-    $<TARGET_PROPERTY:ktx,INTERFACE_INCLUDE_DIRECTORIES>
+$<TARGET_PROPERTY:ktx,INCLUDE_DIRECTORIES>


I'd appreciate consistent indentation. Same in tests.cmake:42

MarkCallow · 2020-10-13T19:56:26Z

PR #324 has been merged. Please rebase this against the new master, make the changes necessary for the static library case and remove lib/basisu/transcoder from the INSTALL_INTERFACE. We will work separately on a CMake config for the dfdutils. Note the dfdutils will be in the library archive sohat an application needs in order to use them are dfd.h and khr_df.h. We should add those to the installers and the INSTALL_INTERFACE here. Please add these changes to this PR.

MarkCallow · 2020-10-25T19:06:58Z

Please update this PR.

# Conflicts: # CMakeLists.txt

MarkCallow · 2020-11-16T10:35:44Z

Travis CI has not built this or the other open PR. I don't at present know why. There has been no change in the .travis.yml since the last time Travis did a PR build. I can't merge this until we have a successful build. If you have any ideas, please let me know.

MarkCallow · 2020-11-19T05:58:46Z

The missing Travis PR build has something to do with Khronos's Travis membership. It will probably take a few days to sort out. Please wait.

MarkCallow

I am repeating my question from my first review. Please answer.

CMakeLists.txt

atteneder · 2020-11-19T08:33:08Z

Sorry for late reply (I'm preparing a relocation myself now).
I try to have a look in the afternoon.

MarkCallow · 2020-11-19T09:20:43Z

Thanks for the explanation. Now we have to wait for Khronos to resolve the issue around Travis CI builds.

Note there is a PR #335 adding support for building on Android which makes many changes to the CMake files some of which I expect will overlap with these. I plan to merge this PR first as it has been open for some time. That means PR #335 will need rebasing.

atteneder · 2020-11-19T14:28:35Z

Sorry for late reply (I'm preparing a relocation myself now).
I try to have a look in the afternoon.

Oh, I see you already cleared everything up. Nonetheless I tested on Linux and found no issues!
Thanks!

MarkCallow · 2020-11-20T00:14:17Z

Khronos has resolved the issue with Travis CI. I triggered a redelivery of the webhook payload for the latest commit in this PR but it has not triggered a build. @UX3D-becher please push something to this PR to trigger a build so we can test the Linux, macOS and Web builds with these changes.

MarkCallow · 2020-11-20T23:57:40Z

Please do a push to trigger a Travis CI build so we can check the iOS, Linux & macOS builds. I want to get this PR merged.

MarkCallow · 2020-11-25T08:14:22Z

You need to make a small change and push it to this repo to trigger a Travis-CI build. I have tried to redelivering the webhook requests from the previous commits from GitHub but it is not working. Until we get successful iOS, Linux, macOS and web builds (all of which are done on Travis) I cannot merge this PR.

MarkCallow · 2020-11-26T13:05:52Z

@UX3D-becher trigger a build or else I will have to close this PR without merging.

MarkCallow · 2020-12-02T05:01:53Z

Unfortunately Khronos seems have used up all its credits with Travis CI so the builds have stopped again. It is infuriating. Trying to get it resolved. I may need you to push a commit again. Perhaps lack of credits is the reason re-sending the WebHook message for a previous commit failed to trigger a build. I'll try that again first once the problem is sorted.

MarkCallow

Been away a couple of days. Sorry for the delay. The change looks good to my limited knowledge of Android. Is zstd included as a standard feature of Android? I'm going to request a review from Andreas.

MarkCallow · 2020-12-03T09:08:40Z

The problem blocking the macOS & Linux builds has reportedly been fixed, however redelivering GitHub webhook payloads to Travis has no visible effect so I have to again ask you to push another commit to this PR to trigger a build. Please do it as soon as possible.

UX3D-becher · 2020-12-09T16:43:43Z

I cleaned some whitespace to trigger a build, hopefully these changes were in your interest anyway

MarkCallow · 2020-12-11T07:23:20Z

Thanks for making it easier to use libktx is other projects.

Make it possible to use the find_package functionality of cmake to use libktx as imported target.

make ktx target includes and libs private

5f5b40b

UX3D-becher added 3 commits October 8, 2020 16:43

fixed more INTERFACE_INCLUDE_DIRECTORIES

4875a85

only zstd private linux library dependency

4f903cd

fixed cmake syntax

e915476

UX3D-becher mentioned this pull request Oct 8, 2020

cmake config-file imported target broken #326

Closed

MarkCallow requested a review from atteneder October 9, 2020 00:05

MarkCallow requested changes Oct 9, 2020

View reviewed changes

moved Threads::Threads to private dependencies

c68ee1d

atteneder approved these changes Oct 10, 2020

View reviewed changes

UX3D-becher added 2 commits November 16, 2020 08:59

Merge remote-tracking branch 'origin/master' into cmake-config-file-fix

9fa02b6

# Conflicts: # CMakeLists.txt

private include and library dependencies

255e4f2

UX3D-becher added 2 commits November 16, 2020 12:10

fix indentation in tests.cmake

cef4e63

fix indentation in basiu_c_binding

2fd8b91

MarkCallow requested changes Nov 19, 2020

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

MarkCallow approved these changes Nov 19, 2020

View reviewed changes

MarkCallow mentioned this pull request Nov 21, 2020

Android Build #335

Closed

set android zstd link to PRIVATE

28571a0

UX3D-becher requested a review from MarkCallow November 30, 2020 09:35

MarkCallow reviewed Dec 2, 2020

View reviewed changes

MarkCallow requested a review from atteneder December 2, 2020 05:04

whitespace cleanup to trigger travis build

30255ec

MarkCallow merged commit 9131fba into KhronosGroup:master Dec 11, 2020

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fix an issue with the generated KtxTargets.cmake (KhronosGroup#325)

e6de103

Make it possible to use the find_package functionality of cmake to use libktx as imported target.

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fix an issue with the generated KtxTargets.cmake (KhronosGroup#325)

4b8998b

Make it possible to use the find_package functionality of cmake to use libktx as imported target.

KaperD pushed a commit to KaperD/KTX-Software that referenced this pull request Feb 22, 2024

Fix an issue with the generated KtxTargets.cmake (KhronosGroup#325)

3e4263f

Make it possible to use the find_package functionality of cmake to use libktx as imported target.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix an issue with the generated KtxTargets.cmake #325

Fix an issue with the generated KtxTargets.cmake #325

UX3D-becher commented Oct 8, 2020 •

edited

Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading

UX3D-nopper commented Oct 8, 2020

MarkCallow commented Oct 8, 2020

MarkCallow Oct 9, 2020

atteneder Oct 10, 2020

MarkCallow Oct 10, 2020

MarkCallow Oct 9, 2020 •

edited

Loading

atteneder Oct 10, 2020

MarkCallow Oct 10, 2020

MarkCallow Oct 9, 2020

atteneder Oct 10, 2020

MarkCallow Oct 10, 2020

atteneder Oct 10, 2020

atteneder Oct 10, 2020

atteneder Oct 10, 2020

atteneder Oct 10, 2020

MarkCallow commented Oct 13, 2020

MarkCallow commented Oct 25, 2020

MarkCallow commented Nov 16, 2020

MarkCallow commented Nov 19, 2020

MarkCallow left a comment

atteneder commented Nov 19, 2020

MarkCallow commented Nov 19, 2020

atteneder commented Nov 19, 2020

MarkCallow commented Nov 20, 2020

MarkCallow commented Nov 20, 2020

MarkCallow commented Nov 25, 2020

MarkCallow commented Nov 26, 2020

MarkCallow commented Dec 2, 2020 •

edited

Loading

MarkCallow left a comment

MarkCallow commented Dec 3, 2020 •

edited

Loading

UX3D-becher commented Dec 9, 2020

MarkCallow commented Dec 11, 2020

Fix an issue with the generated KtxTargets.cmake #325

Fix an issue with the generated KtxTargets.cmake #325

Conversation

UX3D-becher commented Oct 8, 2020 • edited Loading

CLAassistant commented Oct 8, 2020 • edited Loading

UX3D-nopper commented Oct 8, 2020

MarkCallow commented Oct 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow Oct 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkCallow commented Oct 13, 2020

MarkCallow commented Oct 25, 2020

MarkCallow commented Nov 16, 2020

MarkCallow commented Nov 19, 2020

MarkCallow left a comment

Choose a reason for hiding this comment

atteneder commented Nov 19, 2020

MarkCallow commented Nov 19, 2020

atteneder commented Nov 19, 2020

MarkCallow commented Nov 20, 2020

MarkCallow commented Nov 20, 2020

MarkCallow commented Nov 25, 2020

MarkCallow commented Nov 26, 2020

MarkCallow commented Dec 2, 2020 • edited Loading

MarkCallow left a comment

Choose a reason for hiding this comment

MarkCallow commented Dec 3, 2020 • edited Loading

UX3D-becher commented Dec 9, 2020

MarkCallow commented Dec 11, 2020

UX3D-becher commented Oct 8, 2020 •

edited

Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading

MarkCallow Oct 9, 2020 •

edited

Loading

MarkCallow commented Dec 2, 2020 •

edited

Loading

MarkCallow commented Dec 3, 2020 •

edited

Loading