vcpkg patching the sources - zstd???

[Zingam]

In vcpkg they are patching the library:
https://github.com/microsoft/vcpkg/tree/master/ports/ktx
Can something be done to avoid that need?

[MarkCallow]

I don't know why they are doing that. The purpose of the patch seems to be to replace the internal zstd copy with the zstd package. I don't see why it is necessary. The zstd source within KTX-Software builds and works just fine in VC++ on Windows, gcc on Mingw and Linux and clang on macOS and iOS. Find out why they don't want to use our internal source.

[Zingam]

When was zstd introduced? Looking at the history it seems this patch was introduced for KTX 4.0.0-beta5.

What about the versioning patch? It seems necessary for getting the exact tag?

[MarkCallow]

What about the versioning patch? It seems necessary for getting the exact tag?

I don't know why they need it. It looks like it might be an optimization. If KTX_FULL_VERSION is already set don't call git describe again.

You'll have to ask the person who created the patch. I suggest you file an issue over there telling them they don't need the zstd patch because zstd is now included in the KTX-Software source and asking why they have the version patch. If there is something that needs fixing in the versioning over here, ask them to file an issue.

I'm going to close this issue as I don't see any action for me.

[Zingam]

@MarkCallow It looks like this explains why they are patching but does it needs to be and could it be avoided?
microsoft/vcpkg#29756

[MarkCallow]

File a PR with the changes that are needed.

        $<INSTALL_INTERFACE:lib/basisu/zstd>

can be deleted from CMakeLists.txt. It looks like it may be a holdover from when the project linked to an external zstd lib. I'm no CMake expert, so I could be wrong. For sure, clients have no need to refer to zstd headers or link to a zstd library, even when building a static libktx. The internal zstd source should be used. They should not be modifying the build to use an external library.

Issue no. 2 is that the build of the static libktx on macOS uses libtool to combine libktx and the astc-encoder library into a single library for easier use. The GNU libtool does something different. Therefore on linux don't attempt to combine the libraries and when building clients, e.g. the tools, link to both ktx and astcenc-static. I think that was resolved some months ago by a PR I received. If not, include a complete fix in the PR I am requesting here..

[Osyotr]

For sure, clients have no need to refer to zstd headers or link to a zstd library, even when building a static libktx. The internal zstd source should be used. They should not be modifying the build to use an external library.

If a user needs both libktx and zstd they will get a linking error because both libraries provide symbols from zstd.
When vendoring dependencies, always add an escape hatch like this:

option(LIBKTX_USE_EXTERNAL_ZSTD "Use external zstd" OFF)
if(LIBKTX_USE_EXTERNAL_ZSTD)
    find_package(zstd CONFIG REQUIRED)
    target_link_libraries(${lib} PRIVATE $<IF:$<TARGET_EXISTS:zstd::libzstd_shared>,zstd::libzstd_shared,zstd::libzstd_static>)
else()
    target_sources(${lib} PRIVATE "lib/basisu/zstd/zstd.c") # Or similar
endif()

What about the versioning patch? It seems necessary for getting the exact tag?

I don't know why they need it. It looks like it might be an optimization. If KTX_FULL_VERSION is already set don't call git describe again.

There is no git repo in sources from github's generated zip.

The need for 0003-mkversion.patch has been explained here: microsoft/vcpkg#30787 (comment)