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

Ignore internal .NET Framework assemblies in AssemblySymbolLoader #46059

Merged
merged 3 commits into from
Jan 17, 2025

Conversation

ViktorHofer
Copy link
Member

Resolves the false positive warnings in dotnet/aspnetcore#59853

@ViktorHofer ViktorHofer requested a review from a team as a code owner January 16, 2025 18:46
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jan 16, 2025
@ViktorHofer ViktorHofer requested a review from ericstj January 16, 2025 18:46
ViktorHofer added a commit to dotnet/aspnetcore that referenced this pull request Jan 16, 2025
The NoWarn can be removed when dotnet/sdk#46059 got merged and consumed with a new .NET SDK.
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

One minor comment, otherwise LGTM.

@@ -16,6 +16,22 @@ namespace Microsoft.DotNet.ApiSymbolExtensions
/// </summary>
public class AssemblySymbolLoader : IAssemblySymbolLoader
{
// This is a list of dangling .NET Framework internal assemblies that should never get loaded.
private static readonly HashSet<string> s_assembliesToIgnore = [
Copy link
Member

Choose a reason for hiding this comment

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

How did you obtain this list?

Copy link
Member Author

Choose a reason for hiding this comment

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

Eric gave me the list but the tool and the command that generated is C:\Program Files (x86)\Reference Assemblies\Microsoft\Framework\.NETFramework\v4.8>\tools\verifyClosure\verifyClosure.exe .\

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM - just a couple minor suggestions.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Jan 16, 2025

Unrelated but the hardcoded ".dll" extension in AssemblySymbolLoader is a bit concerning. There could be assembly references to files with other extensions as well, i.e. .exe. It's not common but there are cases. I.e. Arcade's RemoteExecutor is both an executable and a library and therefore has the .exe extension for .NET framework:

image

This isn't a problem with Package Validation as only .dll files are considered there anyway: #18165

@ViktorHofer ViktorHofer merged commit eeb86e9 into main Jan 17, 2025
34 of 38 checks passed
@ViktorHofer ViktorHofer deleted the IgnoreAssembliesInAssemblySymbolLoader branch January 17, 2025 13:31
wtgodbe pushed a commit to dotnet/aspnetcore that referenced this pull request Jan 17, 2025
* Update .NET SDK

Update .NET SDK to version 10.0.100-alpha.1.25063.8.

---
updated-dependencies:
- dependency-name: Microsoft.NET.Sdk
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Suppress CP1002 temporarily via NoWarn

The NoWarn can be removed when dotnet/sdk#46059 got merged and consumed with a new .NET SDK.

* Remove NoWarns completely

* Update SDK and dotnet tool versions

* Update SDK and tools version in global.json

* NoWarn CP1002 for MessagePack project

---------

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Viktor Hofer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants