Dipping toes into linker friendliness #24458

davidfowl · 2020-07-31T05:15:20Z

Experimenting with linker friendliness in ASP.NET Core. This investigation did lead to a single API change around the new overload of UseStartup that takes a func (it was changed a generic).

Annotate a couple of methods that do registration specifying a type or T
UseMiddleware
UseStartup
UseHub
MapHub

Here's an example of what this looks like today in aggressive mode https://github.com/rynowak/link-a-thon/blob/master/src/ApiTemplate/Linker.xml

JamesNK

To properly certify asp.net core as linker friendly I think we need functional tests that run with linking enabled. This seems like an easy thing to unknowingly regress.

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs

src/Hosting/Hosting/src/GenericHost/GenericWebHostBuilder.cs

JamesNK · 2020-07-31T05:47:49Z

src/SignalR/server/Core/src/SignalRConnectionBuilderExtensions.cs

        /// <summary>
        /// Configure the connection to host the specified <see cref="Hub"/> type.
        /// </summary>
        /// <typeparam name="THub">The <see cref="Hub"/> type to host on the connection.</typeparam>
        /// <param name="connectionBuilder">The connection to configure.</param>
        /// <returns>The same instance of the <see cref="IConnectionBuilder"/> for chaining.</returns>
-        public static IConnectionBuilder UseHub<THub>(this IConnectionBuilder connectionBuilder) where THub : Hub
+        public static IConnectionBuilder UseHub<[DynamicallyAccessedMembers(HubAccessibility)]THub>(this IConnectionBuilder connectionBuilder) where THub : Hub


Hmm, this should be done in gRPC.

https://github.com/rynowak/link-a-thon/blob/master/src/GrpcService/Linker.xml

davidfowl · 2020-07-31T05:58:50Z

I think to properly certify asp.net core as linker friendly I think we need functional tests that run with linking enabled. This seems like an easy thing to unknowingly regress.

2 things:

I agree in the fullness of time we'll need ways to test this so we don't regress behavior
Aggressive mode isn't a default linker option and it's non-trivial to turn on so the stakes are super low.

This doesn't change any defaults but it allows us to make progress in understanding how to make the framework more linkable.

davidfowl · 2020-08-02T04:04:47Z

UseMiddleware is the most critical because we use it on our own components. The rest is user code which we would probably avoid trimming by default.

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs

src/Hosting/Hosting/test/WebHostBuilderTests.cs

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs

src/Hosting/Hosting/src/Internal/StartupLinkerOptions.cs

eerhardt

These changes look good to me. Nice work.

Do you plan on having tests or analysis turned on in your build to ensure these don't get broken in the future?

davidfowl · 2020-08-02T22:10:27Z

@eerhardt trying to decide if I should add a single test in our end to end tests that already publish bits instead of an individual tests.

eerhardt · 2020-08-02T22:15:12Z

trying to decide if I should add a single test in our end to end tests that already publish bits instead of an individual tests.

In a perfect world - I'd think both. But if you are choosing one, I'd choose end to end.

- The linker.xml preserves constructors all of the types that are registered in DI. This should be removed once we get the linker friendly DI changes.

davidfowl · 2020-08-03T02:41:53Z

OK added a test. It uses a Linker.xml file to keep constructors of types in DI. Most of this file should be gone once we get dotnet/runtime#40227. The last thing looks like options dotnet/runtime#40236.

- Add Restore option to DeploymentParameters

marek-safar · 2020-08-03T12:03:26Z

@davidfowl it might be useful to enable linker warnings which would print if there are more cases to be fixed

eerhardt · 2020-08-03T13:43:36Z

it might be useful to enable linker warnings which would print if there are more cases to be fixed

For library authors (like ASP.NET here - and any library in dotnet/runtime that isn't part of the runtimepack) it would be useful to have an official linker option that runs the analysis/warnings on the whole library using the public surface area as the roots (like -r assembly does today).

That way library authors can easily tell which parts of their library aren't linker safe, and need to be fixed.

davidfowl · 2020-08-03T13:54:51Z

I'll submit a follow up PR with any remaining warnings. I really want to try this with @eerhardt 's DI changes to see how close we are to no annotations.

Tratcher · 2020-08-03T17:29:24Z

src/Hosting/Server.IntegrationTesting/src/ApplicationPublisher.cs

@@ -38,7 +38,7 @@ public virtual Task<PublishedApplication> Publish(DeploymentParameters deploymen
                                 // avoids triggering builds of dependencies of the test app which could cause issues like https://github.com/dotnet/arcade/issues/2941
                                 + $" --no-dependencies"
                                 + $" /p:TargetArchitecture={deploymentParameters.RuntimeArchitecture}"
-                                 + " --no-restore";
+                                 + (deploymentParameters.RestoreDependencies ? "" : " --no-restore");


I recall no-restore is required for all tests on Helix because the test assets need to be pre-built before publishing to helix.

Works fine on helix as far as I could tell.

- Annotated UseMiddleware and UseStartup to preserve constructors and public methods. - Annotated UseHub and MapHub preserve constructors and public methods. - Added a test to verify UseStartup and UseMiddleware works - The linker.xml preserves constructors all of the types that are registered in DI. This should be removed once we get the linker friendly DI changes.

Dipping toes into linkerfriendliness

c8456e8

davidfowl requested review from halter73, JamesNK, BrennanConroy and eerhardt July 31, 2020 05:15

davidfowl changed the title ~~Dipping toes into linkerfriendliness~~ Dipping toes into linker friendliness Jul 31, 2020

davidfowl requested a review from SteveSandersonMS July 31, 2020 05:15

JamesNK reviewed Jul 31, 2020

View reviewed changes

Fixed tests

fa0577c

Pilchie added area-hosting area-signalr Includes: SignalR clients and servers labels Jul 31, 2020

davidfowl marked this pull request as ready for review August 2, 2020 03:17

davidfowl requested review from jkotalik and Tratcher as code owners August 2, 2020 03:17

davidfowl mentioned this pull request Aug 2, 2020

The options subsystem needs linker attributes to preserve constructors on options types dotnet/runtime#40236

Closed

JamesNK approved these changes Aug 2, 2020

View reviewed changes

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs Outdated Show resolved Hide resolved

src/Hosting/Hosting/test/WebHostBuilderTests.cs Show resolved Hide resolved

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs Outdated Show resolved Hide resolved

Mark everything transitively

8c11b8d

eerhardt reviewed Aug 2, 2020

View reviewed changes

src/Hosting/Hosting/src/WebHostBuilderExtensions.cs Outdated Show resolved Hide resolved

eerhardt reviewed Aug 2, 2020

View reviewed changes

src/Hosting/Hosting/src/Internal/StartupLinkerOptions.cs Outdated Show resolved Hide resolved

Fixed usings and copy right

c69d711

eerhardt approved these changes Aug 2, 2020

View reviewed changes

Added a test to verify UseStartup and UseMiddleware works

c47a622

- The linker.xml preserves constructors all of the types that are registered in DI. This should be removed once we get the linker friendly DI changes.

davidfowl added 2 commits August 2, 2020 20:15

Skip OSX and Linux because we avoid restoring tests for other reasons

a3cf88e

Restore dependencies for linked application

603a8da

- Add Restore option to DeploymentParameters

Add retries

845af6c

davidfowl merged commit 404d817 into master Aug 3, 2020

davidfowl deleted the davidfowl/linker-friendly branch August 3, 2020 13:54

Tratcher reviewed Aug 3, 2020

View reviewed changes

davidfowl mentioned this pull request Aug 4, 2020

Fix some linker warnings, report others #24553

Merged

amcasey added area-hosting Includes Hosting area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 1, 2023

Dipping toes into linker friendliness #24458

Dipping toes into linker friendliness #24458

Uh oh!

Conversation

davidfowl commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK left a comment • 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!

Uh oh!

Uh oh!

JamesNK Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Jul 31, 2020

Uh oh!

davidfowl commented Aug 2, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 2, 2020

Uh oh!

eerhardt commented Aug 2, 2020

Uh oh!

davidfowl commented Aug 3, 2020

Uh oh!

marek-safar commented Aug 3, 2020

Uh oh!

eerhardt commented Aug 3, 2020

Uh oh!

davidfowl commented Aug 3, 2020

Uh oh!

Tratcher Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidfowl commented Jul 31, 2020 •

edited

Loading

JamesNK left a comment •

edited

Loading