From f2990bf2c7079ea66c9058e0d94c35d3c4b8e0fd Mon Sep 17 00:00:00 2001 From: Spekular Date: Sun, 16 Aug 2020 21:55:47 +0200 Subject: [PATCH 1/9] Fix ProjectVersion handling of pre-releases --- include/ProjectVersion.h | 19 ++-- src/core/ProjectVersion.cpp | 154 +++++++++----------------- tests/src/core/ProjectVersionTest.cpp | 10 ++ 3 files changed, 74 insertions(+), 109 deletions(-) diff --git a/include/ProjectVersion.h b/include/ProjectVersion.h index 5f7bf09b3c1..4fb14498332 100644 --- a/include/ProjectVersion.h +++ b/include/ProjectVersion.h @@ -28,6 +28,7 @@ #define PROJECT_VERSION_H #include +#include /*! \brief Version number parsing and comparison * @@ -36,29 +37,27 @@ class ProjectVersion { public: - enum CompareType { Major, Minor, Release, Stage, Build }; + enum CompareType { Major=1, Minor=2, Release=3, Stage=4, Build=5 }; ProjectVersion(QString version, CompareType c = Build); ProjectVersion(const char * version, CompareType c = Build); 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; } + QStringList getLabels() const { return m_labels;} CompareType getCompareType() const { return m_compareType; } ProjectVersion setCompareType(CompareType compareType) { m_compareType = compareType; return * this; } - static int compare(const ProjectVersion& a, const ProjectVersion& b, CompareType c); + static int compare(const ProjectVersion& a, const ProjectVersion& b, int c); static int compare(ProjectVersion v1, ProjectVersion v2); 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 = QStringList(); CompareType m_compareType; } ; diff --git a/src/core/ProjectVersion.cpp b/src/core/ProjectVersion.cpp index ea97a6fec7d..49e48c946f1 100644 --- a/src/core/ProjectVersion.cpp +++ b/src/core/ProjectVersion.cpp @@ -27,123 +27,82 @@ #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 + QStringList mainAndLabels = metadataStripped.split("-"); + + // The obligatory segment consists of three identifiers: MAJOR.MINOR.PATCH + QStringList mainVersion = mainAndLabels.first().split("."); + 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 (mainAndLabels.size() >= 2){ m_labels = mainAndLabels.at(1).split("."); } } -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) { } -int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, CompareType c) +//! @param c The number of identifiers to check when comparing +int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, int 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; + // Use the value of c to zero out identifiers we don't care about + int aMaj = 0, bMaj = 0, aMin = 0, bMin = 0, aPat = 0, bPat = 0; + if (c >= 1){ aMaj = a.getMajor(); bMaj = b.getMajor(); } + if (c >= 2){ aMin = a.getMinor(); bMin = b.getMinor(); } + if (c >= 3){ aPat = a.getPatch(); bPat = b.getPatch(); } + + // Then compare as if we care about every identifiers + 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 + int numLabels = qMax(0, c - 3); + auto aLabels = a.getLabels().mid(0, numLabels); + auto bLabels = b.getLabels().mid(0, numLabels); + + // We can only compare identifiers if both versions have them + 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++){ + QString labelA = aLabels.at(i); + QString labelB = bLabels.at(i); + // If both labels are the same, skip + if (labelA == labelB){ continue; } + // Else if both labels are numeric, compare them numerically + bool aIsNumeric = false, bIsNumeric = false; + int numA = labelA.toInt(&aIsNumeric); + int numB = labelB.toInt(&bIsNumeric); + 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 +112,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 index 7c99727397f..6dbc74ee618 100644 --- a/tests/src/core/ProjectVersionTest.cpp +++ b/tests/src/core/ProjectVersionTest.cpp @@ -23,6 +23,7 @@ */ #include "QTestSuite.h" +#include #include "ProjectVersion.h" @@ -39,9 +40,18 @@ 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"); + //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"); + } } ProjectVersionTests; From 8b400ba3ae2c983ddc62aff31ae8805c6cd8ef2b Mon Sep 17 00:00:00 2001 From: Spekular Date: Fri, 21 Aug 2020 16:18:42 +0200 Subject: [PATCH 2/9] Add workaround for old, non-standard version --- src/core/ProjectVersion.cpp | 13 ++++++++++++- tests/src/core/ProjectVersionTest.cpp | 10 ++++++++-- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/src/core/ProjectVersion.cpp b/src/core/ProjectVersion.cpp index 49e48c946f1..99dee6f0622 100644 --- a/src/core/ProjectVersion.cpp +++ b/src/core/ProjectVersion.cpp @@ -49,6 +49,17 @@ ProjectVersion::ProjectVersion(QString version, CompareType c) : // Any # of optional pre-release identifiers may follow, separated by '.'s if (mainAndLabels.size() >= 2){ m_labels = mainAndLabels.at(1).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; + } } @@ -70,7 +81,7 @@ int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, if (c >= 2){ aMin = a.getMinor(); bMin = b.getMinor(); } if (c >= 3){ aPat = a.getPatch(); bPat = b.getPatch(); } - // Then compare as if we care about every identifiers + // Then 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; } diff --git a/tests/src/core/ProjectVersionTest.cpp b/tests/src/core/ProjectVersionTest.cpp index 6dbc74ee618..8375e2c7e70 100644 --- a/tests/src/core/ProjectVersionTest.cpp +++ b/tests/src/core/ProjectVersionTest.cpp @@ -23,7 +23,6 @@ */ #include "QTestSuite.h" -#include #include "ProjectVersion.h" @@ -42,6 +41,8 @@ private slots: 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"); @@ -51,7 +52,12 @@ private slots: 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"); } } ProjectVersionTests; From c860b78522229d4310fd35d9292e6291f4b0928a Mon Sep 17 00:00:00 2001 From: Spekular Date: Fri, 21 Aug 2020 19:54:48 +0200 Subject: [PATCH 3/9] Attempt to fix versioning --- CMakeLists.txt | 4 +++- cmake/CMakeLists.txt | 3 ++- cmake/modules/VersionInfo.cmake | 35 +++++++++++++++++++++++++++++---- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 4ead95c32db..59818474b8d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,7 +44,9 @@ IF(VERSION_STAGE) SET(VERSION "${VERSION}-${VERSION_STAGE}") ENDIF() IF(VERSION_BUILD) - SET(VERSION "${VERSION}.${VERSION_BUILD}") + MATH(EXPR VERSION_RELEASE_BUMP "${VERSION_RELEASE}+1") + SET(VERSION "${VERSION_MAJOR}.${VERSION_MINOR}.${VERSION_RELEASE_BUMP"}) + SET(VERSION "${VERSION}-${VERSION_BUILD}") ENDIF() # Override version information for non-base builds diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt index b27dec91e0f..78aa86e917c 100644 --- a/cmake/CMakeLists.txt +++ b/cmake/CMakeLists.txt @@ -9,7 +9,8 @@ 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}") + math(EXPR CPACK_PACKAGE_VERSION_PATH_BUMP "${CPACK_PACKAGE_VERSION_PATH}+1") + SET(CPACK_PACKAGE_VERSION_PATCH "${CPACK_PACKAGE_VERSION_PATCH_BUMP}-${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 index cf6932cbbba..ebc0287987f 100644 --- a/cmake/modules/VersionInfo.cmake +++ b/cmake/modules/VersionInfo.cmake @@ -1,29 +1,57 @@ FIND_PACKAGE(Git) IF(GIT_FOUND AND NOT FORCE_VERSION) + SET(MAJOR_VERSION 0) + SET(MINOR_VERSION 0) + SET(PATCH_VERSION 0) # Look for git tag information (e.g. Tagged: "v1.0.0", Non-tagged: "v1.0.0-123-a1b2c3d") + # Non tagged 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) + # Non-tagged versions contain at least 2 dashes, and 2 dashes gives 3 strings + # so we know that TAG_LIST_LENGTH = [dashes in latest tag] + 3. IF(TAG_LIST_LENGTH GREATER 0) + # Set FORCE_VERSION to TAG_LIST[0], strip the initial "v" 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 => prerelease with no additional commits IF(TAG_LIST_LENGTH EQUAL 2) LIST(GET TAG_LIST 1 VERSION_STAGE) SET(FORCE_VERSION "${FORCE_VERSION}-${VERSION_STAGE}") + # 0 dashes in latest tag => stable release with additional commits + # For example, 1.2.2 + 50 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}") + # 1 dash in latest tag => pre-release with additional commits + # For example, 1.3.0-alpha.1 with an additional 50 commits ELSEIF(TAG_LIST_LENGTH EQUAL 4) + # Get prerelease 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 +102,3 @@ MESSAGE("\n" "* Override version: -DFORCE_VERSION=x.x.x-x\n" "* Ignore Git information: -DFORCE_VERSION=internal\n" ) - From a4a6b273a17d42dad9f1f940f44e62f12cac242b Mon Sep 17 00:00:00 2001 From: Spekular Date: Wed, 26 Aug 2020 21:58:09 +0200 Subject: [PATCH 4/9] More consistent comments --- cmake/modules/VersionInfo.cmake | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/cmake/modules/VersionInfo.cmake b/cmake/modules/VersionInfo.cmake index ebc0287987f..9571514a6eb 100644 --- a/cmake/modules/VersionInfo.cmake +++ b/cmake/modules/VersionInfo.cmake @@ -3,8 +3,8 @@ IF(GIT_FOUND AND NOT FORCE_VERSION) SET(MAJOR_VERSION 0) SET(MINOR_VERSION 0) SET(PATCH_VERSION 0) - # Look for git tag information (e.g. Tagged: "v1.0.0", Non-tagged: "v1.0.0-123-a1b2c3d") - # Non tagged format: [latest tag]-[number of commits]-[latest commit hash] + # 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 @@ -15,24 +15,24 @@ IF(GIT_FOUND AND NOT FORCE_VERSION) STRING(REPLACE "-" ";" TAG_LIST "${GIT_TAG}") # Read: TAG_LIST_LENGTH = TAG_LIST.length() LIST(LENGTH TAG_LIST TAG_LIST_LENGTH) - # Non-tagged versions contain at least 2 dashes, and 2 dashes gives 3 strings - # so we know that TAG_LIST_LENGTH = [dashes in latest tag] + 3. + # 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 the initial "v" to get MAJ.MIN.PAT + # 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 + # 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 => prerelease with no additional commits + # 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}") - # 0 dashes in latest tag => stable release with additional commits - # For example, 1.2.2 + 50 commits + # 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) @@ -42,10 +42,9 @@ IF(GIT_FOUND AND NOT FORCE_VERSION) # 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}") - # 1 dash in latest tag => pre-release with additional commits - # For example, 1.3.0-alpha.1 with an additional 50 commits + # 3 dashes: Assume untagged with 1 dash in latest tag name => pre-release + commits ELSEIF(TAG_LIST_LENGTH EQUAL 4) - # Get prerelease stage, number of commits, and latest commit hash + # 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) LIST(GET TAG_LIST 3 COMMIT_HASH) From 6a73c281648c31d171bd9f7fc56f53bf85e84917 Mon Sep 17 00:00:00 2001 From: Spekular Date: Sun, 6 Sep 2020 15:48:47 +0200 Subject: [PATCH 5/9] 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 --- include/ProjectVersion.h | 11 +++++--- src/core/ProjectVersion.cpp | 40 +++++++++++++++++---------- tests/src/core/ProjectVersionTest.cpp | 14 ++++++++++ 3 files changed, 47 insertions(+), 18 deletions(-) diff --git a/include/ProjectVersion.h b/include/ProjectVersion.h index 4fb14498332..8573f2f45d2 100644 --- a/include/ProjectVersion.h +++ b/include/ProjectVersion.h @@ -30,6 +30,8 @@ #include #include +#include + /*! \brief Version number parsing and comparison * * Parses and compares version information. i.e. "1.0.3" < "1.0.10" @@ -37,10 +39,11 @@ class ProjectVersion { public: - enum CompareType { Major=1, Minor=2, Release=3, Stage=4, Build=5 }; + enum CompareType : int { None = 0, Major=1, Minor=2, Release=3, Stage=4, Build=5, All = std::numeric_limits::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; } @@ -49,7 +52,7 @@ class ProjectVersion CompareType getCompareType() const { return m_compareType; } ProjectVersion setCompareType(CompareType compareType) { m_compareType = compareType; return * this; } - static int compare(const ProjectVersion& a, const ProjectVersion& b, int c); + static int compare(const ProjectVersion& a, const ProjectVersion& b, CompareType c); static int compare(ProjectVersion v1, ProjectVersion v2); private: diff --git a/src/core/ProjectVersion.cpp b/src/core/ProjectVersion.cpp index 99dee6f0622..a863143e0f6 100644 --- a/src/core/ProjectVersion.cpp +++ b/src/core/ProjectVersion.cpp @@ -39,16 +39,19 @@ ProjectVersion::ProjectVersion(QString version, CompareType c) : QString metadataStripped = version.split("+").first(); // They must have an obligatory initial segement, and may have // optional identifiers prefaced by a '-'. Both parts affect precedence - QStringList mainAndLabels = metadataStripped.split("-"); + QString obligatorySegment = metadataStripped.section('-', 0, 0); + QString prereleaseSegment = metadataStripped.section('-', 1); // The obligatory segment consists of three identifiers: MAJOR.MINOR.PATCH - QStringList mainVersion = mainAndLabels.first().split("."); + 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 (mainAndLabels.size() >= 2){ m_labels = mainAndLabels.at(1).split("."); } + 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. @@ -72,22 +75,25 @@ ProjectVersion::ProjectVersion(const char* version, CompareType c) : ProjectVers -//! @param c The number of identifiers to check when comparing -int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, int c) +//! @param c Determines the number of identifiers to check when comparing +int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, CompareType c) { - // Use the value of c to zero out identifiers we don't care about + // How many identifiers to compare before we consider the versions equal + const int limit = static_cast(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 (c >= 1){ aMaj = a.getMajor(); bMaj = b.getMajor(); } - if (c >= 2){ aMin = a.getMinor(); bMin = b.getMinor(); } - if (c >= 3){ aPat = a.getPatch(); bPat = b.getPatch(); } + 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 compare as if we care about every identifier + // 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 - int numLabels = qMax(0, c - 3); + int numLabels = qMax(0, limit - 3); auto aLabels = a.getLabels().mid(0, numLabels); auto bLabels = b.getLabels().mid(0, numLabels); @@ -103,10 +109,16 @@ int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, QString labelB = bLabels.at(i); // If both labels are the same, skip if (labelA == labelB){ continue; } - // Else if both labels are numeric, compare them numerically + // Numeric and non-numeric identifiers compare differently bool aIsNumeric = false, bIsNumeric = false; - int numA = labelA.toInt(&aIsNumeric); - int numB = labelB.toInt(&bIsNumeric); + 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); diff --git a/tests/src/core/ProjectVersionTest.cpp b/tests/src/core/ProjectVersionTest.cpp index 8375e2c7e70..e52088f6f8c 100644 --- a/tests/src/core/ProjectVersionTest.cpp +++ b/tests/src/core/ProjectVersionTest.cpp @@ -58,6 +58,20 @@ private slots: //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; From 30a03465c2f909e2a718d8830e40dbf9ce28e1fe Mon Sep 17 00:00:00 2001 From: Spekular Date: Wed, 9 Sep 2020 17:28:23 +0200 Subject: [PATCH 6/9] CMAKE fixes from code review Co-authored-by: Tres Finocchiaro --- CMakeLists.txt | 2 +- cmake/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 59818474b8d..a28cee8e97a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -45,7 +45,7 @@ IF(VERSION_STAGE) ENDIF() IF(VERSION_BUILD) MATH(EXPR VERSION_RELEASE_BUMP "${VERSION_RELEASE}+1") - SET(VERSION "${VERSION_MAJOR}.${VERSION_MINOR}.${VERSION_RELEASE_BUMP"}) + SET(VERSION "${VERSION_MAJOR}.${VERSION_MINOR}.${VERSION_RELEASE_BUMP}") SET(VERSION "${VERSION}-${VERSION_BUILD}") ENDIF() diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt index 78aa86e917c..207c3aaec94 100644 --- 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) - math(EXPR CPACK_PACKAGE_VERSION_PATH_BUMP "${CPACK_PACKAGE_VERSION_PATH}+1") + MATH(EXPR CPACK_PACKAGE_VERSION_PATH_BUMP "${CPACK_PACKAGE_VERSION_PATH}+1") SET(CPACK_PACKAGE_VERSION_PATCH "${CPACK_PACKAGE_VERSION_PATCH_BUMP}-${VERSION_BUILD}") ENDIF() SET(CPACK_PACKAGE_INSTALL_DIRECTORY "${PROJECT_NAME_UCASE}") From de05889dbb4dd4a4a233ecea81db7f661904b88b Mon Sep 17 00:00:00 2001 From: Spekular Date: Thu, 10 Sep 2020 21:58:11 +0200 Subject: [PATCH 7/9] Remove unnecessary changes to CMake logic --- CMakeLists.txt | 2 -- cmake/CMakeLists.txt | 3 +-- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index a28cee8e97a..aaeec055f30 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,8 +44,6 @@ IF(VERSION_STAGE) SET(VERSION "${VERSION}-${VERSION_STAGE}") ENDIF() IF(VERSION_BUILD) - MATH(EXPR VERSION_RELEASE_BUMP "${VERSION_RELEASE}+1") - SET(VERSION "${VERSION_MAJOR}.${VERSION_MINOR}.${VERSION_RELEASE_BUMP}") SET(VERSION "${VERSION}-${VERSION_BUILD}") ENDIF() diff --git a/cmake/CMakeLists.txt b/cmake/CMakeLists.txt index 207c3aaec94..833fad5819f 100644 --- a/cmake/CMakeLists.txt +++ b/cmake/CMakeLists.txt @@ -9,8 +9,7 @@ IF(VERSION_STAGE) SET(CPACK_PACKAGE_VERSION_PATCH "${CPACK_PACKAGE_VERSION_PATCH}-${VERSION_STAGE}") ENDIF() IF(VERSION_BUILD) - MATH(EXPR CPACK_PACKAGE_VERSION_PATH_BUMP "${CPACK_PACKAGE_VERSION_PATH}+1") - SET(CPACK_PACKAGE_VERSION_PATCH "${CPACK_PACKAGE_VERSION_PATCH_BUMP}-${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") From e034f699fb6351699743eff5682cb224f5014a02 Mon Sep 17 00:00:00 2001 From: Spekular Date: Thu, 10 Sep 2020 22:35:37 +0200 Subject: [PATCH 8/9] More const, more reference --- include/ProjectVersion.h | 2 +- src/core/ProjectVersion.cpp | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/ProjectVersion.h b/include/ProjectVersion.h index 8573f2f45d2..8f705ad1629 100644 --- a/include/ProjectVersion.h +++ b/include/ProjectVersion.h @@ -48,7 +48,7 @@ class ProjectVersion int getMajor() const { return m_major; } int getMinor() const { return m_minor; } int getPatch() const { return m_patch; } - QStringList getLabels() const { return m_labels;} + const QStringList& getLabels() const { return m_labels;} CompareType getCompareType() const { return m_compareType; } ProjectVersion setCompareType(CompareType compareType) { m_compareType = compareType; return * this; } diff --git a/src/core/ProjectVersion.cpp b/src/core/ProjectVersion.cpp index a863143e0f6..6f3dcddbeb1 100644 --- a/src/core/ProjectVersion.cpp +++ b/src/core/ProjectVersion.cpp @@ -93,20 +93,20 @@ int ProjectVersion::compare(const ProjectVersion & a, const ProjectVersion & b, if(aPat != bPat){ return aPat - bPat; } // Decide how many optional identifiers we care about - int numLabels = qMax(0, limit - 3); - auto aLabels = a.getLabels().mid(0, numLabels); - auto bLabels = b.getLabels().mid(0, numLabels); + 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 - int commonLabels = qMin(aLabels.size(), bLabels.size()); + 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++){ - QString labelA = aLabels.at(i); - QString labelB = bLabels.at(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 From 888e87d1112a4f296a161b8e1599b1d4e1a4553c Mon Sep 17 00:00:00 2001 From: Spekular Date: Thu, 17 Sep 2020 16:18:50 +0200 Subject: [PATCH 9/9] Apply suggestions from code review --- include/ProjectVersion.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/ProjectVersion.h b/include/ProjectVersion.h index 8f705ad1629..e938ccc81c7 100644 --- a/include/ProjectVersion.h +++ b/include/ProjectVersion.h @@ -48,7 +48,7 @@ class ProjectVersion int getMajor() const { return m_major; } int getMinor() const { return m_minor; } int getPatch() const { return m_patch; } - const QStringList& getLabels() const { return m_labels;} + const QStringList& getLabels() const { return m_labels; } CompareType getCompareType() const { return m_compareType; } ProjectVersion setCompareType(CompareType compareType) { m_compareType = compareType; return * this; } @@ -60,7 +60,7 @@ class ProjectVersion int m_major = 0; int m_minor = 0; int m_patch = 0; - QStringList m_labels = QStringList(); + QStringList m_labels; CompareType m_compareType; } ;