Use valid Semver versions for pre-releases (#5636)

* Fix ProjectVersion handling of pre-releases * Add workaround for old, non-standard version * Attempt to fix versioning * More consistent comments * Apply suggestions from code review - Set CompareType's underlying type to int and revert change to ProjectVersion::compare's parameters - Add "None" and "All" as names elements of CompareType enum - Preserve hyphens in prerelease identifiers - Pad invalid (too short) versions to prevent crashes or nasty behavior - Compare numeric identifiers to non-numeric ones correctly - Don't interpret identifiers of form "-#" as numeric (where '#' is any number of digits) - Add tests to ensure fixes in this commit work and won't regress in the future * CMAKE fixes from code review Co-authored-by: Tres Finocchiaro <tres.finocchiaro@gmail.com> * Remove unnecessary changes to CMake logic * More const, more reference * Apply suggestions from code review Co-authored-by: Tres Finocchiaro <tres.finocchiaro@gmail.com>
LMMS · Sep 17, 2020 · af32800 · tresf · Oct 20, 2020 · af32800
1 parent f211c19
commit af32800
Show file tree

Hide file tree

Showing 6 changed files with 153 additions and 116 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -44,7 +44,7 @@ IF(VERSION_STAGE)
 	SET(VERSION     "${VERSION}-${VERSION_STAGE}")
 ENDIF()
 IF(VERSION_BUILD)
-	SET(VERSION     "${VERSION}.${VERSION_BUILD}")
+	SET(VERSION     "${VERSION}-${VERSION_BUILD}")
 ENDIF()
 
 # Override version information for non-base builds

diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt
@@ -9,7 +9,7 @@ IF(VERSION_STAGE)
 	SET(CPACK_PACKAGE_VERSION_PATCH         "${CPACK_PACKAGE_VERSION_PATCH}-${VERSION_STAGE}")
 ENDIF()
 IF(VERSION_BUILD)
-	SET(CPACK_PACKAGE_VERSION_PATCH         "${CPACK_PACKAGE_VERSION_PATCH}.${VERSION_BUILD}")
+	SET(CPACK_PACKAGE_VERSION_PATCH         "${CPACK_PACKAGE_VERSION_PATCH}-${VERSION_BUILD}")
 ENDIF()
 SET(CPACK_PACKAGE_INSTALL_DIRECTORY             "${PROJECT_NAME_UCASE}")
 SET(CPACK_SOURCE_GENERATOR                      "TBZ2")

diff --git a/cmake/modules/VersionInfo.cmake b/cmake/modules/VersionInfo.cmake
@@ -1,29 +1,56 @@
 FIND_PACKAGE(Git)
 IF(GIT_FOUND AND NOT FORCE_VERSION)
-	# Look for git tag information (e.g. Tagged: "v1.0.0", Non-tagged: "v1.0.0-123-a1b2c3d")
+	SET(MAJOR_VERSION 0)
+	SET(MINOR_VERSION 0)
+	SET(PATCH_VERSION 0)
+	# Look for git tag information (e.g. Tagged: "v1.0.0", Untagged: "v1.0.0-123-a1b2c3d")
+	# Untagged format: [latest tag]-[number of commits]-[latest commit hash]
 	EXECUTE_PROCESS(
 		COMMAND "${GIT_EXECUTABLE}" describe --tags --match v[0-9].[0-9].[0-9]*
 		OUTPUT_VARIABLE GIT_TAG
 		WORKING_DIRECTORY "${CMAKE_SOURCE_DIR}"
 		TIMEOUT 10
 		OUTPUT_STRIP_TRAILING_WHITESPACE)
+	# Read: TAG_LIST = GIT_TAG.split("-")
 	STRING(REPLACE "-" ";" TAG_LIST "${GIT_TAG}")
+	# Read: TAG_LIST_LENGTH = TAG_LIST.length()
 	LIST(LENGTH TAG_LIST TAG_LIST_LENGTH)
+	# Untagged versions contain at least 2 dashes, giving 3 strings on split.
+	# Hence, for untagged versions TAG_LIST_LENGTH = [dashes in latest tag] + 3.
+	# Corollary: if TAG_LIST_LENGTH <= 2, the version must be tagged.
 	IF(TAG_LIST_LENGTH GREATER 0)
+		# Set FORCE_VERSION to TAG_LIST[0], strip any 'v's to get MAJ.MIN.PAT
 		LIST(GET TAG_LIST 0 FORCE_VERSION)
 		STRING(REPLACE "v" "" FORCE_VERSION "${FORCE_VERSION}")
+		# Split FORCE_VERSION on '.' and populate MAJOR/MINOR/PATCH_VERSION
+		STRING(REPLACE "." ";" MAJ_MIN_PAT "${FORCE_VERSION}")
+		LIST(GET MAJ_MIN_PAT 0 MAJOR_VERSION)
+		LIST(GET MAJ_MIN_PAT 1 MINOR_VERSION)
+		LIST(GET MAJ_MIN_PAT 2 PATCH_VERSION)
 	ENDIF()
+	# 1 dash total: Dash in latest tag, no additional commits => pre-release
 	IF(TAG_LIST_LENGTH EQUAL 2)
 		LIST(GET TAG_LIST 1 VERSION_STAGE)
 		SET(FORCE_VERSION "${FORCE_VERSION}-${VERSION_STAGE}")
+	# 2 dashes: Assume untagged with no dashes in latest tag name => stable + commits
 	ELSEIF(TAG_LIST_LENGTH EQUAL 3)
+		# Get the number of commits and latest commit hash
 		LIST(GET TAG_LIST 1 EXTRA_COMMITS)
-		SET(FORCE_VERSION "${FORCE_VERSION}.${EXTRA_COMMITS}")
+		LIST(GET TAG_LIST 2 COMMIT_HASH)
+		# Bump the patch version
+		MATH(EXPR PATCH_VERSION "${PATCH_VERSION}+1")
+		# Set the version to MAJOR.MINOR.PATCH-EXTRA_COMMITS+COMMIT_HASH
+		SET(FORCE_VERSION "${MAJOR_VERSION}.${MINOR_VERSION}.${PATCH_VERSION}")
+		SET(FORCE_VERSION "${FORCE_VERSION}-${EXTRA_COMMITS}+${COMMIT_HASH}")
+	# 3 dashes: Assume untagged with 1 dash in latest tag name => pre-release + commits
 	ELSEIF(TAG_LIST_LENGTH EQUAL 4)
+		# Get pre-release stage, number of commits, and latest commit hash
 		LIST(GET TAG_LIST 1 VERSION_STAGE)
 		LIST(GET TAG_LIST 2 EXTRA_COMMITS)
-		SET(FORCE_VERSION
-			"${FORCE_VERSION}-${VERSION_STAGE}.${EXTRA_COMMITS}")
+		LIST(GET TAG_LIST 3 COMMIT_HASH)
+		# Set the version to MAJOR.MINOR.PATCH-VERSION_STAGE.EXTRA_COMMITS+COMMIT_HASH
+		SET(FORCE_VERSION "${FORCE_VERSION}-${VERSION_STAGE}")
+		SET(FORCE_VERSION "${FORCE_VERSION}.${EXTRA_COMMITS}+${COMMIT_HASH}")
 	ENDIF()
 ENDIF()
 
@@ -74,4 +101,3 @@ MESSAGE("\n"
 	"*   Override version:           -DFORCE_VERSION=x.x.x-x\n"
 	"*   Ignore Git information:     -DFORCE_VERSION=internal\n"
 )
-
diff --git a/include/ProjectVersion.h b/include/ProjectVersion.h
@@ -28,6 +28,9 @@
 #define PROJECT_VERSION_H
 
 #include <QtCore/QString>
+#include <QtCore/QStringList>
+
+#include <limits>
 
 /*! \brief Version number parsing and comparison
  *
@@ -36,16 +39,16 @@
 class ProjectVersion
 {
 public:
-	enum CompareType { Major, Minor, Release, Stage, Build };
+	enum CompareType : int { None = 0, Major=1, Minor=2, Release=3, Stage=4, Build=5, All = std::numeric_limits<int>::max() };
+
 
-	ProjectVersion(QString version, CompareType c = Build);
-	ProjectVersion(const char * version, CompareType c = Build);
+	ProjectVersion(QString version, CompareType c = All);
+	ProjectVersion(const char * version, CompareType c = All);
 
 	int getMajor() const { return m_major; }
 	int getMinor() const { return m_minor; }
-	int getRelease() const { return m_release; }
-	QString getStage() const { return m_stage; }
-	int getBuild() const { return m_build; }
+	int getPatch() const { return m_patch; }
+	const QStringList& getLabels() const { return m_labels; }
 	CompareType getCompareType() const { return m_compareType; }
 	ProjectVersion setCompareType(CompareType compareType) { m_compareType = compareType; return * this; }
 
@@ -54,11 +57,10 @@ class ProjectVersion
 
 private:
 	QString m_version;
-	int m_major;
-	int m_minor;
-	int m_release;
-	QString m_stage;
-	int m_build;
+	int m_major = 0;
+	int m_minor = 0;
+	int m_patch = 0;
+	QStringList m_labels;
 	CompareType m_compareType;
 } ;
 

diff --git a/src/core/ProjectVersion.cpp b/src/core/ProjectVersion.cpp
@@ -27,123 +27,105 @@
 
 #include "ProjectVersion.h"
 
-int parseMajor(QString & version) {
-	return version.section( '.', 0, 0 ).toInt();
-}
-
-
-
-
-int parseMinor(QString & version) {
-	return version.section( '.', 1, 1 ).toInt();
-}
-
-
-
-
-int parseRelease(QString & version) {
-	return version.section( '.', 2, 2 ).section( '-', 0, 0 ).toInt();
-}
-
-
-
-
-QString parseStage(QString & version) {
-	return version.section( '.', 2, 2 ).section( '-', 1 );
-}
-
-
-
-
-int parseBuild(QString & version) {
-	return version.section( '.', 3 ).toInt();
-}
-
 
 
 
 ProjectVersion::ProjectVersion(QString version, CompareType c) :
 	m_version(version),
-	m_major(parseMajor(m_version)),
-	m_minor(parseMinor(m_version)),
-	m_release(parseRelease(m_version)),
-	m_stage(parseStage(m_version)),
-	m_build(parseBuild(m_version)),
 	m_compareType(c)
 {
+	// Version numbers may have build data, prefixed with a '+',
+	// but this mustn't affect version precedence in comparisons
+	QString metadataStripped = version.split("+").first();
+	// They must have an obligatory initial segement, and may have
+	// optional identifiers prefaced by a '-'. Both parts affect precedence
+	QString obligatorySegment = metadataStripped.section('-', 0, 0);
+	QString prereleaseSegment = metadataStripped.section('-', 1);
+
+	// The obligatory segment consists of three identifiers: MAJOR.MINOR.PATCH
+	QStringList mainVersion = obligatorySegment.split(".");
+	// HACK: Pad invalid versions in order to prevent crashes
+	while (mainVersion.size() < 3){ mainVersion.append("0"); }
+	m_major = mainVersion.at(0).toInt();
+	m_minor = mainVersion.at(1).toInt();
+	m_patch = mainVersion.at(2).toInt();
+
+	// Any # of optional pre-release identifiers may follow, separated by '.'s
+	if (!prereleaseSegment.isEmpty()){ m_labels = prereleaseSegment.split("."); }
+
+	// HACK: Handle old (1.2.2 and earlier), non-standard versions of the form
+	// MAJOR.MINOR.PATCH.COMMITS, used for non-release builds from source.
+	if (mainVersion.size() >= 4 && m_major <= 1 && m_minor <= 2 && m_patch <= 2){
+		// Drop the standard version identifiers. erase(a, b) removes [a,b)
+		mainVersion.erase(mainVersion.begin(), mainVersion.begin() + 3);
+		// Prepend the remaining identifiers as prerelease versions
+		m_labels = mainVersion + m_labels;
+		// Bump the patch version. x.y.z-a < x.y.z, but we want x.y.z.a > x.y.z
+		m_patch += 1;
+	}
 }
 
 
 
 
-ProjectVersion::ProjectVersion(const char* version, CompareType c) :
-	m_version(QString(version)),
-	m_major(parseMajor(m_version)),
-	m_minor(parseMinor(m_version)),
-	m_release(parseRelease(m_version)),
-	m_stage(parseStage(m_version)),
-	m_build(parseBuild(m_version)),
-	m_compareType(c)
+ProjectVersion::ProjectVersion(const char* version, CompareType c) : ProjectVersion(QString(version), c)
 {
 }
 
 
 
 
+//! @param c Determines the number of identifiers to check when comparing
 int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, CompareType c)
 {
-	if(a.getMajor() != b.getMajor())
-	{
-		return a.getMajor() - b.getMajor();
-	}
-	if(c == Major)
-	{
-		return 0;
-	}
-
-	if(a.getMinor() != b.getMinor())
-	{
-		return a.getMinor() - b.getMinor();
-	}
-	if(c == Minor)
-	{
-		return 0;
+	// How many identifiers to compare before we consider the versions equal
+	const int limit = static_cast<int>(c);
+
+	// Use the value of limit to zero out identifiers we don't care about
+	int aMaj = 0, bMaj = 0, aMin = 0, bMin = 0, aPat = 0, bPat = 0;
+	if (limit >= 1){ aMaj = a.getMajor(); bMaj = b.getMajor(); }
+	if (limit >= 2){ aMin = a.getMinor(); bMin = b.getMinor(); }
+	if (limit >= 3){ aPat = a.getPatch(); bPat = b.getPatch(); }
+
+	// Then we can compare as if we care about every identifier
+	if(aMaj != bMaj){ return aMaj - bMaj; }
+	if(aMin != bMin){ return aMin - bMin; }
+	if(aPat != bPat){ return aPat - bPat; }
+
+	// Decide how many optional identifiers we care about
+	const int maxLabels = qMax(0, limit - 3);
+	const auto aLabels = a.getLabels().mid(0, maxLabels);
+	const auto bLabels = b.getLabels().mid(0, maxLabels);
+
+	// We can only compare identifiers if both versions have them
+	const int commonLabels = qMin(aLabels.size(), bLabels.size());
+	// If one version has optional labels and the other doesn't,
+	// the one without them is bigger
+	if (commonLabels == 0){ return bLabels.size() - aLabels.size(); }
+
+	// Otherwise, compare as many labels as we can
+	for (int i = 0; i < commonLabels; i++){
+		const QString& labelA = aLabels.at(i);
+		const QString& labelB = bLabels.at(i);
+		// If both labels are the same, skip
+		if (labelA == labelB){ continue; }
+		// Numeric and non-numeric identifiers compare differently
+		bool aIsNumeric = false, bIsNumeric = false;
+		const int numA = labelA.toInt(&aIsNumeric);
+		const int numB = labelB.toInt(&bIsNumeric);
+		// toInt reads '-x' as a negative number, semver says it's non-numeric
+		aIsNumeric &= !labelA.startsWith("-");
+		bIsNumeric &= !labelB.startsWith("-");
+		// If only one identifier is numeric, that one is smaller
+		if (aIsNumeric != bIsNumeric){ return aIsNumeric ? -1 : 1; }
+		// If both are numeric, compare as numbers
+		if (aIsNumeric && bIsNumeric){ return numA - numB; }
+		// Otherwise, compare lexically
+		return labelA.compare(labelB);
 	}
 
-	if(a.getRelease() != b.getRelease())
-	{
-		return a.getRelease() - b.getRelease();
-	}
-	if(c == Release)
-	{
-		return 0;
-	}
-
-	if(!(a.getStage().isEmpty() && b.getStage().isEmpty()))
-	{
-		// make sure 0.x.y > 0.x.y-alpha
-		if(a.getStage().isEmpty())
-		{
-			return 1;
-		}
-		if(b.getStage().isEmpty())
-		{
-			return -1;
-		}
-
-		// 0.x.y-beta > 0.x.y-alpha
-		int cmp = QString::compare(a.getStage(), b.getStage());
-		if(cmp)
-		{
-			return cmp;
-		}
-	}
-	if(c == Stage)
-	{
-		return 0;
-	}
-
-	return a.getBuild() - b.getBuild();
+	// If everything else matches, the version with more labels is bigger
+	return aLabels.size() - bLabels.size();
 }
 
 
@@ -153,6 +135,3 @@ int ProjectVersion::compare(ProjectVersion v1, ProjectVersion v2)
 {
 	return compare(v1, v2, std::min(v1.getCompareType(), v2.getCompareType()));
 }
-
-
-
diff --git a/tests/src/core/ProjectVersionTest.cpp b/tests/src/core/ProjectVersionTest.cpp
@@ -39,9 +39,39 @@ private slots:
 		QVERIFY(ProjectVersion("1.1.0", ProjectVersion::Minor) == "1.1.5");
 		QVERIFY( ! ( ProjectVersion("3.1.0", ProjectVersion::Minor) < "2.2.5" ) );
 		QVERIFY( ! ( ProjectVersion("2.5.0", ProjectVersion::Release) < "2.2.5" ) );
+		//A pre-release version has lower precedence than a normal version
 		QVERIFY(ProjectVersion("1.1.0") > "1.1.0-alpha");
+		//But higher precedence than the previous version
+		QVERIFY(ProjectVersion("1.1.0-alpha") > "1.0.0");
+		//Identifiers with letters or hyphens are compare lexically in ASCII sort order
 		QVERIFY(ProjectVersion("1.1.0-alpha") < "1.1.0-beta");
 		QVERIFY(ProjectVersion("1.2.0-rc1") < "1.2.0-rc2");
+		//Build metadata MUST be ignored when determining version precedence
+		QVERIFY(ProjectVersion("1.2.2") == "1.2.2+metadata");
+		QVERIFY(ProjectVersion("1.0.0-alpha") < "1.0.0-alpha.1");
+		QVERIFY(ProjectVersion("1.0.0-alpha.1") < "1.0.0-alpha.beta");
+		QVERIFY(ProjectVersion("1.0.0-alpha.beta") < "1.0.0-beta");
+		QVERIFY(ProjectVersion("1.0.0-beta.2") < "1.0.0-beta.11");
+		//Test workaround for old, nonstandard version numbers
+		QVERIFY(ProjectVersion("1.2.2.42") == "1.2.3-42");
+		QVERIFY(ProjectVersion("1.2.2.42") > "1.2.2.21");
+		//Ensure that newer versions of the same format aren't upgraded
+		//in order to discourage use of incorrect versioning
+		QVERIFY(ProjectVersion("1.2.3.42") == "1.2.3");
+		//CompareVersion "All" should compare every identifier
+		QVERIFY(
+			ProjectVersion("1.0.0-a.b.c.d.e.f.g.h.i.j.k.l", ProjectVersion::All)
+			< "1.0.0-a.b.c.d.e.f.g.h.i.j.k.m"
+		);
+		//Prerelease identifiers may contain hyphens
+		QVERIFY(ProjectVersion("1.0.0-Alpha-1.2") > "1.0.0-Alpha-1.1");
+		//We shouldn't crash on invalid versions
+		QVERIFY(ProjectVersion("1-invalid") == "1.0.0-invalid");
+		QVERIFY(ProjectVersion("") == "0.0.0");
+		//Numeric identifiers are smaller than non-numeric identiiers
+		QVERIFY(ProjectVersion("1.0.0-alpha") > "1.0.0-1");
+		//An identifier of the form "-x" is non-numeric, not negative
+		QVERIFY(ProjectVersion("1.0.0-alpha.-1") > "1.0.0-alpha.1");
 	}
 } ProjectVersionTests;