-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Need a performace and unified way to compare versions #3212
Comments
cc @dsplaisted @nguerrera @rainersigwald I hope we can discuss it on next Monday's sync meeting |
What's the performance concern? Something you've measured, or just theoretical? |
@nguerrera do you have concrete example? |
Before we can say what the performance issue is (if any), I think we have to say what the code we would write would be.
|
@wli3 This is on the agenda for Monday's meeting |
(edit description according to above) |
The answer @wli3 figured out to 1 is:
This seems pretty reasonable to me, and I have no reason to suspect it'd be a significant performance bottleneck--that's why I was asking "measured or theoretical?". |
If the answers to my questions: are:
Then, great we'll change our code to that. If there's a no in there, then we may need an msbuild feature. |
Try
|
I think we first have to normalize both sides to four parts. |
Somewhat separate, but we would also like there to be a way to compare semantic/nuget versions. We set $(NETCoreSdkVersion) currently based on that and we would like customers to be able to reason about it with minimum versions. Currently, Roslyn has code that is enforcing it as an exact version, and I'd like to get them to change it to a min version, but it occurs to me that I don't know how to write that. |
Can we get NuGet's |
Ah, already proposed: dotnet/corefx#13526. |
If we had System.SemanticVersion, would we want to hardcode it in, as general enough? Or provide a way in which different package managers could plugin their own version comparers? I think there's little chance of having other package managers (using something else other than semver) with tight msbuild integration right? :) |
I'd say we expose the ".NET Framework way". That's what we've done historically with |
Pragmatically, I'd want a single type / msbuild expression that supports 4 parts as an extension to semver. But I am not inclined to get into that debate. :( |
For 16.0, I propose
That:
Think this would be totally useless without support for prerelease semvers? Speak up, please! |
I think this would be helpful without the prerelease semvers, but adding prerelease semvers is even better. Otherwise, we have to split, and if it's a property that might be, we always have to copy/test for it. Think .NET Core SDK version, which has prereleases and not. |
I think it would be helpful if |
How about But maybe it's sufficient to always do that, and doc the behavior. |
I think I'd prefer that or something that will read better than a literal true in code review. CompareVersionsIgnoringPrerelease or something. |
Do we also need to worry about versions with '+' and no '-', such as Could we get away with stripping after first non-digit-or-dot? Also, can you allow leading v so that we can compare directly against TargetFrameworkVersion without our ugly _TargetFrameworkVersionWithoutV? |
👍 |
Putting a note here that we will support |
(Putting that note here because I just approved a PR that uses it and points here.) |
@nguerrera |
Add intrinsic property functions to compare versions 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. * Remaining part of string from above is parsed with Version.Parse, then normalized to have 4 explicit parts to make 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
This implementation does not have such restrictions and can be used to compare, e.g. |
In msbuild condition. msbuild build will try to parse it as dec or hex first. So 2.11 will be smaller than 2.2.
At the same time. If appending 0 at the end as in 2.2.0, it will cause parse as number fail and consider it as Version. However, Version class's comparison does not consider 2.1 and 2.1.0 to be equal. In fact 2.1 < 2.1.0. This will also occur in Property Function.
It is different behavior of NuGet version.
Also there is performance concern.
The text was updated successfully, but these errors were encountered: