-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Pack symbols in SharedFx for libraries we redist from runtime #62429
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
Conversation
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.
Pull Request Overview
This PR enables explicit symbol packaging for libraries redistributed from the runtime by adding a PackageReference and new MSBuild ItemGroup entries to include relevant PDB files.
- Adds a PackageReference for "Microsoft.Internal.Runtime.AspNetCore.Transport"
- Introduces ItemGroups to include and conditionally package symbol files in both the runtime and composite projects
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.sfxproj | Inserts PackageReference and symbol inclusion logic for runtime symbols |
src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.Composite.sfxproj | Mirrors the runtime project changes to include symbol files in the composite project |
Name="%(Filename)" /> | ||
|
||
<_SymbolFilesToPackage Include="@(_TransportPdbsWithName)" | ||
Condition="'@(ExternalAspNetCoreAppReference->AnyHaveMetadataValue('Identity', '%(Name)'))' == 'true'" /> |
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.
Consider refactoring this duplicated symbol packaging block into a shared targets or props file to improve maintainability across projects.
Copilot uses AI. Check for mistakes.
Name="%(Filename)" /> | ||
|
||
<_SymbolFilesToPackage Include="@(_TransportPdbsWithName)" | ||
Condition="'@(ExternalAspNetCoreAppReference->AnyHaveMetadataValue('Identity', '%(Name)'))' == 'true'" /> |
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.
Consider refactoring this duplicated symbol packaging block into a shared targets or props file to improve maintainability across projects.
Copilot uses AI. Check for mistakes.
src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.Composite.sfxproj
Outdated
Show resolved
Hide resolved
src/Framework/App.Runtime/src/Microsoft.AspNetCore.App.Runtime.sfxproj
Outdated
Show resolved
Hide resolved
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.
LGTM to unblock the missing symbols but we should (ideally immediately) follow-up with only consuming the transport package and not the individual packages anymore.
Also let NuGet handle referencing and bringing the assets in. I didn't have to change a single LOC in windowsdesktop.
/backport to release/10.0-preview6 |
Started backporting to release/10.0-preview6: https://github.com/dotnet/aspnetcore/actions/runs/15835680419 |
Fixes #62165. Test build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2735990&view=results