Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow KTX_VERSION_FULL to be manually specified #922

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

xoofx
Copy link
Contributor

@xoofx xoofx commented Jun 23, 2024

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.

@CLAassistant
Copy link

CLAassistant commented Jun 23, 2024

CLA assistant check
All committers have signed the CLA.

@MarkCallow
Copy link
Collaborator

(automatically from the tar.gz downloaded)

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

@xoofx
Copy link
Contributor Author

xoofx commented Jun 27, 2024

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.

@MarkCallow
Copy link
Collaborator

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.

@xoofx
Copy link
Contributor Author

xoofx commented Jun 27, 2024

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
Copy link
Contributor Author

xoofx commented Jun 27, 2024

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
Copy link
Contributor Author

xoofx commented Jun 27, 2024

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

@MarkCallow
Copy link
Collaborator

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
Copy link
Collaborator

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?

@xoofx
Copy link
Contributor Author

xoofx commented Jul 2, 2024

Are you working on this?

Yes, I will do it on my spare time.

@xoofx
Copy link
Contributor Author

xoofx commented Jul 19, 2024

@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
Copy link
Contributor Author

xoofx commented Jul 19, 2024

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
Copy link
Contributor Author

xoofx commented Jul 19, 2024

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.

@MarkCallow
Copy link
Collaborator

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
Copy link
Collaborator

MarkCallow commented Jul 20, 2024

@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
Copy link
Collaborator

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.

@xoofx
Copy link
Contributor Author

xoofx commented Jul 22, 2024

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:

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

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.

cmake/version.cmake Outdated Show resolved Hide resolved
@MarkCallow
Copy link
Collaborator

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 MarkCallow merged commit 6f54c86 into KhronosGroup:main Jul 24, 2024
17 checks passed
MarkCallow added a commit that referenced this pull request Aug 6, 2024
1.3.290 is the first release of official iOS support and first with support
for Windows arm64.

Stop using binaries of SDL2, assimp and glew libraries stored in repo
because an updated SDL2 is needed for 1.3.290 and more platforms are
now supported. Dependencies for iOS, macOS and Windows are now obtained
through vcpkg. Those for Linux through the package manager as before.

Thus remove other_lib, other_include/{SDL2,assimp} and
other_include/GL/{glew,glxew,wglew}.h. Move the modified Vulkan
manifest files to tests/loadtests/appfwSDL/VulkanAppSDL.

This incidentally fixes #763.

These changes plus removal of the hacks for finding the iOS components
in older Vulkan SDKs means a major revamp of the load tests-related
CMake files.

vcpkg is used in manifest mode so installation of dependencies is
automatic once vcpkg is installted. This use of vcpkg led to a reshuffle
of the root CMakeLists.txt to put some options before the project()
comment so their values can be used to set VCPKG-related variables.

Deployment target for iOS is bumped to 12.0 and for macOS to 11.0 to
match what is supported by this Vulkan SDK.

The PR also fixes some unrelated problems:

1. A syntax error in scripts/mkversion most likely introduced by PR
    #922. It is unclear why CI did not fail with this.
2. Use of a query in `scripts/build_win.ps1 that returned process
    architecture not machine architecture. VS2022 Developer PowerShells
    are x86 processes leading to failure with the old query. Use
    Get-ComputerInfo instead.
3. Also in `scripts/build_win.ps1`, a warning when `PYTHON` is defined
    on the CMake configure command line if FEATURE_PYTHON is not ON.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants