-
-
Notifications
You must be signed in to change notification settings - Fork 964
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
Change .NET SDK Validator to account for backwards compatibility #2645
Change .NET SDK Validator to account for backwards compatibility #2645
Conversation
Pinging @lewing @am11 @timcassell as they were involved in the previous discussion |
LGTM, whatever unblocks dotnet/performance#4470 at this point and get the .NET 10 builds rolling. 😅 |
var versions = new List<Version>(lines.Count()); | ||
foreach (var line in lines) | ||
{ | ||
// Each line will start with the SDK version followed by the SDK path. Since we only support | ||
// targeting SDK versions by the major version, we'll just look at extracting the major and | ||
// minor versions. This is done by looking for the first two dots in the line. | ||
var firstDot = line.IndexOf('.'); | ||
if (firstDot < 0) | ||
continue; | ||
|
||
var secondDot = line.IndexOf('.', firstDot + 1); | ||
if (secondDot < 0) | ||
continue; | ||
|
||
if (Version.TryParse(line.Substring(0, secondDot), out var version)) | ||
{ | ||
versions.Add(version); | ||
} | ||
} | ||
|
||
return versions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do need to account for framework versions too? i.e.
var versions = new List<Version>(lines.Count()); | |
foreach (var line in lines) | |
{ | |
// Each line will start with the SDK version followed by the SDK path. Since we only support | |
// targeting SDK versions by the major version, we'll just look at extracting the major and | |
// minor versions. This is done by looking for the first two dots in the line. | |
var firstDot = line.IndexOf('.'); | |
if (firstDot < 0) | |
continue; | |
var secondDot = line.IndexOf('.', firstDot + 1); | |
if (secondDot < 0) | |
continue; | |
if (Version.TryParse(line.Substring(0, secondDot), out var version)) | |
{ | |
versions.Add(version); | |
} | |
} | |
return versions; | |
return lines.Select(line => Version.Parse(line.Split(' ')[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timcassell this will break net framework since it is accounting for only the major version. Sometimes like net472 will break? Suggested code was fixing that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Framework sdk versions are retrieved in a separate method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also few others: netcoreapp3.1 and netstandrd2.1 etc. (not sure if tests are exercising them, but we do have tests for those TFMs).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is comparing major and minor version, so netcoreapp3.1 should work fine.
BDN doesn't support netstandard builds (it must be executable).
This is an alternative approach that was discussed in #2641 to account for backwards compatibility in the SDK (e.g. that you can target previous versions from a later version of the SDK). I have verified locally on my machine that with this BDN installed, that we can run the wasm benchmarks with
--runtimes wasmnet90
passed in while having a .NET 10 SDK installed via the changes in dotnet/runtime#106599 so merging this and using it in the dotnet/performance repository should get everything unblocked.