Skip to content

Use version comparison function #41102

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 20, 2024
Merged

Use version comparison function #41102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3536f7a
Use version comparison function
May 23, 2024
20d6614
Flip sign
May 23, 2024
4fe96b3
Add tests
May 23, 2024
15731de
Fix other spot
Forgind May 24, 2024
0b482b6
8.0.200.1 is not an invalid version
Forgind May 24, 2024
9a01f1e
disambiguate folders
Forgind May 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions ...esolvers/Microsoft.NET.Sdk.WorkloadManifestReader/SdkDirectoryWorkloadManifestProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,11 +170,38 @@ public void RefreshWorkloadManifests()

if (_workloadSet == null && availableWorkloadSets.Any())
{
var maxWorkloadSetVersion = availableWorkloadSets.Keys.Select(k => new ReleaseVersion(k)).Max()!;
var maxWorkloadSetVersion = availableWorkloadSets.Keys.Aggregate((s1, s2) => VersionCompare(s1, s2) >= 0 ? s1 : s2);
_workloadSet = availableWorkloadSets[maxWorkloadSetVersion.ToString()];
}
}

private static int VersionCompare(string first, string second)
{
if (first.Equals(second))
{
return 0;
}

var firstDash = first.IndexOf('-');
var secondDash = second.IndexOf('-');
firstDash = firstDash < 0 ? first.Length : firstDash;
secondDash = secondDash < 0 ? second.Length : secondDash;

var firstVersion = new Version(first.Substring(0, firstDash));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens if you pass in the string without removing the - part first? The dash is supported in versions but I recall there being a reason you needed to do it this way (comment)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I tried a fairly standard version with a - in a console project I use for testing, and it threw an exception. The dash is supported in ReleaseVersions, I think, but not Version? Or perhaps there are some strings with dashes that can be parsed into a Version while others can't?

var secondVersion = new Version(second.Substring(0, secondDash));

var comparison = firstVersion.CompareTo(secondVersion);
if (comparison != 0)
{
return comparison;
}

var modifiedFirst = "1.1.1" + (firstDash == first.Length ? string.Empty : first.Substring(firstDash));
var modifiedSecond = "1.1.1" + (secondDash == second.Length ? string.Empty : second.Substring(secondDash));

return new ReleaseVersion(modifiedFirst).CompareTo(new ReleaseVersion(modifiedSecond));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does release version have any limitations? Curious why you can't just compare the part after the dash directly rather than making it into a release version first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it needed some digits before the dash to make a ReleaseVersion, but it couldn't support four '.'-separated numbers. I originally proposed doing a simpler comparison like a string comparison, since my current understanding is that the part after the dash is just compared alphanumerically, but dsplaisted was concerned that there might be more complexity and suggested doing it with a ReleaseVersion.

}

