Merge release changes into main #5275
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#### AI description (iteration 1) #### PR Classification Code cleanup and configuration update. #### PR Summary This pull request updates the release management process to use WIF (Workload Identity Federation) connection. - Modified `prepare-release.yml` to remove SAS token generation steps and update Azure subscription. - Updated `AcquireBuild.ps1` to remove SAS suffixes parameter and use Azure credentials for blob access.
- `src/SOS/Strike/clrma/managedanalysis.cpp`: Fixed issues with `GetClrDataProcess` and `QueryInterface` calls, ensuring proper handling and reference management of `m_clrData` and `m_sosDac`. Added error handling to prevent fallback to old SOS unstructured provider.
Add auth call to release
Code cleanup and configuration update. This pull request updates the release management process to use WIF (Workload Identity Federation) connection. - Modified `prepare-release.yml` to remove SAS token generation steps and update Azure subscription. - Updated `AcquireBuild.ps1` to remove SAS suffixes parameter and use Azure credentials for blob access.
Contributor
|
This seems to be missing the src\SOS\SOS.Hosting\RuntimeWrapper.cs verification changes https://dev.azure.com/dnceng/internal/_git/dotnet-diagnostics?path=/src/SOS/SOS.Hosting/RuntimeWrapper.cs&version=GBinternal/release/stable&line=474&lineEnd=475&lineStartColumn=1&lineEndColumn=1&lineStyle=plain&_a=contents and others. |
Member
Author
|
That's why the no-merge. It's missing one commit |
Add DAC cert check Calls the CLRMD signing and cert checking helper function before loading the DAC module. The check honors the Debugger.Settings.EngineInitialization.SecureLoadDotNetExtensions setting when running under windbg. Defaults to true under dotnet-dump on Windows, false on Linux/MacOS for dotnet-dump and lldb. TestHost (used by the Microsoft.Diagnostics.DebugServices.UnitTests, etc.) disables the signature check because all the test asset dumps used are preview 6.0 and not signed (that needs to be fixed but it is beyond the scope of this PR). ---- New feature This pull request introduces a new feature to check DAC certificate validity. - Added `GetSettings` method in `dbgengservices.cpp` to retrieve debugger settings. - Implemented `DacSignatureVerificationEnabled` property in `HostServices.cs` and `DebuggerServices.cs` to enforce DAC certificate validation. - Created new interface `ISettingsService` in `ISettingsService.cs` to define settings service. - Updated `RuntimeWrapper.cs` to verify DAC signing and certificate before loading. - Modified `Versions.props` to update `MicrosoftDiagnosticsRuntimeVersion` to `4.0.0-beta.24521.1`.
noahfalk
previously approved these changes
Feb 28, 2025
Contributor
|
All the tests are failing with the below error. I can't see any problems with the merge but this command fails. Technically the tests shouldn't need to set this/turn it off. |
Contributor
|
It is a new command line parsing problem. |
mikem8361
suggested changes
Mar 5, 2025
src/SOS/SOS.UnitTests/SOSRunner.cs
Outdated
| bool secureLoadDotNetExtensions = !(config.PrivateBuildTesting() || (setHostRuntime != null && setHostRuntime == "-none")); | ||
| initialCommands.Add($"dx @Debugger.Settings.EngineInitialization.SecureLoadDotNetExtensions={(secureLoadDotNetExtensions ? "true" : "false")}"); | ||
| bool shouldVerifyDacSignature = !config.PrivateBuildTesting() | ||
| && !"nightly".Equals(config.GetValue("BuildType"), StringComparison.OrdinalIgnoreCase) |
Contributor
There was a problem hiding this comment.
You should add an TestConfiguration extension method for BuildType at the end of this file.
src/SOS/SOS.UnitTests/SOSRunner.cs
Outdated
| initialCommands.Add($"runtimes --DacSignatureVerification:{(config.PrivateBuildTesting() || OS.Kind != OSKind.Windows ? "false" : "true")}"); | ||
| shouldVerifyDacSignature = OS.Kind == OSKind.Windows | ||
| && !config.PrivateBuildTesting() | ||
| && !"nightly".Equals(config.GetValue("BuildType"), StringComparison.OrdinalIgnoreCase); |
Contributor
There was a problem hiding this comment.
Use the extension method here.
| <BuildProjectFramework>$(TargetFrameworkLatest)</BuildProjectFramework> | ||
| <DotNetDiagnosticExtensions>$(RootBinDir)/bin/TestExtension/$(TargetConfiguration)/netstandard2.0/TestExtension.dll</DotNetDiagnosticExtensions> | ||
| <SetHostRuntime>$(DotNetRoot)/shared/Microsoft.NETCore.App/$(RuntimeFrameworkVersion)</SetHostRuntime> | ||
| <BuildType>Nightly</BuildType> |
Contributor
There was a problem hiding this comment.
I wish there was a better way to do this than putting in the config file. RuntimeVersionLast is setting to the latest/nightly version. You could put code in SOSRunner.cs to get that config value and compare it to the RuntimeFrameworkVersion config property
mikem8361
reviewed
Mar 5, 2025
| <RuntimeSymbolsPath>$(DotNetRoot)/shared/Microsoft.NETCore.App/$(RuntimeFrameworkVersion)</RuntimeSymbolsPath> | ||
| <LLDBHelperScript>$(ScriptRootDir)/lldbhelper.py</LLDBHelperScript> | ||
|
|
||
| <BuildType>nightly</BuildType> |
Contributor
There was a problem hiding this comment.
You have the condition switched compared to the similar logic in the Windows config.
mikem8361
approved these changes
Mar 5, 2025
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
prepare-release.ymlto remove SAS token generation steps and update Azure subscription. - UpdatedAcquireBuild.ps1to remove SAS suffixes parameter and use Azure credentials for blob access.