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

.Net SDK Validation #2523

Merged
merged 8 commits into from
Mar 4, 2024
Merged

Conversation

MattFromRVA
Copy link
Contributor

Fixes #2199

@MattFromRVA MattFromRVA mentioned this pull request Feb 14, 2024
@MattFromRVA MattFromRVA marked this pull request as ready for review February 15, 2024 21:31
@MattFromRVA MattFromRVA marked this pull request as draft February 15, 2024 21:34
@MattFromRVA MattFromRVA marked this pull request as ready for review February 21, 2024 20:02
Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

MonoAotLLVMToolChain and CsProjClassicNetToolchain also need the validation.

…MonoAotLLVMToolChain and CsProjClassicNetToolchain
@timcassell
Copy link
Collaborator

Another thing you could do is integrate the Toolchain.IsCliPathInvalid into this validator.

Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

Almost there, few more comments.

@timcassell
Copy link
Collaborator

Tests are failing

Failed CanDisassembleAllMethodCalls(jit: LegacyJit, platform: X86, runtime: .NET Framework 4.6.2) [13 ms]
  Error Message:
   The "Summary" should have NOT "HasCriticalValidationErrors"
  Stack Trace:
     at BenchmarkDotNet.IntegrationTests.BenchmarkTestExecutor.CanExecute(Type type, IConfig config, Boolean fullValidation) in D:\a\BenchmarkDotNet\BenchmarkDotNet\tests\BenchmarkDotNet.IntegrationTests\BenchmarkTestExecutor.cs:line 70
   at BenchmarkDotNet.IntegrationTests.DisassemblyDiagnoserTests.CanDisassembleAllMethodCalls(Jit jit, Platform platform, Runtime runtime) in D:\a\BenchmarkDotNet\BenchmarkDotNet\tests\BenchmarkDotNet.IntegrationTests\DisassemblyDiagnoserTests.cs:line 101
  Standard Output Messages:
 // Validating benchmarks:
 // Benchmark WithCalls.Benchmark: Dry(Jit=LegacyJit, Platform=X86, Runtime=.NET Framework 4.6.2, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=1, WarmupCount=1) [someArgument=2147483647]
 //    * The required .NET Framework SDK version 4.6.2 for runtime moniker Net462 is not installed.

Maybe some faulty logic with the Framework sdk validation (maybe newer Framework sdks are backwards-compatible?).

@MattFromRVA
Copy link
Contributor Author

MattFromRVA commented Mar 4, 2024

Tests are failing

Failed CanDisassembleAllMethodCalls(jit: LegacyJit, platform: X86, runtime: .NET Framework 4.6.2) [13 ms]
  Error Message:
   The "Summary" should have NOT "HasCriticalValidationErrors"
  Stack Trace:
     at BenchmarkDotNet.IntegrationTests.BenchmarkTestExecutor.CanExecute(Type type, IConfig config, Boolean fullValidation) in D:\a\BenchmarkDotNet\BenchmarkDotNet\tests\BenchmarkDotNet.IntegrationTests\BenchmarkTestExecutor.cs:line 70
   at BenchmarkDotNet.IntegrationTests.DisassemblyDiagnoserTests.CanDisassembleAllMethodCalls(Jit jit, Platform platform, Runtime runtime) in D:\a\BenchmarkDotNet\BenchmarkDotNet\tests\BenchmarkDotNet.IntegrationTests\DisassemblyDiagnoserTests.cs:line 101
  Standard Output Messages:
 // Validating benchmarks:
 // Benchmark WithCalls.Benchmark: Dry(Jit=LegacyJit, Platform=X86, Runtime=.NET Framework 4.6.2, IterationCount=1, LaunchCount=1, RunStrategy=ColdStart, UnrollFactor=1, WarmupCount=1) [someArgument=2147483647]
 //    * The required .NET Framework SDK version 4.6.2 for runtime moniker Net462 is not installed.

Maybe some faulty logic with the Framework sdk validation (maybe newer Framework sdks are backwards-compatible?).

You are correct. Framework sdks are backwards-compatible and it only shows the latest in the registry. I have updated my code to reflect that.

Copy link
Collaborator

@timcassell timcassell left a comment

Choose a reason for hiding this comment

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

Thanks a lot @MattFromRVA!

@timcassell timcassell merged commit 59647c9 into dotnet:master Mar 4, 2024
8 checks passed
@timcassell timcassell added this to the v0.14.0 milestone Mar 4, 2024
@caaavik-msft
Copy link
Contributor

caaavik-msft commented Mar 5, 2024

This change causes wasm runs to fail (and potentially others I haven't tested) because not all runtime monikers are accounted for in GetSdkVersionFromMoniker. I have a PR that adds an integration test for wasm runs which reproduced this and I have included a fix for it by updating GetSdkVersionFromMoniker there. I think there are some other monikers missing such as the MonoAotLLVM?

@timcassell
Copy link
Collaborator

This change causes wasm runs to fail (and potentially others I haven't tested) because not all runtime monikers are accounted for in GetSdkVersionFromMoniker. I have a PR that adds an integration test for wasm runs which reproduced this and I have included a fix for it by updating GetSdkVersionFromMoniker there. I think there are some other monikers missing such as the MonoAotLLVM?

Oops. This is why we need those integration tests. If someone sends another PR with that fix, I can approve and merge (I can't merge my own PRs).

@caaavik-msft
Copy link
Contributor

PR up now: #2538

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deadlock when SDK is not installed
3 participants