void ThrowExceptionIfManifestsNotAvailable()
{
if (_exceptionToThrow != null)
Expand Down Expand Up @@ -240,10 +267,10 @@ void AddManifest(string manifestId, string manifestDirectory, string featureBand

void ProbeDirectory(string manifestDirectory, string featureBand)
{
(string? id, string? finalManifestDirectory, ReleaseVersion? version) = ResolveManifestDirectory(manifestDirectory);
(string? id, string? finalManifestDirectory, string? version) = ResolveManifestDirectory(manifestDirectory);
if (id != null && finalManifestDirectory != null)
{
AddManifest(id, finalManifestDirectory, featureBand, version?.ToString() ?? Path.GetFileName(manifestDirectory));
AddManifest(id, finalManifestDirectory, featureBand, version ?? Path.GetFileName(manifestDirectory));
}
}

Expand Down Expand Up @@ -348,7 +375,7 @@ void ProbeDirectory(string manifestDirectory, string featureBand)
/// Given a folder that may directly include a WorkloadManifest.json file, or may have the workload manifests in version subfolders, choose the directory
/// with the latest workload manifest.
/// </summary>
private (string? id, string? manifestDirectory, ReleaseVersion? version) ResolveManifestDirectory(string manifestDirectory)
private (string? id, string? manifestDirectory, string? version) ResolveManifestDirectory(string manifestDirectory)
{
string manifestId = Path.GetFileName(manifestDirectory);
if (_outdatedManifestIds.Contains(manifestId) ||
Expand All @@ -361,26 +388,22 @@ void ProbeDirectory(string manifestDirectory, string featureBand)
.Where(dir => File.Exists(Path.Combine(dir, "WorkloadManifest.json")))
.Select(dir =>
{
ReleaseVersion? releaseVersion = null;
ReleaseVersion.TryParse(Path.GetFileName(dir), out releaseVersion);
return (directory: dir, version: releaseVersion);
})
.Where(t => t.version != null)
.OrderByDescending(t => t.version)
.ToList();
return (directory: dir, version: Path.GetFileName(dir));
});

// Assume that if there are any versioned subfolders, they are higher manifest versions than a workload manifest directly in the specified folder, if it exists
if (manifestVersionDirectories.Any())
{
return (manifestId, manifestVersionDirectories.First().directory, manifestVersionDirectories.First().version);
var maxVersionDirectory = manifestVersionDirectories.Aggregate((d1, d2) => VersionCompare(d1.version, d2.version) > 0 ? d1 : d2);
return (manifestId, maxVersionDirectory.directory, maxVersionDirectory.version);
}
else if (File.Exists(Path.Combine(manifestDirectory, "WorkloadManifest.json")))
{
var manifestPath = Path.Combine(manifestDirectory, "WorkloadManifest.json");
try
{
var manifestContents = WorkloadManifestReader.ReadWorkloadManifest(manifestId, File.OpenRead(manifestPath), manifestPath);
return (manifestId, manifestDirectory, new ReleaseVersion(manifestContents.Version));
return (manifestId, manifestDirectory, manifestContents.Version);
}
catch
{ }
Expand Down
38 changes: 13 additions & 25 deletions ...crosoft.NET.Sdk.WorkloadManifestReader.Tests/SdkDirectoryWorkloadManifestProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,23 +135,29 @@ var sdkDirectoryWorkloadManifestProvider
.BeEmpty();
}

[Fact]
public void ItReturnsLatestManifestVersion()
[Theory]
[InlineData("11.0.1", "11.0.2", "11.0.2-rc.1", "11.0.2")]
[InlineData("8.0.200", "8.0.201", "8.0.105", "8.0.201")]
[InlineData("8.0.203.1", "8.0.203", "8.0.200-rc.1", "8.0.203.1")]
[InlineData("9.0.100-preview.2", "9.0.100-preview.2.3.4", "9.0.100-preview.2.4.3", "9.0.100-preview.2.4.3")]
[InlineData("8.0.201.1-preview", "8.0.201.1-rc.1", "8.0.201.1-rc.2", "8.0.201.1-rc.2")]
[InlineData("8.0.200-servicing.23015", "8.0.200-preview.7.30301", "8.0.200-servicing.23201", "8.0.200-servicing.23201")]
public void ItReturnsLatestManifestVersion(string first, string second, string third, string answer)
{
Initialize();
Initialize(identifier: answer);

CreateMockManifest(_manifestRoot, "5.0.100-preview.5", "ios", "11.0.3", true);

CreateMockManifest(_manifestRoot, "5.0.100", "ios", "11.0.1", true);
CreateMockManifest(_manifestRoot, "5.0.100", "ios", "11.0.2", true);
CreateMockManifest(_manifestRoot, "5.0.100", "ios", "11.0.2-rc.1", true);
CreateMockManifest(_manifestRoot, "5.0.100", "ios", first, true);
CreateMockManifest(_manifestRoot, "5.0.100", "ios", second, true);
CreateMockManifest(_manifestRoot, "5.0.100", "ios", third, true);

var sdkDirectoryWorkloadManifestProvider
= new SdkDirectoryWorkloadManifestProvider(sdkRootPath: _fakeDotnetRootDirectory, sdkVersion: "5.0.100", userProfileDir: null, globalJsonPath: null);

GetManifestContents(sdkDirectoryWorkloadManifestProvider)
.Should()
.BeEquivalentTo("ios: 11.0.2/5.0.100");
.BeEquivalentTo($"ios: {answer}/5.0.100");
}

[Fact]
Expand Down Expand Up @@ -379,24 +385,6 @@ var sdkDirectoryWorkloadManifestProvider
.BeEquivalentTo("ios: 11.0.2/8.0.100", "android: 33.0.2/8.0.100", "maui: 15.0.1-rc.456/8.0.200-rc.2");
}

[Fact]
public void ItThrowsIfWorkloadSetHasInvalidVersion()
{
Initialize("8.0.200");

CreateMockManifest(_manifestRoot, "8.0.100", "ios", "11.0.1", true);
CreateMockManifest(_manifestRoot, "8.0.100", "ios", "11.0.2", true);
CreateMockManifest(_manifestRoot, "8.0.200", "ios", "12.0.1", true);

CreateMockWorkloadSet(_manifestRoot, "8.0.200", "8.0.200.1", """
{
"ios": "11.0.2/8.0.100"
}
""");

Assert.Throws<FormatException>(() => new SdkDirectoryWorkloadManifestProvider(sdkRootPath: _fakeDotnetRootDirectory, sdkVersion: "8.0.200", userProfileDir: null, globalJsonPath: null));
}

[Fact]
public void ItThrowsIfManifestFromWorkloadSetIsNotFound()
{
Expand Down