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

Update configure.ac with some m4 magic to add a git commit id... #1456

Merged

Conversation

fasterit
Copy link
Member

... and a reference of the last tag, the changes since then and the
dirty flag from git describe

@BenBE BenBE added enhancement Extension or improvement to existing feature build system 🔧 Affects the build system rather then the user experience labels Apr 16, 2024
@BenBE BenBE added this to the 3.4.0 milestone Apr 16, 2024
@Explorer09
Copy link
Contributor

  1. What's the main use case of this PR?
  2. The git commit ID in the version string seems to be produced during the bootstrap (./autogen.sh) phase. Am I correct? And why in this phase and not during "configure"?

@fasterit
Copy link
Member Author

  1. Getting a better feedback in cases like Sudden Segementation Fault #1455
  2. Yes. Because this is how our version setup is. Changing that would be more invasive with less added benefit.
    /DLange

 and a reference of the last tag, the changes since then and the
 dirty flag from `git describe`
@fasterit fasterit force-pushed the add-git-commit-to-compiled-dev-versions branch from 36b58d8 to 2acd62d Compare April 16, 2024 17:07
@Explorer09
Copy link
Contributor

@fasterit Some more questions:

  1. I often build htop while developing it and I make local commits. Since the commit ID is taken during ./autogen.sh, it could differ from the commit I made before running ./configure && make. How to address this problem?
  2. Is there a way to manually override or disable the commit ID during build?

My wish is to have a solution that works in a longer term. And that hacking with M4/Autoconf doesn't look like a long term solution to me.

@fasterit
Copy link
Member Author

  1. run ./autogen.sh again if you care enough
  2. if there is no .git directory present (normal tarball builds) it will fall back to the version explicitly given as htop_release_version

Making this change commit ids on every build would need to target make and not the autotools. It's possible but imho no worth it. This is to improve our user feedback, not for core devs that should know what they are doing™.

@Explorer09
Copy link
Contributor

@fasterit
Chances are, the use case we are addressing would not be unique to us. There would be other projects that would like to embed a git commit ID during their build.

And I found some Stack Overflow posts like this and this.

And it look like it's possible for git to put the commit ID into a file during git archive. If htop can utilize that, we can even let people who build from a downloaded git archive to include the commit ID in the htop built.

@fasterit
Copy link
Member Author

fasterit commented Apr 16, 2024

There are many helper scripts floating around. There's the $id$ option. As I said above, there are many more invasive options. This is the relatively easy improvement on the current situation and good enough for me. We don't use git archive anywhere and I see no need to change that either.

@Explorer09
Copy link
Contributor

@fasterit

We don't use git archive anywhere and I see no need to change that either.

GitHub has this download source code archive feature for the web interface whether we use it or not. The user could download from there and if it's me, I would add support of this use case.

This is the relatively easy improvement on the current situation and good enough for me.

While I am personally not convinced, I did look up the use of M4 macros in AC_INIT and that Autoconf itself does use macros in AC_INIT similar to the style like yours. However, it use this git-version-gen script. The script has some features comparing to your ad-hoc approach, AFAICT:

  • It generates version with the number of commits as well as commit ID. The number of commits is a good idea when it makes the version strings sortable.
  • It reads from the ".tarball-version" file if present. That file is used for marking the release version and it can also (unintentionally) be used as an override.
  • It doesn't seem to support git archive use case though.

@BenBE
Copy link
Member

BenBE commented Apr 16, 2024

Small note: @fasterit Would be nice, if we could report this commit id also as part of the summary report at the bottom of the configure output.

@Explorer09
Copy link
Contributor

Small note: @fasterit Would be nice, if we could report this commit id also as part of the summary report at the bottom of the configure output.

It's already part of the version string at the summary report. Example build log has it: htop 3.4.0-dev-ddd9de7

@Explorer09
Copy link
Contributor

An issue with the current solution by @fasterit:
It won't be seen clearly in the GitHub Actions' build log, but it happened in my local checkout. git describe --abbrev=7 --dirty --always --tags output can differ on whether you clone the tags from the source repo or not.

  • The GitHub Actions CI performs shallow clone by default, and thus no tags are copied. The aforementioned git describe output would be "2acd62d", i.e. just the commit ID. (I use 2acd62d as an example.)
  • On a full clone, including tags, the git describe output becomes "3.3.0-95-g2acd62d". If combined with the m4_join codes of this, the joined version string becomes "3.4.0-dev-3.3.0-95-g2acd62d", which is, IMO, longer than desirable.

Perhaps there is another issue worth mentioning, in the maintenance perspective:

  • git describe outputs versions in the form of "<latest released version>-<num of commits>-g<commit ID>", but htop uses "<next version to release>-dev" for our development version. This scheme would need to adapted too.
  • Perhaps we can use 3.4.0-dev if no git repo is found in the worktree, and 3.3.0-95-g2acd62d if these is a repo to generate version string from. But we have to keep the shallow clone in mind, thus the tags might not always exist in the repo clone.

@BenBE
Copy link
Member

BenBE commented Apr 17, 2024

Both are rather cosmetic. The important information in the first case is the commit id. If there's a git tag and tags available, we at least know about how much changed since the last release it was build upon.

Also, the second issue, is something I somehow find redundant, but to little to actually care … The team has to be able to understand those versions after all …

@Explorer09
Copy link
Contributor

Speaking of, I have experimented the git-version-gen script from Gnulib (used in Autoconf too), and I found it has some weaknesses preventing me from adopting for htop:

  • It has no way to override certain options of git describe, e.g. allowing a longer commit ID rather than 4 hex digits (--abbrev=4).
  • It ignores non-annotated tags by default (i.e. no --tags option in git describe). Our 3.3.0 is un-annotated, perhaps by accident.
  • It has a filter of tags by default – only tags prefixed with "v" are considered. This is not a big deal but we have to use --prefix '' when invoking git-version-gen.
  • As commented before, it has no compatibility with git archive.

@Explorer09
Copy link
Contributor

@BenBE

If there's a git tag and tags available, we at least know about how much changed since the last release it was build upon.

I would guess they are to address the sorting issue. When a package version is generated that way, it affects the name of make dist tarballs. Having a "number of commits" field before a commit ID allows development snapshot tarballs to sort properly.

@fasterit fasterit merged commit 6f0adfa into htop-dev:main Apr 17, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system 🔧 Affects the build system rather then the user experience enhancement Extension or improvement to existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants