libjxl
export version information in headers
Library version information is currently not exported, which means client code cannot conditionally compile code for different API versions.
For example, the recent addition of JxlEncoderOptionsSetInteger along with the deprecation of JxlEncoderOptionsSetEffort means it is now impossible to compile dependent code without raising a compilation error or deprecation warning for code targeting 0.6 and 0.7+, c.f. OSGeo/GDAL#4770
This PR creates a version.h file exporting the JXL library version.
Could this be modified to put the JPEGXL_LIBRARY_SOVERSION in the .h file instead of the major/minor/patch version?
The SOVERSION tells you exactly what API is available, so it is what is needed to write code that can be compiled against different versions.
Could this be modified to put the
JPEGXL_LIBRARY_SOVERSIONin the .h file instead of the major/minor/patch version?The SOVERSION tells you exactly what API is available, so it is what is needed to write code that can be compiled against different versions.
How does this cover the case when e.g. a new function was added to the API? From what I understand in that case the major version is left intact, and the minor version is bumped. Supposing a stable v1+ version, the JPEGXL_LIBRARY_SOVERSION would not be bumped in this case, and as a client I cannot do something like
#if JPEGXL_LIBRARY_SOVERSION >= JXL_COMPUTE_VERSION(1,1)
call_jxl_function_from_10()
call_jxl_function_added_in_11()
#else
call_jxl_function_from_10()
#endif
Could this be modified to put the
JPEGXL_LIBRARY_SOVERSIONin the .h file instead of the major/minor/patch version? The SOVERSION tells you exactly what API is available, so it is what is needed to write code that can be compiled against different versions.How does this cover the case when e.g. a new function was added to the API? From what I understand in that case the major version is left intact, and the minor version is bumped. Supposing a stable v1+ version, the JPEGXL_LIBRARY_SOVERSION would not be bumped in this case, and as a client I cannot do something like
#if JPEGXL_LIBRARY_SOVERSION >= JXL_COMPUTE_VERSION(1,1) call_jxl_function_from_10() call_jxl_function_added_in_11() #else call_jxl_function_from_10() #endif
As far as I understand, the SOVERSION needs to be bumped also when new functions are added — it has to be bumped whenever the ABI changes — and the way SOVERSION is currently defined in lib/CMakeLists.txt is wrong: in principle the SOVERSION is not related to the major.minor.patch version, since a minor version bump can be caused either by ABI-changing new functionality or by just significant improvements (e.g. speedups or encoder density improvements) without any ABI impact.
The major.minor.patch version is related to the API in the sense that major version bumps mean backwards incompatible changes (an application that works with version 3 can stop working with version 4); minor version bumps mean only backwards compatible changes but possibly forward incompatible changes (an application that works with version 3.5 will still work with version 3.6, but it may or may not work with version 3.4); patch version bumps imply no API changes at all. The SOVERSION is approximated by major.minor, but there can be minor version bumps that don't bump the SOVERSION. Similarly, within the same minor version, a patch version bump could in principle change the ABI without changing the API (though this is something we likely want to avoid: if we change the ABI, we likely will want to bump the minor version too).
At least that is how I understand things.
But I think I might understand things incorrectly, see e.g. https://www.debian.org/doc/debian-policy/ch-sharedlibs.html and other sources that say that the SOVERSION can remain the same if only backwards compatible changes are made to the ABI. That means that the SOVERSION alone is not enough to know if a given compiled program can dynamically link with the library though, since it might be using new functions that were introduced within the same SOVERSION. So in that case, the SOVERSION would only be useful to know when a library update cannot be done because something that depends on it might break; the dependency itself would have to be specified using the major.minor.patch version and not just the SOVERSION though.
So if we go by that definition, then after 1.0 the SOVERSION will indeed just be the major version number like it is now in lib/CMakeLists.txt. But if you want to write code that optionally calls new functions, then the SOVERSION alone will not be enough. In that case I think it would make sense to also expose the minor version in the .h file, so you can use macros to conditionally use functions that don't exist in all versions, if you want to write code that can be compiled for e.g. both version 1.4 and version 1.5 while (conditionally) using functionality that was introduced in 1.5.
@lvandeve @veluca93 @eustas what are your thoughts on this?
Extracting version number via pkg-config in build script is possible. Below an example of the script:
pkg_check_modules(LibJXL IMPORTED_TARGET libjxl>=0.6.1)
pkg_check_modules(LibJXLThreads IMPORTED_TARGET libjxl_threads>=0.6.1)
if (LibJXL_FOUND AND LibJXLThreads_FOUND)
kimageformats_add_plugin(kimg_jxl SOURCES jxl.cpp)
target_link_libraries(kimg_jxl PkgConfig::LibJXL PkgConfig::LibJXLThreads)
if (LibJXL_VERSION VERSION_GREATER_EQUAL "0.7.0")
target_compile_definitions(kimg_jxl PRIVATE KIMG_JXL_API_VERSION=70)
endif()
install(FILES jxl.desktop DESTINATION ${KDE_INSTALL_KSERVICESDIR}/qimageioplugins/)
endif()
However with other libraries, everything is much easier, without need to adjust build scripts.
libheif:
#if LIBHEIF_HAVE_VERSION(1,8,0)
Qt:
#if (QT_VERSION >= QT_VERSION_CHECK(5, 14, 0))
libavif:
#if AVIF_VERSION >= 80400
I added the PATCH_VERSION and a slighly different JPEGXL_NUMERIC_VERSION. Any thoughts, @veluca93 @szabadka @eustas?
@tbonfort @novomesk @dlemstra this also aims at closing #1721
LGTM, same as #1721, eagerly waiting for this to land in order to support multiple versions of libjxl in GDAL without resorting to parsing pkg-config output.
The comment still mentions JPEGXL_COMPUTE_LIBRARY_VERSION? Might also be nice to add this and update the comment to also include the patch version? And also use JPEGXL_COMPUTE_NUMERIC_VERSION in the comment instead?
#define JPEGXL_COMPUTE_NUMERIC_VERSION(major,minor,patch) ((major<<24) | (minor<<16) | (patch<<8) | 0)
The comment still mentions
JPEGXL_COMPUTE_LIBRARY_VERSION? Might also be nice to add this and update the comment to also include the patch version? And also useJPEGXL_COMPUTE_NUMERIC_VERSIONin the comment instead?#define JPEGXL_COMPUTE_NUMERIC_VERSION(major,minor,patch) ((major<<24) | (minor<<16) | (patch<<8) | 0)
Makes sense, I had forgotten to update the comment.
Do we need to mention this in changelog? It is not a binary API, still an important change
It would be great to have this backported to the 0.7.x branch as well, if there is a plan to release 0.7 that is. Thanks.
