Update for 8 --> 9 #268
Update for 8 --> 9 #268
Conversation
| @@ -27,8 +27,8 @@ public class VSVersionTests | |||
| [InlineData(new string[] { "2.2.100", "2.2.200" }, new bool[] { false, false })] | |||
| [InlineData(new string[] { "2.2.100", "2.2.200", "2.2.300" }, new bool[] { false, true, false })] | |||
| [InlineData(new string[] { "3.0.0", "3.0.1", "5.0.0" }, new bool[] { true, true, false })] | |||
| [InlineData(new string[] { "5.0.0", "5.0.1", "6.0.1" }, new bool[] { true, false, true })] | |||
| [InlineData(new string[] { "6.0.0", "6.0.1", "7.0.0" }, new bool[] { true, true, false })] | |||
| [InlineData(new string[] { "6.0.0", "6.0.1", "7.0.1" }, new bool[] { true, false, false })] | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I did note that main built, so I think there's something I'm missing here. Just wanted to call out my confusion.
There was a problem hiding this comment.
We need to dig into this as this doesn't match the pattern i would expect from past history. I'd have to run the test to know for sure why it works this way.
There was a problem hiding this comment.
I think this test is doing essentially the same thing as the other test I commented on, so these outputs are as I would expect given that.
There was a problem hiding this comment.
By the way, the answer to the question I posed in October is that 6.0.1 wasn't in any bucket and wasn't above the upper limit, so it was automatically uninstallable.
| { (new SemanticVersion(2, 2, 100), new SemanticVersion(2, 2, 200)), string.Format(LocalizableStrings.WindowsRequirementExplanationString, " 2017") }, | ||
| { (new SemanticVersion(2, 2, 200), new SemanticVersion(2, 2, 500)), string.Format(LocalizableStrings.WindowsRequirementExplanationString, " 2019") }, | ||
| { (new SemanticVersion(3, 0, 100), new SemanticVersion(5, 0, 600)), string.Format(LocalizableStrings.WindowsRequirementExplanationString, " 2019") }, | ||
| { (new SemanticVersion(5, 0, 600), new SemanticVersion(6, 0, 600)), string.Format(LocalizableStrings.WindowsRequirementExplanationString, " 2019") }, |
There was a problem hiding this comment.
lines 29 and 30 should just be the line 29 from before. Basically, this maps SDKs to VS versions. Since 2022 it everything from 6.0.100 forward, that one line doesn't need to change.
There was a problem hiding this comment.
So basically revert the meaningful changes to this file other than increasing the semantic version upper limit?
There was a problem hiding this comment.
I believe so yes. You'll still need the spelling fix obviously.
| [InlineData("remove --all --sdk", new string[] { "1.0.0", "1.0.1" })] | ||
| [InlineData("dry-run --all --sdk", new string[] { "1.0.0", "1.0.1" })] | ||
| [InlineData("whatif --all --sdk", new string[] { "1.0.0", "1.0.1" })] | ||
| [InlineData("remove --all --sdk", new string[] { "1.0.0", "1.0.1", "7.0.1" })] |
There was a problem hiding this comment.
Why was 7 needed to be added here? (I'm just comparing to our last TFM update and looking at other data inputs for this test method)
There was a problem hiding this comment.
To be totally honest, I have no recollection of anything having to do with this PR anymore, so I have no idea.
There was a problem hiding this comment.
Ok; think I figured this out. This just has to do with how many versions (in versions above) happen to be in each of the WindowsVersionDivisionsToExplanation buckets above. We mark a version as 'required' if it's the highest version within a bucket. Previously, there was a bucket that included 1.0.0, 1.0.1, and 1.1.0, so the first two were marked as uninstallable but the third was marked as required. Conveniently, there was almost a 1:1 correspondence between lines in that dictionary and versions used for this test—there just wasn't anything between 5.0.600 and 6.0.600. (Note that a version like 2.2.100 that's on two lines is in the later bucket; they're [v1, v2).)
With my change to increase UpperLimit to 9.0.0, 7.0.1 found itself in the same bucket as 8.0.1, which meant it was no longer the highest in its bucket and hence became uninstallable.
There was a problem hiding this comment.
ahh, so in the other file we need to add additional buckets?
There was a problem hiding this comment.
If we don't want 7.0.1 to be uninstallable, then yes. But then I have to ask why 7.0.1 shouldn't be uninstallable...and also why some of those other versions shouldn't be uninstallable. Are we really going to say we can't uninstall 1.0.0 unless we have 1.1.0? Or that we want to keep both 2.1.200 and 2.1.300? Most of those are out-of-support; part of me wonders if we can just delete everything before the 3.0.100 line. This is definitely much more your area, though, so if you think we should just add more buckets, I can do that.
There was a problem hiding this comment.
It sounds like your suggestion then might be to revert my last commit then add another line for [6.0.600, 7.0.600)?
There was a problem hiding this comment.
Ugh, the tool is making silly guesses. we haven't shipped bundles in VS since 16.2 and .NET Core 2.2. A standalone bundle doesn't imply that VS is using it, nor does the highest band because you can have an unlimited number of instances from 15.11 through 17.9 covering each feature band.
There was a problem hiding this comment.
Sounds like joeloff recommends deprecating the uninstall tool entirely since we can't get it right anyway 😜
There was a problem hiding this comment.
@joeloff are you saying the tool is silly or the tests are silly? I think I'm close to getting a simple fix for the tests which adding in the buckets we were talking about above and adds in new items in the version object. I am just not sure about some of the true/false values for some of the other tests. I'll push something in the morning.
There was a problem hiding this comment.
The tool is silly :). Back when this was designed, .NET used to ship the standalone SDK bundles in VS. Bundles in VS require overhead to track. For example, we shipped both 1.0 and 1.1 SDKs in VS 15 (eventually 2.0, 2.1, and 2.2). The tools is simply making an assumption that if you have something like the 1.1.14 SDK (which was the last version we shipped for 1.1) then it's likely that it's used by VS. And leaving the highest version of a major.minor .NET SDK on the machine is less likely to break someone.
| @@ -26,10 +26,10 @@ public class VSVersionTests | |||
| [InlineData(new string[] { "2.1.500", "2.1.400", "2.1.600" }, new bool[] { false, true, false })] | |||
| [InlineData(new string[] { "2.2.100", "2.2.200" }, new bool[] { false, false })] | |||
| [InlineData(new string[] { "2.2.100", "2.2.200", "2.2.300" }, new bool[] { false, true, false })] | |||
| [InlineData(new string[] { "3.0.0", "3.0.1", "5.0.0" }, new bool[] { true, true, false })] | |||
There was a problem hiding this comment.
@Forgind @joeloff I think I figured out these tests now. The WindowsVersionDivisionsToExplanation dictionary is only used in ApplyWindowsVersionDivisions. This method is applying the buckets to the SDKs only. So this test was previously using runtime versioning which didn't fit into the buckets. To fix this, I modified the test to use SDK versions that fit within the buckets and the test passes now.
Increases the maximum allowed version to 9