Use valid Semver versions for pre-releases #5636

Spekular · 2020-08-16T19:59:23Z

Reworks the way optional identifiers are handled to be more in line with Semver spec, adds a few new tests.

Our current versioning of MAJOR.MINOR.PATCH.COMMITS for builds from source is not a valid semantic version. I've added a workaround for versions up to 1.2.2.x to avoid breaking older comparisons, that converts them to 1.2.3-x. This is intentionally disabled for later versions as don't believe we should continue to use this versioning.

In order for everything to work, this PR (or another PR merged first) needs to update the versioning used for builds.

Builds with no pre-release tag based on version x.y.z with c extra commits must be versioned x.y.z+1-c
- Example: 1.2.2.42 becomes 1.2.3-42
Builds with a pre-release tag must separate the iteration from the stage, and can have commits appended
- Example: 1.2.0-rc1 should have been 1.2.0-rc.1, which could then have commits appended, 1.2.0-rc.1.42

For reference (source): 1.2.2-42 < 1.2.2 < 1.2.3-42 < 1.2.3-rc < 1.2.3-rc.9 < 1.2.3-rc.42 < 1.2.3

Spekular

Minor fixes that I only noticed when reviewing the github diff.

src/core/ProjectVersion.cpp

tests/src/core/ProjectVersionTest.cpp

LmmsBot · 2020-08-16T20:04:32Z

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Linux (AppImage): lmms-1.2.3-708%2Bgd736e4d-linux-x86_64.AppImage (build link)

Windows

Windows 32-bit: lmms-1.2.3-708%2Bgd736e4daa-mingw-win32.exe (build link)
Windows 64-bit: lmms-1.2.3-708%2Bgd736e4daa-mingw-win64.exe (build link)
Windows 32-bit: lmms-1.2.2-msvc2017-win32.exe (build link)
Windows 64-bit: lmms-1.2.2-msvc2017-win64.exe (build link)

🤖

{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://8768-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-708%2Bgd736e4d-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8768?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://8764-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-708%2Bgd736e4daa-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8764?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://8767-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-708%2Bgd736e4daa-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/8767?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/8gt4j9xoccjp3pt8/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35259741"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/vqnhpx5dl0620nrx/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35259741"}]}, "commit_sha": "888e87d1112a4f296a161b8e1599b1d4e1a4553c"}

include/ProjectVersion.h

PhysSong · 2020-08-17T04:13:41Z

It seems like it can't compare some non-standard version we use properly, for example, 1.2.2 vs. 1.2.2.555.

Spekular · 2020-08-17T08:14:21Z

I think we should change the formatting of those versions to be either 1.2.3-555 or 1.3.0-555. This is a valid semantic version and compares in a suitable manner:

They're bigger than 1.2.2
They're smaller than 1.2.3-alpha/1.3.0-alpha
1.2.3-555 is less than 1.2.3-600

I think we can write an upgrade routine for older versions in the constructor. If the first three identifiers are "1.2.2" or less, we convert x.y.z.aa to x.y.(z+1), and prepend aa to the prerelease labels.

As for which of the two options to choose, it'd be great if we could have different versions on master and stable. Assuming the latest release is x.y.z, it makes a lot of sense to me that:

Stable builds would be of the form x.y.(z+1)-commits
Master builds would be of the form x.(y+1).0-commits

Veratil · 2020-08-17T20:13:14Z

I agree with Spekular. We should start sticking to semver, and we can write a preroutine to capture older formats and convert to what it should be in semver.

Spekular · 2020-08-26T20:13:12Z

Did some testing of the new CMake versioning logic:

Latest Tag	Additional Commits	Output Version	Valid?
v1.2.2	689	1.2.3-689+gc860b7852	✔️
v1.2.2.300	0	1.2.3.300	❌
v1.2.2.300	2	1.2.3-2+ga4a6b273a	❌
v1.2.3	0	1.2.3	✔️
v1.3.0-rc1	0	1.3.0-rc1	⭕
v1.3.0-rc1	5	1.3.0-rc1.5+ga4a6b273a	⭕
v1.3.0-rc.1	0	1.3.0-rc.1	✔️
v1.3.0-rc.1	10	1.3.0-rc.1.10+ga4a6b273a	✔️

Bolded tags are invalid and we don't have any existing tags in this format. I tested them anyway for completeness sake: they cause the logic to break.
Italicized tags are valid but not recommended. We have tags in this format and they work as you'd expect: sorting only breaks if you have too many rc versions.
FORCE_VERSION flag works as I'd expect, whatever you enter becomes the version.

Is there anything else to test? Otherwise this seems ready to merge as far as I can tell.

Spekular · 2020-08-27T13:30:09Z

@PhysSong re-requesting review since I've fixed the issue you mentioned
@tresf requesting review since you're familiar with CMake and had opinions on how we do versioning ;)

DomClark

I've only reviewed the C++ here - I'll leave the CMake to someone more familiar with it.

src/core/ProjectVersion.cpp

DomClark · 2020-09-02T21:30:33Z

Probably out of scope here, but I'll mention it since you're rewriting the comparison logic:

I don't really like part of the current comparison system, where a CompareType is set on a ProjectVersion prior to comparison. The CompareType is a property of the comparison operation, not of the ProjectVersions being compared, and the code should reflect that. (You could argue to the contrary, that a ProjectVersion with a CompareType represents a range of versions, but I think this is a bad abstraction. If this is the intention, then there should be a ProjectVersionRange type, or a conversion from any ProjectVersion to another ProjectVersion representing the base of its range for a given CompareType.)
Furthermore, for ProjectVersions holding CompareTypes, operator< does not define a strict weak order, which means that arbitrary ProjectVersions cannot be used successfully with many standard library containers and algorithms.

Spekular · 2020-09-03T18:03:06Z

@DomClark Great review, thanks! I'll make sure to extend the tests as well when I fix this. (Note to self: every unresolved conversation is an actionable TODO)

Probably out of scope here, but [...]
I don't really like part of the current comparison system, where a CompareType is set on a ProjectVersion prior to comparison.

Agreed on both counts.

- 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

Spekular · 2020-09-06T14:04:04Z

All outstanding comments should be fixed in the latest commit. @DomClark I'll request re-review from you to see if you approve of the fixes.

cmake/CMakeLists.txt

tresf · 2020-09-08T15:14:57Z

~~Out of curiosity, I tested some older upgrade routines and it appears they'd fail using proper semver:~~ (Edit: They pass, I read it wrong, sorry!)

~~https://jubianchi.github.io/semver-check/#/%3C0.4.0-20080622/0.4.0-beta1~~

Since most of the popular + older LSP tracks seem to be from the 0.4.15 era, the chances of this upgrade routine being need and not triggered are tremendously small/rare, but it's a risk worth identifying.

I was hoping to sort the LSP on project version, but we've added that feature retroactively so it's not indexed (and we never went back and crawled them). I'm sure a bash script on the webserver could find this out, but for the purposes of this PR I simply wanted to state the fall throughs as a future reference point if the edge-case is exposed via issue or support.

tresf · 2020-09-08T15:19:01Z

Whoops, I used that online tool wrong. My points are still valid, but irrelevant as the example I chose passes the online tool.

CMakeLists.txt

cmake/CMakeLists.txt

Co-authored-by: Tres Finocchiaro <tres.finocchiaro@gmail.com>

DomClark

C++ looks good to me. I like the attention given to tests.

There a couple of places I've pointed out that make unnecessary copies - this isn't much of a problem with Qt containers, since their data is shared and refcounted behind the scenes, but is something to bear in mind, and would have more of an impact with standard library containers. If you consider this premature optimisation, feel free to ignore it - it's just as functional as it is.

include/ProjectVersion.h

src/core/ProjectVersion.cpp

Spekular · 2020-09-10T19:58:42Z

Testing again with changes that I haven't pushed yet, everything seems to behave the same even if I remove the changes outside of VersionInfo.cmake.

Latest Tag	Additional Commits	Output Version	Valid?
v1.2.2	692	1.2.3-692+g30a03465c	✔️
v1.2.2.300	0	1.2.2.300 (Changed since last test)	❌
v1.2.2.300	2	1.2.3-2+g30a03465c	❌
v1.2.3	0	1.2.3	✔️
v1.3.0-rc1	0	1.3.0-rc1	⭕
v1.3.0-rc1	5	1.3.0-rc1.5+g30a03465c	⭕
v1.3.0-rc.1	0	1.3.0-rc.1	✔️
v1.3.0-rc.1	10	1.3.0-rc.1.10+g30a03465c	✔️

Spekular · 2020-09-16T15:24:14Z

Merge?

tresf · 2020-09-16T15:35:25Z

Merge?

I haven't tested it locally, but all of my concerns were addressed, so 👍 from me.

include/ProjectVersion.h

PhysSong · 2020-09-17T02:28:53Z

LGTM

Spekular

Changes suggested by @PhysSong

include/ProjectVersion.h

* 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>

Fix ProjectVersion handling of pre-releases

f2990bf

Spekular commented Aug 16, 2020

View reviewed changes

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

tests/src/core/ProjectVersionTest.cpp Outdated Show resolved Hide resolved

tests/src/core/ProjectVersionTest.cpp Outdated Show resolved Hide resolved

Spekular mentioned this pull request Aug 16, 2020

Don't give clips a hidden default name (Fix #5528) #5621

Merged

Veratil reviewed Aug 16, 2020

View reviewed changes

include/ProjectVersion.h Outdated Show resolved Hide resolved

Spekular added 3 commits August 21, 2020 16:18

Add workaround for old, non-standard version

8b400ba

Attempt to fix versioning

c860b78

More consistent comments

a4a6b27

Spekular changed the title ~~Fix ProjectVersion handling of pre-releases~~ Use valid Semver versions for pre-releases Aug 26, 2020

Spekular requested review from PhysSong and tresf August 27, 2020 13:28

DomClark reviewed Sep 2, 2020

View reviewed changes

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

Spekular requested a review from DomClark September 6, 2020 14:04

tresf reviewed Sep 8, 2020

View reviewed changes

cmake/CMakeLists.txt Outdated Show resolved Hide resolved

tresf reviewed Sep 9, 2020

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Spekular commented Sep 9, 2020

View reviewed changes

cmake/CMakeLists.txt Outdated Show resolved Hide resolved

CMAKE fixes from code review

30a0346

Co-authored-by: Tres Finocchiaro <tres.finocchiaro@gmail.com>

DomClark approved these changes Sep 9, 2020

View reviewed changes

include/ProjectVersion.h Outdated Show resolved Hide resolved

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

src/core/ProjectVersion.cpp Outdated Show resolved Hide resolved

Remove unnecessary changes to CMake logic

de05889

More const, more reference

e034f69

PhysSong reviewed Sep 17, 2020

View reviewed changes

include/ProjectVersion.h Outdated Show resolved Hide resolved

include/ProjectVersion.h Outdated Show resolved Hide resolved

DomClark mentioned this pull request Sep 17, 2020

Improvements to the upgrade routines #5660

Merged

Spekular commented Sep 17, 2020

View reviewed changes

include/ProjectVersion.h Outdated Show resolved Hide resolved

include/ProjectVersion.h Outdated Show resolved Hide resolved

Apply suggestions from code review

888e87d

Spekular merged commit af32800 into LMMS:master Sep 17, 2020

Spekular deleted the VersionCompareTests branch September 17, 2020 15:23

Spekular mentioned this pull request Sep 17, 2020

Fix semantic versioning #3594

Closed

PhysSong mentioned this pull request Oct 3, 2020

VersionInfo: Not tagged refs should be resolved better #5250

Closed

tresf mentioned this pull request Oct 20, 2020

Add Windows JACK support #5716

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use valid Semver versions for pre-releases #5636

Use valid Semver versions for pre-releases #5636

Spekular commented Aug 16, 2020 •

edited

Loading

Spekular left a comment

LmmsBot commented Aug 16, 2020 •

edited

Loading

Linux

Windows

PhysSong commented Aug 17, 2020

Spekular commented Aug 17, 2020 •

edited

Loading

Veratil commented Aug 17, 2020

Spekular commented Aug 26, 2020 •

edited

Loading

Spekular commented Aug 27, 2020

DomClark left a comment

DomClark commented Sep 2, 2020

Spekular commented Sep 3, 2020

Spekular commented Sep 6, 2020

tresf commented Sep 8, 2020 •

edited

Loading

tresf commented Sep 8, 2020

DomClark left a comment

Spekular commented Sep 10, 2020 •

edited

Loading

Spekular commented Sep 16, 2020

tresf commented Sep 16, 2020

PhysSong commented Sep 17, 2020

Spekular left a comment

Use valid Semver versions for pre-releases #5636

Use valid Semver versions for pre-releases #5636

Conversation

Spekular commented Aug 16, 2020 • edited Loading

Spekular left a comment

Choose a reason for hiding this comment

LmmsBot commented Aug 16, 2020 • edited Loading

Linux

Windows

PhysSong commented Aug 17, 2020

Spekular commented Aug 17, 2020 • edited Loading

Veratil commented Aug 17, 2020

Spekular commented Aug 26, 2020 • edited Loading

Spekular commented Aug 27, 2020

DomClark left a comment

Choose a reason for hiding this comment

DomClark commented Sep 2, 2020

Spekular commented Sep 3, 2020

Spekular commented Sep 6, 2020

tresf commented Sep 8, 2020 • edited Loading

tresf commented Sep 8, 2020

DomClark left a comment

Choose a reason for hiding this comment

Spekular commented Sep 10, 2020 • edited Loading

Spekular commented Sep 16, 2020

tresf commented Sep 16, 2020

PhysSong commented Sep 17, 2020

Spekular left a comment

Choose a reason for hiding this comment

Spekular commented Aug 16, 2020 •

edited

Loading

LmmsBot commented Aug 16, 2020 •

edited

Loading

Spekular commented Aug 17, 2020 •

edited

Loading

Spekular commented Aug 26, 2020 •

edited

Loading

tresf commented Sep 8, 2020 •

edited

Loading

Spekular commented Sep 10, 2020 •

edited

Loading