-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add intrinsic property function to compare versions #4911
Add intrinsic property function to compare versions #4911
Conversation
965c1a4
to
139d69b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big question: Is this sufficiently ergonomic? Or should we have $([MSBuild]::VersionGreater(a, b))
and friends to fit more easily into conditions?
If you can build a project that uses this with the env var MSBuildLogPropertyFunctionsRequiringReflection=1
and don't see it logged, then I guess we don't need a fast-path for it, but we do have fastpaths for string methods so I suspect we might. Should be easy to throw in Expander.cs
I like the idea of having better ergonomics for conditions. So, how about all of these: VersionGreaterThan And relegate comparison that returns an int to an implementation detail. I added Than because VersionLess read like version-less (as in having no version) to me. EDIT: Tweaked this comment to reflect where I landed while doing it. |
Will be used to implement version comparison intrinic property functions. Allows major version only (e.g. "3" is 3.0.0.0), ignores leading 'v' (e.g. "v3.0" is 3.0.0.0). Ignores semver prerelease and metadata portions (e.g. "1.0.0-preview+info" is 1.0.0.0). Treats unspecified components as 0 (e.g. x == x.0 == x.0.0 == x.0.0.0). Unlike System.Version, does not tolerate whitespace, and '+' is ignored as semver metadata as described above, not tolerated as positive sign of integer component. Tolerating leading 'v' allows using $(TargetFrameworkVersion) directly. Ignoring semver portions allows, for example, checking >= major.minor while still in development of that release. Implemented as a struct to avoid heap allocation. Parsing is done without heap allocation at all on .NET Core. However, on .NET Framework, the integer component substrings are allocated as there is no int.Parse on span there.
139d69b
to
a54c5be
Compare
Perhaps VersionNotEquals is unnecessary since leading |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I could go either way on VersionNotEquals
vs !VersionEquals
but you already wrote it so let's leave it?
Did we consider having a |
Usage:
Versions are parsed like System.Version, with the following exceptions:
Leading v or V is ignored. Allows comparison to $(TargetFrameworkVersion)
Everything from first '-' or '+' to end of string is ignored. Allows passing in semantic versions, though the order is not the same as semver. Instead, prerelease specified and build metadata do not have any sorting weight. This can be useful, for example, to turn on a feature for >= x.y and have it kick in on x.y.z-pre.
Unspecified parts are same as zero value parts. (x == x.0 == x.0.0 == x.0.0.0)
Whitespace is not allowed in integer components
major version only is valid ("3" is equal to "3.0.0.0")
+
is not allowed as positive sign in integer components (it is treated as semver metadata and ignored per above)The parser is adapted from System.Version source code, but simplified and tweaked to handle quirks above, with trivial opportunities to improve perf / reduce allocations taken. Tests are also adapted from corefx.
Fix #3212