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

bzip3 built with cmake reports version "1.3.0 #126

Open
dearblue opened this issue Jan 11, 2024 · 9 comments
Open

bzip3 built with cmake reports version "1.3.0 #126

dearblue opened this issue Jan 11, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@dearblue
Copy link
Contributor

Binaries downloaded and extracted from a source code release package and built with cmake + make have the incorrect version.

OS: FreeBSD 13.2 amd64

% fetch -o- https://github.com/kspalaiologos/bzip3/releases/download/1.4.0/bzip3-1.4.0.tar.xz | tar xf -
% cd bzip3-1.4.0
% cmake .
% make

% ./bzip3 --version
bzip3 1.3.0
Copyright (C) by Kamila Szewczyk, 2022-2023.
License: GNU Lesser GPL version 3 <https://www.gnu.org/licenses/lgpl-3.0.en.html>

bz3_version() in libbzip3.so returns "1.3.0" as well.

% irb33 -rfiddle
irb(main):001> h = Fiddle::Handle.new("./libbzip3.so")
=> #<Fiddle::Handle:0x0000000869d8cf68>
irb(main):002> bz3_version_func = Fiddle::Function.new(h.sym("bz3_version"), [], Fiddle::TYPE_VOIDP, name: "bz3_version")
=> #<Fiddle::Function:0x000000086b0c7920 @abi=2, @argument_types=[], @closure=36336442016, @is_variadic=false, @name="bz3_version", @need_gvl=false, @ptr=36336442016, @return_type=1>
irb(main):003> version = bz3_version_func.call
irb(main):004> version.to_s
=> "1.3.0"

libtool (configure + make) correctly returns "1.4.0".

@kspalaiologos
Copy link
Owner

OK. It seems like the version in the CMake build instructions is hardcoded, but I am not entirely sure how to make it update with the tag. @sorairolake @alerque - ideas?

@dearblue
Copy link
Contributor Author

CMake seems to be able to take in different files.
So, if we generate a file that defines a version in bootstrap.sh, it may be OK.

However, I have never written a CMake file, so I don't know if this is the right way to do it.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cb24d6f..9f8e871 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -2,11 +2,13 @@ cmake_minimum_required(VERSION 3.13 FATAL_ERROR)

 project(
   bzip3
-  VERSION 1.3.0
   DESCRIPTION "A better and stronger spiritual successor to BZip2"
   HOMEPAGE_URL "https://github.com/kspalaiologos/bzip3"
   LANGUAGES C)

+# need run bootstrap.sh
+include(${CMAKE_CURRENT_LIST_DIR}/.version.cmake)
+
 set(CMAKE_C_STANDARD 99)

 option(BUILD_SHARED_LIBS "Build libbz3 as a shared library" ON)
diff --git a/Makefile.am b/Makefile.am
index da36828..a3a6586 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -55,6 +55,7 @@ _BRANCH_REF != $(AWK) '{print ".git/" $$2}' .git/HEAD 2>/dev/null ||:

 dist-hook:
        printf "$(VERSION)" > "$(distdir)/.tarball-version"
+       cp "$(srcdir)/.version.cmake" "$(distdir)/.version.cmake"

 # Begin developer convenience targets

diff --git a/bootstrap.sh b/bootstrap.sh
index 0d8d284..b363666 100755
--- a/bootstrap.sh
+++ b/bootstrap.sh
@@ -21,4 +21,6 @@ else
     ./build-aux/git-version-gen .tarball-version > .version
 fi

+echo "set(PROJECT_VERSION \"$(cat .version)\")" > .version.cmake
+
 autoreconf --install

@alerque
Copy link
Contributor

alerque commented Jan 11, 2024

The contribution of the CMakeLists.txt file in 5d21594 hard coded the version. This was naive from the beginning.

The diff @dearblue isn't quite right either. That would hack it together for builds using Git clone sources, but it would not work correctly when using source distributions (e.g. make dist). Instead of making a new CMake specific file I think you should use the resources that are there already. In particular you should figure out how to get CMake to execute the same command that Autoconf runs to get the current version: ./build-aux/git-version-gen .tarball-version. The output of that should be used. Also it should not be stored in another file and include()ed, it should be generated from that command each time CMake is run.

@sorairolake
Copy link
Contributor

I think this can be resolved by making the following changes when bumping the version (e.g., at 23659dc):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index cb24d6f..82780ed 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.13 FATAL_ERROR)
 
 project(
   bzip3
-  VERSION 1.3.0
+  VERSION 1.4.0
   DESCRIPTION "A better and stronger spiritual successor to BZip2"
   HOMEPAGE_URL "https://github.com/kspalaiologos/bzip3"
   LANGUAGES C)

However, this method may forget changes, so I think it is better to be able to detect it if possible.

@kspalaiologos
Copy link
Owner

Yes, we can hardcode the version, but it's not sustainable.

@alerque
Copy link
Contributor

alerque commented Jan 12, 2024

I think this can be resolved by making the following changes when bumping the version

@sorairolake Of course it can be hard coded for releases, but that is a pain and adds a manual step to the release cycle that it doesn't need. The GNU Autotools tooling is all setup to build with correct version information from any Git clone at any commit and/or with any source dist tarball. In between releases it will even show the development commit information. Releasing only requires adding a tag.

I'm quite sure CMake is capable of following suit here and playing along. The auxiliary script to determine the current version string from the several possible scenarios is readily available, all CMake has to do is be taught to execute it and use the output. Somebody that knows how CMake works should be able to accomplish that without hard coding the value again.

Running ./build-aux/git-version-gen .tarball-version should work with or without git and either in a Git clone or in a source tarball and output just a simple string with the current version corresponding to the sources. That's how you "detect" the version, somebody just needs to teach CMake to run that command to get the version instead of having a hard coded string or reading a file.

@kspalaiologos
Copy link
Owner

I will try to retrofit https://github.com/antoniovazquezblanco/cmake-gitversiondetect for this purpose later. That said, I have never used CMake before :-).

@alerque
Copy link
Contributor

alerque commented Jan 12, 2024

I'm pretty sure that module has two issues: one it is overkill and a complete re-implementation of something we already have, and also it won't handle working outside of a Git clone, meaning it won't work in the source dist tarball.

I'll look around in a bit...

@alerque
Copy link
Contributor

alerque commented Jan 12, 2024

Something like this few lines is probably what we need to adapt:

https://github.com/davidmoreno/onion/blob/master/CMakeLists.txt#L24-L32

One question I don't know the answer to is if we need to parse the output of our script for just the x.y.z triplet or if we can use the output wholesale with the extra dev version stuff appended. Either way it should be just a little regex work to get whatever form CMake needs from the output of our existing script which already handles the Git working dir vs. source tarball issues.

netgate-git-updates pushed a commit to pfsense/FreeBSD-ports that referenced this issue Jan 12, 2024
@kspalaiologos kspalaiologos added the bug Something isn't working label Jan 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants