Add LinkabilityChecker tool #40342

JamesNK · 2022-02-21T22:38:03Z

Addresses #27384

Replicate https://github.com/dotnet/aspnetcore/tree/5760f2864016081a142642b417db17e5340a1d67/src/Components/WebAssembly/testassets/WasmLinkerTest

I've removed the wasm specific build attributes:

    <RuntimeIdentifier>browser-wasm</RuntimeIdentifier>
    <SelfContained>true</SelfContained>
    <UseMonoRuntime>true</UseMonoRuntime>
    <UsingMicrosoftNETSdkBlazorWebAssembly>true</UsingMicrosoftNETSdkBlazorWebAssembly>

For lack of anywhere else to put it, I've placed the new testassets project in Shared. @aspnet-build let me know if you'd like it moved somewhere else.

JamesNK · 2022-02-21T22:41:07Z

@pranavkm After removing the wasm/mono attributes the project generates an error:

   "C:\Development\Source\aspnetcore\src\Shared\test\testassets\LinkerTest\LinkerTest.csproj" (default target) (1:7) ->
   (ILLinkTrimProjects target) ->
     ILLink : error IL1009: Assembly reference 'System.Security.Permissions' could not be resolved. [C:\Development\Source\aspnetcore\src\Shared\test\testassets\LinkerTest\LinkerTest.csproj]
     ILLink : error IL1009: Assembly reference 'System.Threading.AccessControl' could not be resolved. [C:\Development\Source\aspnetcore\src\Shared\test\testassets\LinkerTest\LinkerTest.csproj]

There are no references to those projects in aspnetcore.

Any idea what is wrong? If not, who should I talk to?

davidfowl · 2022-02-22T02:25:57Z

You might want to also get rid of this test

aspnetcore/src/Hosting/test/FunctionalTests/LinkedApplicationTests.cs

Line 12 in c85baf8

public async Task LinkedApplicationWorks()

pranavkm · 2022-02-22T14:45:01Z

@JamesNK I'm gonna guess there's a reference to these assemblies in the graph somewhere (maybe a TypeForwardedTo) which requires this assemblies to be available to the trimmer. Adding these package reference appears to help here.

pranavkm · 2022-02-22T14:45:50Z

FYI - I force pushed to this branch after rebasing on main

wtgodbe

src/Shared seems reasonable to me

dougbu · 2022-02-22T17:38:31Z

For lack of anywhere else to put it, I've placed the new testassets project in Shared. @aspnet-build let me know if you'd like it moved somewhere else.

src/Shared seems reasonable to me

I don't feel strongly about this but what is "shared" about this project❔ Not quite understanding how it fits into this repo in the first place and where it might fit otherwise. I'd feel better if the project were in its own src/ directory than under Shared.

JamesNK · 2022-02-22T17:57:46Z

There isn't anything shared. Basically, this is a project that checks linking warnings in shipping projects. It will end up referencing most shipping projects. It's generic, but it isn't itself shared.

An alternative location I considered was /src/Tools. It is kind of a tool for checking for linker warnings and generating linker suppression.

JamesNK · 2022-02-22T18:15:34Z

FYI - I force pushed to this branch after rebasing on main

You're a ⭐

dougbu · 2022-02-22T18:57:32Z

For lack of anywhere else to put it, I've placed the new testassets project in Shared. @aspnet-build let me know if you'd like it moved somewhere else.

src/Shared seems reasonable to me

I don't feel strongly about this but what is "shared" about this project❔ Not quite understanding how it fits into this repo in the first place and where it might fit otherwise. I'd feel better if the project were in its own src/ directory than under Shared.

There isn't anything shared. Basically, this is a project that checks linking warnings in shipping projects. It will end up referencing most shipping projects. It's generic, but it isn't itself shared.

An alternative location I considered was /src/Tools. It is kind of a tool for checking for linker warnings and generating linker suppression.

src/Tools/ would work for me. Other options might be src/Testing/test/ or src/CheckLinkability/.

pranavkm · 2022-02-22T23:10:50Z

@JamesNK / @dougbu I was playing around with this change and came up with pranavkm@0a96a3d. The idea is to use ProjectReferenceProvider items to find projects that are part of the shared framework and are trimmable. This negates the need to maintain a separate list of trimmable projects.

Thoughts?

dougbu · 2022-02-23T01:03:22Z

I … came up with pranavkm@0a96a3d. The idea is to use ProjectReferenceProvider items to find projects that are part of the shared framework and are trimmable. This negates the need to maintain a separate list of trimmable projects.

The idea sounds reasonable though I suggest things may be simpler if the metadata were collected when we run CodeGen.proj rather than added in a new corner of our infrastructure and re-generated every time trimmability is tested. Put another way, I agree w/ the direction but suggest getting there by a slightly different route.

The targets in CodeGen.proj and the supporting bits at the bottom of ResolveReferences.targets could easily be extended to collect $(IsTrimmable) information. I leave it up to you whether you want to generate a brand new file in eng/ or add the information to one of eng/ProjectReferences.props or eng/SharedFramework.Local.props.

More generally, I'd like to be fairly sure we do not limit checking assemblies are trimmable to the shared framework projects. I could see us wanting to ensure something that ships only in a package is and remains trimmable.

JamesNK · 2022-02-23T01:27:26Z

@pranavkm I've moved the app to tools. Feel free to apply your changes.

I added a couple of projects, up until I hit one that generated trim warnings: Microsoft.AspNetCore.Http.Abstractions. I fixed some warnings, most are still around. I want to double-check the warnings show up in build.

JamesNK · 2022-02-23T01:29:17Z

src/Shared/PropertyHelper/PropertyHelper.cs

@@ -136,7 +137,8 @@ public void SetValue(object instance, object? value)
    /// <param name="typeInfo">The type info to extract property accessors for.</param>
    /// <returns>A cached array of all public properties of the specified type.
    /// </returns>
-    public static PropertyHelper[] GetProperties(TypeInfo typeInfo)
+    public static PropertyHelper[] GetProperties(


I think PropertyHelper in general is incompatible with trimming.

JamesNK · 2022-02-23T01:31:23Z

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

@@ -95,6 +95,7 @@ public RouteValueDictionary()
    /// property names are keys, and property values are the values, and copied into the dictionary.
    /// Only public instance non-index properties are considered.
    /// </remarks>
+    [RequiresUnreferencedCode("RouteValueDictionary uses reflection to generate the route values.")]
    public RouteValueDictionary(object? values)


I think we might want new public API ctors for RouteValueDictionary.

public RouteValueDictionary(object? values); +public RouteValueDictionary(IDictionary<string, object> values);

Copying values from a IDictionary is fine. Copying values via reflection from public properties seems incompatible with trimming.

pranavkm · 2022-02-23T19:00:40Z

@JamesNK lets do this in two passes - we can get this change in once we resolve the current failure and I'll send a PR to use @dougbu's suggestion to find projects that are interesting to test.

src/Tools/LinkabilityChecker/LinkabilityChecker.csproj

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs

BrennanConroy · 2022-02-23T20:51:24Z

src/Extensions/Features/src/Microsoft.Extensions.Features.csproj

@@ -12,7 +12,7 @@ Microsoft.AspNetCore.Http.Features.FeatureCollection
    <IsAspNetCoreApp>true</IsAspNetCoreApp>
    <GenerateDocumentationFile>true</GenerateDocumentationFile>
    <PackageTags>aspnetcore</PackageTags>
-    <Nullable>enable</Nullable>


Why is nullable being removed?

Nullable is now turned on by default for all projects. Removing because it's redundent.

src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj

…actions.csproj

JamesNK requested review from a team and Pilchie as code owners February 21, 2022 22:38

JamesNK requested a review from dougbu as a code owner February 22, 2022 14:41

JamesNK and others added 2 commits February 22, 2022 06:41

Add LinkerTest project

cfca2ca

Fixup

5b61a70

pranavkm force-pushed the jamesnk/linkertest branch from c942ae2 to 5b61a70 Compare February 22, 2022 14:44

Merge branch 'main' into jamesnk/linkertest

32791e3

wtgodbe approved these changes Feb 22, 2022

View reviewed changes

Pilchie added the area-runtime label Feb 22, 2022

Move project

a8f9c08

Move linker app to tools and add some more projects

8b6235a

JamesNK requested review from Tratcher, BrennanConroy and halter73 as code owners February 23, 2022 01:23

JamesNK commented Feb 23, 2022

View reviewed changes

Tratcher removed their request for review February 23, 2022 18:55

pranavkm reviewed Feb 23, 2022

View reviewed changes

src/Tools/LinkabilityChecker/LinkabilityChecker.csproj Outdated Show resolved Hide resolved

Update src/Tools/LinkabilityChecker/LinkabilityChecker.csproj

8eb4114

Merge branch 'main' into jamesnk/linkertest

645af5e

pranavkm approved these changes Feb 23, 2022

View reviewed changes

JamesNK commented Feb 23, 2022

View reviewed changes

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs Outdated Show resolved Hide resolved

src/Http/Http.Abstractions/src/Routing/RouteValueDictionary.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

136903d

pranavkm mentioned this pull request Feb 23, 2022

Code gen #40383

Closed

BrennanConroy reviewed Feb 23, 2022

View reviewed changes

JamesNK commented Feb 23, 2022

View reviewed changes

src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstractions.csproj Outdated Show resolved Hide resolved

Update src/Http/Http.Abstractions/src/Microsoft.AspNetCore.Http.Abstr…

ef45773

…actions.csproj

JamesNK changed the title ~~Add LinkerTest project~~ Add LinkabilityChecker tool Feb 23, 2022

JamesNK merged commit e0110ad into main Feb 24, 2022

JamesNK deleted the jamesnk/linkertest branch February 24, 2022 04:15

ghost added this to the 7.0-preview3 milestone Feb 24, 2022

JamesNK mentioned this pull request Apr 1, 2022

Add trimming guidance for developers #40902

Merged

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

Add LinkabilityChecker tool #40342

Add LinkabilityChecker tool #40342

Uh oh!

Conversation

JamesNK commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK commented Feb 21, 2022

Uh oh!

davidfowl commented Feb 22, 2022

Uh oh!

pranavkm commented Feb 22, 2022

Uh oh!

pranavkm commented Feb 22, 2022

Uh oh!

wtgodbe left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu commented Feb 22, 2022

Uh oh!

JamesNK commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK commented Feb 22, 2022

Uh oh!

dougbu commented Feb 22, 2022

Uh oh!

pranavkm commented Feb 22, 2022

Uh oh!

dougbu commented Feb 23, 2022

Uh oh!

JamesNK commented Feb 23, 2022

Uh oh!

JamesNK Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamesNK Feb 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pranavkm commented Feb 23, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

JamesNK Feb 23, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JamesNK commented Feb 21, 2022 •

edited

Loading

JamesNK commented Feb 22, 2022 •

edited

Loading

JamesNK Feb 23, 2022 •

edited

Loading

JamesNK Feb 23, 2022 •

edited

Loading