Releases made by CI have funky version numbers

pjrobertson commented 2 years ago

To Reproduce

Push a new branch to the QS repo: git push origin v2.0.1
Wait for CI to create the build and make a release
Check CFBundleShortVersionString in the Info.plist. It's set to 2.0.1: (HEAD detatched at v2.0.1)

Expected behavior For official releases (made by tags) they should just be named 'version number'. E.g. 2.0.1, but other builds should include the repo status

Screenshots Screenshot 2022-04-04 at 12 15 55

What version of Quicksilver? All those built by GH Actions.

n8henrie commented 2 years ago

Plan to look at this today

n8henrie commented 2 years ago

This should trigger a new (test) release in a few minutes to make sure this works on CI, if so we can cut 2.0.2 soon.

https://github.com/quicksilver/Quicksilver/releases/tag/2.0.1-test-version-fix

n8henrie commented 2 years ago

Huh. That didn't trigger a release, and it has a trailing colon.

n8henrie commented 2 years ago

You can use this standalone script to see the logic and preview the NEW_VERSION_STRING that will end up in CFBundleShortVersionString.

I just ran a test on a new tag v2.0.3, and other than not having updated the version info (so the DMG had the wrong name as you can see), it seemed to work:

For all conditions other than a tagged Release build, it includes more contextual information.

#!/bin/sh
# Script to append development information to the QS version string

# Created by Henning Jungkurth on Apr 18 2012
# Modifications by Nathan Henrie on 20220405

set -euf

# Change these to see how they change the output
CONFIGURATION=Release
QS_INFO_VERSION=2.0.2

CURRENT_TAG=$(git describe --exact-match --tags 2> /dev/null || true)
CURRENT_BRANCH=$(git branch --show-current)
HEAD=$(git rev-parse --short HEAD)

# Default: `2.0.2:master-a83756f6:Debug`
NEW_VERSION_STRING=${QS_INFO_VERSION}:${CURRENT_BRANCH:-"deatched HEAD"}-${HEAD}:${CONFIGURATION}

# If on an tag, make prettier: `v2.0.2:Debug` or `v2.0.2`
if [ -n "${CURRENT_TAG}" ]; then
  if [ "${CONFIGURATION}" = "Release" ]; then
    NEW_VERSION_STRING=${CURRENT_TAG}
  else
    NEW_VERSION_STRING=${CURRENT_TAG}:${CONFIGURATION}
  fi
fi

echo "${NEW_VERSION_STRING}"
exit 0

if [ -n "${NEW_VERSION_STRING}" ]; then
  defaults write $QS_APP_CONTENTS_FOLDER/Info CFBundleShortVersionString "\"${NEW_VERSION_STRING}\""
fi

I think the associated PR is ready to merge if this looks okay: https://github.com/quicksilver/Quicksilver/pull/2710

Once merged I think we should push v2.0.3 to fully resolve this issue.

MiqViq commented 2 years ago

        <key>CFBundleShortVersionString</key>
        <string>v2.0.2</string>

This version formatting is better now but still the best version number in CFBundleShortVersionString does not have anything but digits and dots...so I suggest that you use plain "2.0.2" style with public releases.

skurfer commented 2 years ago

I’m thinking the easiest way to fix this is to just stop putting ‘v’ at the beginning of the tags. It’s not like we’d look at a tag called “2.2.0” and not know what it was.

MiqViq commented 2 years ago

Yes, version numbers should start with digits only. I guess that something like 2.0.3b may be acceptable but it is best to use plain digits and dots. I found this out the hard way as that starnge CFBundleShortVersionString with 2.0.1 messed up my Munki Repo catalog rebuilds...took few hours to figure out what was causing the issue. Well, now I know better...

n8henrie commented 2 years ago

the best version number

By best, do you mean the one you prefer based on aesthetics? Or did you mean something else?

I considered making it something different, but thought that having the release match the tag was best, and we've been using a v prefix for tags (which I've found useful in other projects a few times, gives you a nice | grep '^v[0-9.]+$ for filtering various things).

I made CFBundleShortVersionString match the tag name, so options that come to mind are:

Leave it as is. I don't see a huge problem with a leading v, common in many projects, though I agree it might not fit everyone's preferences for aesthetics. It does help avoid issues with stringly-typed stuff getting interpreted as numbers and leading to sorting issues (i.e. numerous python 3.10 vs "3.10" vs 3.9 vs issues)
Going forward, remember to stop prefixing tags with v as has been done up until now
Strip all leading vs from the tag when creating CFBundleShortVersionString. Unlikely to be an issue, but if Quicksilver were to ever go to string-based releases like QS 17.9: victory obviously would need changing
s/[^0-9.]//g (or alternative regex) when creating CFBundleShortVersionString

Looks like @skurfer beat me to the comment.

n8henrie commented 2 years ago

Huh, I've never heard of munki. What issue do they have with e.g. v2.0.1? I wonder if they're incorrectly interpreting version numbers as floats.

MiqViq commented 2 years ago

A working compromise would be that if you make the CFBundleVersion with formatting like 2.0.3.4567 where the build number is the last component. There is an issue with Munki tools when importing items with strangely formatted version numbers into repo. In this case munkiimport defaults to CFBundleVersion as it seems to be more reliable for parsing. But CFBundleShortVersionString is visible to user in Finder and in QS app About pane and munkiimport will use that if it is valid. There may be other such tools for app deployment that do not like misbehaving version numbers os it is generally best to keep version numbers strictly semantic: https://semver.org/

n8henrie commented 2 years ago

While not semver, The v prefix is so common that it is specifically referenced: https://semver.org/#is-v123-a-semantic-version -- have you raised an issue with munki? It seems like it would be reasonable for them to support a v prefix.

@skurfer @pjrobertson do either of you have a strong opinion (or alternatives to) the options in https://github.com/quicksilver/Quicksilver/issues/2705#issuecomment-1091789527?

pjrobertson commented 2 years ago

I'd vote for either:

Strip all leading vs from the tag when creating CFBundleShortVersionString. Unlikely to be an issue, but if Quicksilver were to ever go to string-based releases like QS 17.9: victory obviously would need changing
Stop putting ‘v’ at the beginning of the tags. It’s not like we’d look at a tag called “2.2.0” and not know what it was.

One other issue with the current format is that when I uploaded to the updates server, it ads an additional v to the start of the version string, so the version came out as vv2.0.2.

Since our other tags are all prefixed with v then I'd probably lean towards No.1 above.

n8henrie commented 2 years ago

One other issue with the current format is that when I uploaded to the updates server, it ads an additional v to the start of the version string, so the version came out as vv2.0.2.

Doh!

Actually, I could also just use QS_INFO_VERSION instead of using the git tag directly -- should we just go with that?

pjrobertson commented 2 years ago

Actually, I could also just use QS_INFO_VERSION instead of using the git tag directly -- should we just go with that?

Yes 👍 keeps things consistent with the codebase :)

n8henrie commented 2 years ago

https://github.com/quicksilver/Quicksilver/pull/2719

Let's see if it worked... EDIT: https://github.com/quicksilver/Quicksilver/actions/runs/2112202922

quicksilver / Quicksilver

Releases made by CI have funky version numbers #2705