Replace SemanticVersion

[straight-shoota]

SemanticVersion is a data type for representing version numbers.
It's used in the compiler to implement the compare_versions macro, which in turn is used to adapt C bindings depending on the library version.

The implementation of SemanticVersion and compare_versions is solid. But the design is problematic. It's only targeting version numbers conforming to semver (major.minor.patch-pre). semver is a popular versioning system, but far from the only one. The reality shows a great variety of software version schemes employed in the wild.

For the implementation of SemanticVersion, a great number of current software versions can be considered compatible to semver, even if it doesn't actually follow semver, but has a similar format. But as soon as the format doesn't match up, SemanticVersion and compare_versions can't handle a version (such as 1.0 for example). For a general-purpose implementation, this is unacceptable. It should support a wide variety of version number formats.

SemanticVersion is therefore pretty much unusable for anything, unless all versions it needs to handle are guaranteed to conform to semver. In any real-world application, with versions coming from different vendors, this is very unlikely. shards for example uses a custom implementation for comparing version numbers that is more flexible.

Ruby has Gem::Version which is very versatile. I have a Crystal version of that type with a similar interface but a different internal implementation which should be more efficient and could even be further improved for comparison performance.
I think it could be used as a drop-in replacement of SemanticVersion and renamed to SoftwareVersion. It's available here: https://gist.github.com/straight-shoota/d8eec4a346018b0a51c1730b2a5c2bf8
I could also consider publishing it as a shard, but I think it would also fit in the stdlib and would improve compare_versions as well.

[asterite]

👍 From me.

About the implementation, I wonder why it's not storing an Array(String | Int32) as the internal representation. Having to parse the version on each comparison seems... strange (and probably not very efficient). That said, maybe comparing versions isn't something that's too frequent so it can be acceptable. And as long as we agree on an API we can always change the internal implementation later on.

[straight-shoota]

Yeah, it's a trade-off between instantiation and comparison. Splitting the version string in an array is by itself costly. Comparing integers is obviously very fast, but the custom comparison method should be pretty efficient.
Performance obviously depends on usage characteristics, especially the ratio between instantiation and operations like a comparison. I haven't performed a detailed benchmark. I'm mostly interested in functionality and at least decent performance which is both provided.

But refactoring is always possible. Gem::Version uses an array as internal representation and this could also be implemented with the same interface.

[straight-shoota]

@RX14 in #7642 (comment)

I think things like Gem::Version prove that comparing version numbers isn't nearly as easy as it looks, there's a bunch of edgcases and tradeoffs. Semver fixed that, by being a rigid spec with rigid semantics. That means it's simple and unopinionated to make a SemanticVersion class.

I'd prefer this more generic class to be in a shard, where there are no backwards-compatibility guarantees past 1.0 so there won't be problems with locking in a bad behaviour. The shard should still have a way to validate that a string is exactly a semver string.

As for what happens to SemanticVersion i'd be fine with moving it into the compiler sources and making it private.

I'm answering here, because I think we should discuss the general design in the issue and only implementation in the PR.

I proposed publishing it as a shard as a possible alternative. The downside of this approach would be that the compare_versions macro can only handle (somewhat) semver-compatible version schemes. This strictly limits its usability for real world applications.

[j8r]

Following my question, SemanticVersion doesn't seem to be as used as I thought. Half of the search comes from Cloudfoundry's depwatcher.

compare_versions being mainly used to compare OpenSSL versions, LibCrypto::OPENSSL_VERSION >= 0x10100000 can be used instead, like in this commit. That was @ysbaddaden suggested too in the first place.

I even think the language can perfectly live without SemanticVersion/SoftwareVersion, being moved in separate shard. If compare_version is really needed, a little helper, without having a whole class, can do the job fine.

[bcardiff]

I haven't need more that semver so far. Along the way one might need to re-learn that M.m.p-x < M.m.p I tend to forget it :-P.

Change compare_versions to handle a superset of semver seems fine. But I would not get rid to SemanticVersion, at least for now.

[Blacksmoke16]

Thoughts on also adding methods to handle altering the version?

sem_ver = SoftwareVersion.parse "1.1.8"

sem_ver.increment(:minor).to_s # => "1.2.0"

[straight-shoota]

@Blacksmoke16 I don't think a dedicated API would be a good thing. But you should be able to do something like version.copy_with(major: version.major + 1) which is also much more versatile.

[Blacksmoke16]

I would be ok with that too. Anything to really just make working with them a bit easier as the current state is dedicated to just parsing and comparing versus being able to manipulate.