Skip to content
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

Conversation

caaavik-msft
Copy link
Contributor

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.

@caaavik-msft
Copy link
Contributor Author

Pinging @lewing @am11 @timcassell as they were involved in the previous discussion

@am11
Copy link
Member

am11 commented Sep 23, 2024

LGTM, whatever unblocks dotnet/performance#4470 at this point and get the .NET 10 builds rolling. 😅

Comment on lines +111 to +131
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;
Copy link
Member

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.

Suggested change
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]));

Copy link
Member

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.

Copy link
Collaborator

@timcassell timcassell Sep 23, 2024

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.

Copy link
Member

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).

Copy link
Collaborator

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).

@timcassell timcassell merged commit 5fe0c78 into dotnet:master Sep 23, 2024
8 checks passed
@timcassell timcassell added this to the v0.14.1 milestone Sep 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants