Fix PlatformDetection.GetFrameworkVersion()

[safern]

Fixes PlatformDetection.GetFrameworkVersion() bug where it was not returning the right version. This way will map the shipped version ranges to the actual framework version correctly. If we ship a new version we would just need to add its range of version.

This is needed to unblock the Serialization effort.

cc: @danmosemsft @stephentoub @AlexGhiondea

FYI: @ViktorHofer @krwq

[safern]

This could also be done like:

if (version.BuildNumber >= 2500)
   return new Version(4, 7, 1);
if (version.BuildNumber >= 2000)
   return new Version(4, 7, 0);

   ...

}

but with the Range pattern if the build numbers/revision patterns changes we would be safe as we could keep adding ranges into the array to map to new versions. As for example 4.5.2 doesn't follow the incremental build pattern, so it is more accurate to follow the Range pattern.

Didn't add 4.5.1 ranges as it had too much releases and we want to unblock the Serialization PR ASAP. They can be added in a separate PR.

[danmoseley]

I don't follow, how would 4.5.2 not work in this style?

if (version >= new Version(4, 7, 2500))
   return new Version(4, 7, 1);
if (version >= new Version(4, 6, 2000))
   return new Version(4, 7, 0);
.. etc

how is that not totally generalized?

[safern]

When we add 4.5.1 and 4.5 it would break as 4.5.2, 4.5.1 and 4.5 some versions intersect, so we need to create the ranges and the ifs would be more complicated. But if we don't think this is going to change soon and we don't care about 4.5.1 and 4.5 we could do the if style.

[danmoseley]

I'm not sure it would be any better than the ranges. I would do if style.

[safern]

But do we want to parse 4.5.1 and/or 4.5? Or just return 4.5.2 for lower than 4.6? Because the ranges approach was because parsing 4.5.x is imposible with the if pattern

[danmoseley]

lol I am being dense somehow as I don't see why. There is a continuous line of versions and the if's cut off a piece at a time. Maybe you can explain on Monday. :)

Or if @stephentoub follows then feel free to merge meantime..

[safern]

Haha no worries. I can show you the spreadsheet on Monday. In example, we shipped 4.5.1 version as 4.0.2300.2267 and 4.5.2 version as 4.0.1019.3927 and as you can see the 4.5.1 is > than 4.5.2 so the if pattern for some of the 4.5.1 will return 4.5.2

The problem is that the incremental version pattern started from 4.6 and before the build and revision numbers where calculated through the date it was built and since we have a lot of shipping/servicing branches before 4.6 there are a lot of builds for each framework <= 4.5.2 which intersect and not always are incremental.

[danmoseley]

Fair enough we can always improve it later.

[karelz]

@safern fixing milestone to 2.1. This PR wasn't against 2.0 branch.

[safern]

@safern fixing milestone to 2.1. This PR wasn't against 2.0 branch.

I added 2.0 as it was needed for the Serialization effort, so idf if this needs to actually be ported. I thought it might be ported with the Serialization PRs, @ViktorHofer might know better.

[ViktorHofer]

Yes it will be ported into the release/2.0 branch. I'm tracking that. @karelz tbh I'm not very happy with the current milestone strategy. I would definitely put a 2.0 milestone on this issue but I know that you already had a long discussion about that with @stephentoub

[karelz]

@ViktorHofer I understand your view point. What you propose is LOTS of extra manual work and room for errors. My current scheme is simple, fast to do (query + bulk-edit) and if people link issues properly (which is sadly not the case here), then everything is fine.

The only other alternative is IMO to give up on milestones on PRs entirely, which I would really dislike.

If anyone has better idea, I am open to suggestions - but I would ask that person to follow those suggestions for few weeks to understand the "hidden" costs as well.

[ViktorHofer]

I understand your view point as well. I don't really wanna argue about it as you and stephen already brought pros and cons of each approach on the table. I'm fine with it.

[stephentoub]

The only other alternative is IMO to give up on milestones on PRs entirely, which I would really dislike.

Not to rehash the conversion, but you know my opinion here: I don't believe the current approach works, and I believe we should either change how the milestones on PRs are used, or not use them at all. The only accurate way to know whether a change is in a branch is to go look at the code in the branch and see if it's there, and second to that looking at the commit history for the branch. This extra metadata on PRs is extra work, provides no additional information, and is potentially misleading. I know you disagree with me, but my $.02.