KTX-Software icon indicating copy to clipboard operation
KTX-Software copied to clipboard

Allow KTX_VERSION_FULL to be manually specified

Open xoofx opened this issue 1 year ago • 8 comments

Hello,

I would like to add ktx as a package to Alpine Linux, and one requirement is to be able to build from a tar.gz published from v4.3.2) but the current cmake/version.cmake is forcing to build from the original git repo, while when building within Alpine aports, we specify the version ourselves as part of the Alpine build system (automatically from the tar.gz downloaded).

This PR is making a small change to cmake/version.cmake to allow to pass KTX_VERSION_FULL to cmake.

xoofx avatar Jun 23 '24 12:06 xoofx

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jun 23 '24 12:06 CLAassistant

(automatically from the tar.gz downloaded)

What is the information contained in the downloaded tar.gz? Is it the name of the git tag?

MarkCallow avatar Jun 27 '24 09:06 MarkCallow

What is the information contained in the downloaded tar.gz? Is it the name of the git tag?

Oh, maybe this was not clear.

When we go to the release tab here to the latest version https://github.com/KhronosGroup/KTX-Software/releases/tag/v4.3.2

At the bottom we can download the tar.gz snapshot of this repository:

image

Which gives a link to https://github.com/KhronosGroup/KTX-Software/archive/refs/tags/v4.3.2.tar.gz

If you download this file and try to build from source, you will get a 0.0.0.0 version number because it is failing to resolve the Version number from git.

This PR is allowing to specify the version to cmake when we know about it (we download a specific version through a tag), so we can build this repository without doing a git checkout by just using the artifacts. This is a requirement for Alpine Linux to be able to build from tar.gz.

Hope that clarifies.

xoofx avatar Jun 27 '24 09:06 xoofx

The only bit that wasn't, and still isn't, clear is "automatically from the tar.gz downloaded". I get you download the artifacts for a particular tag and I get that absent the git repo the version.cmake does not produce a version. Is the automatic part something in the tar.gz, which is implied by "from" or is it that your scripts request a particular tag and use that same tag name when creating the full version?

I want to understand how easy or otherwise it is for your system to create a totally nonsense version number.

In this scenario how do you correctly create the various version.h files that are included in the commands to support their --version queries? These are create by scripts/mkversion but I think also rely on having the git repo.

MarkCallow avatar Jun 27 '24 10:06 MarkCallow

I want to understand how easy or otherwise it is for your system to create a totally nonsense version number.

Ok, the number is specified when we download a version. So, it's not totally nonsense, because it is the actual tag on this repo. And this repo is using the actual tag to make the version number, so we are matching this accordingly. But to explain this further, the following should explain how packages in Alpine Linux are built:

Alpine linux is using a custom build system to build packages. This is stored in an APKBUILD file in their repo.

For example, at this line, You see the version number: https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/community/allegro/APKBUILD#L4

# Contributor: Bart Ribbers <[email protected]>
# Maintainer: Bart Ribbers <[email protected]>
pkgname=allegro
pkgver=5.2.9.1

At the following line here https://gitlab.alpinelinux.org/alpine/aports/-/blob/master/community/allegro/APKBUILD#L27 you have the URL to GitHub to download the package:

source="https://github.com/liballeg/allegro5/releases/download/$pkgver/allegro-$pkgver.tar.gz"
subpackages="$pkgname-dev"

The download package is matching the git version.

In this scenario how do you correctly create the various version.h files that are included in the commands to support their --version queries? These are create by scripts/mkversion but I think also rely on having the git repo.

With this PR, I can pass the $pkgver to KTX_VERSION_FULL and it should propagate within the build system. That's what I got from the binaries that I built. Do you mean that outside of cmake, there are other places that we fetch the git tag?

xoofx avatar Jun 27 '24 10:06 xoofx

That's what I got from the binaries that I buil

That being said, I think I checked only libktx, but looking at cmake scripts, for tools, it looks like we are delegating this to scripts/mkversion via create_version_header that is creating that for each tool. Is that correct?

xoofx avatar Jun 27 '24 10:06 xoofx

I could propagate the tag to the mkversion with e.g a special argument like --version or something like this. Would that be enough?

xoofx avatar Jun 27 '24 10:06 xoofx

scripts/mkversion predates use of CMake and creation of cmake/version.cmake. I don't know if it is reasonable to combine them. Passing a --version argument as you propose seems like a reasonable alternative.

MarkCallow avatar Jun 27 '24 13:06 MarkCallow

I could propagate the tag to the mkversion with e.g a special argument like --version or something like this. Would that be enough?

Are you working on this?

MarkCallow avatar Jul 02 '24 13:07 MarkCallow

Are you working on this?

Yes, I will do it on my spare time.

xoofx avatar Jul 02 '24 13:07 xoofx

@MarkCallow so I have pushed a fix that propagates the version to mkversion, but it seems that there was a discrepancy between the version expected for tools (prefixed with v) and the lib itself.

All tool tests are checking that the --version is including the prefix v:

set_tests_properties(
    ktx2ktx2-test.version
PROPERTIES
    PASS_REGULAR_EXPRESSION "^ktx2ktx2 v[0-9][0-9\\.]+"
)

What would you prefer to see? Keeping the existing behavior or normalizing version across all tools so that 4.3.2 is printed for both the library and all the tools instead of v4.3.2?

xoofx avatar Jul 19 '24 10:07 xoofx

Oh, actually, that's because a few lines later in version.cmake KTX_VERSION_FULL is overridden and discarding the v, so should it keep the original version with v or not?

set(KTX_VERSION_FULL ${KTX_VERSION}${KTX_VERSION_TWEAK})

xoofx avatar Jul 19 '24 12:07 xoofx

I have started to open a PR on alpine with the latest fixes from this PR here to give an example of how the KTX package will be prepared there.

xoofx avatar Jul 19 '24 13:07 xoofx

The intent is that the version emitted by a tool's --version option can be fed direct to git checkout to checkout the matching source code so the 'v' is required. The library version needs the 'v' for the same reason. It is used in KTXwriter metadata. However KTX_VERSION_FULL is used to name the release packages, e.g., KTX-Software-4.3.2-*. We don't want the 'v' in those.

That you can pass -DKTX_VERSION_FULL to CMake should be documented somewhere. I don't think it should be a cache variable given how easy it is to inadvertently have CMake used values stored in the cache. BUILDING.md is the obvious document but there is no obvious section to put it in. Maybe we need to make a new very short section. The documentation needs to be accompanied by warnings not to use it except it special circumstances.

MarkCallow avatar Jul 20 '24 06:07 MarkCallow

@xoofx, In your PR you have assimp-dev as one of the dependencies. Assimp is only needed by KTX_FEATURE_LOADTEST_APPS and you are not configuring that.

MarkCallow avatar Jul 20 '24 07:07 MarkCallow

The name change to KTX_GIT_VERSION_FULL breaks the package builds in CMakeLists.txt because they rely on KTX_VERSION_FULL being set by cmake/version.cmake. This won't show up in CI at this point because packages are only built when release tags are created.

MarkCallow avatar Jul 22 '24 09:07 MarkCallow

The name change to KTX_GIT_VERSION_FULL breaks the package builds in CMakeLists.txt because they rely on KTX_VERSION_FULL being set by cmake/version.cmake. This won't show up in CI at this point because packages are only built when release tags are created.

But as I said earlier, KTX_VERSION_FULL is still set in my PR by this line that I haven't changed:

https://github.com/KhronosGroup/KTX-Software/blob/110f049fde285893bb2cc971c697dfc3abba8436/cmake/version.cmake#L160

This line was actually overriding KTX_VERSION_FULL returned by git describe earlier. Now the two are distincts so that KTX_GIT_VERSION_FULL helps here to propagate it to mkversion.

xoofx avatar Jul 22 '24 10:07 xoofx

But as I said earlier, KTX_VERSION_FULL is still set in my PR by this line that I haven't changed:

Sorry. I missed that line.

MarkCallow avatar Jul 22 '24 11:07 MarkCallow