Skip to content
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

Merged
merged 4 commits into from
Nov 14, 2019

Conversation

nguerrera
Copy link
Contributor

@nguerrera nguerrera commented Nov 12, 2019

Usage:

$([MSBuild]::VersionEquals(a, b))
$([MSBuild]::VersionNotEquals(a, b))
$([MSBuild]::VersionLessThan(a, b))
$([MSBuild]::VersionLessThanOrEquals(a, b))
$([MSBuild]::VersionLessThan(a, b))
$([MSBuild]::VersionGreaterThanOrEquals(a, b))

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

@nguerrera nguerrera requested review from rainersigwald and a team November 12, 2019 23:29
@nguerrera nguerrera added this to the MSBuild 16.5 milestone Nov 12, 2019
Copy link
Member

@rainersigwald rainersigwald left a 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

src/Build.UnitTests/Evaluation/Expander_Tests.cs Outdated Show resolved Hide resolved
@nguerrera
Copy link
Contributor Author

nguerrera commented Nov 13, 2019

I like the idea of having better ergonomics for conditions.

So, how about all of these:

VersionGreaterThan
VersionGreaterThanOrEquals
VersionLessThan
VersionLessThanOrEquals
VersionEquals
VersionNotEquals

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.
@nguerrera
Copy link
Contributor Author

nguerrera commented Nov 14, 2019

Perhaps VersionNotEquals is unnecessary since leading !$([MSBuild]::VersionEquals(a, b)) is pretty readable and fewer characters anyway?

Copy link
Member

@rainersigwald rainersigwald left a 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?

@eerhardt
Copy link
Member

eerhardt commented Apr 9, 2020

Did we consider having a IsVersion or TryParse kind of method along with these? That way someone could use it for $(LangVersion), which may be of the form x.y, but it also may be latest, preview, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need a performace and unified way to compare versions
3 participants