Use linker matching TFM #29441
Use linker matching TFM #29441
Conversation
- Don't depend on ILLink (since the targets aren't always imported) - Set PublishTrimmed for PublishAot - Download KnownILLinkPack for PublishAot
Also use raw string literals while touching this
This reverts commit 824d8fd.
There was a problem hiding this comment.
Looks good, but I'm missing how the ILLink targets actually end up getting imported.
Also it looks like this means that ILLink will always need to be downloaded in a NuGet restore instead of being included in the SDK. That means the scenario won't work "offline" anymore. Probably this is OK, but I wanted to make sure that's expected.
src/Tasks/Microsoft.NET.Build.Tasks/ProcessFrameworkReferences.cs
Outdated
Show resolved
Hide resolved
| @@ -111,6 +112,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| DisableTransitiveFrameworkReferenceDownloads="$(DisableTransitiveFrameworkReferenceDownloads)" | |||
| KnownCrossgen2Packs="@(KnownCrossgen2Pack)" | |||
| KnownILCompilerPacks="@(KnownILCompilerPack)" | |||
| KnownILLinkPacks="@(KnownILLinkPack)" | |||
There was a problem hiding this comment.
Where is KnownILLinkPack consumed? Is it supposed to be converted to a PackageReference? How are the .props and .targets inside it (which I assume used to be in the Microsoft.NET.ILLink.Tasks SDK) imported?
There was a problem hiding this comment.
It gets converted to a PackageReference to import the targets: https://github.com/dotnet/sdk/pull/29441/files#diff-8178b4f8b3b09a85467ac9f786b31881d689383c6a339eb6541f5468d1f3bb2aR695 and
Co-authored-by: Daniel Plaisted <[email protected]>
Yes that's true - I think it's ok because in general we might need to restore a runtime pack for the self-contained publish anyway. I haven't looked at how offline self-contained publish behaves when targeting the host RID - does it reuse the bundled runtime bits? If so, maybe we could make this work offline by using a bundled ILLink only when targeting the host RID, but I'm not sure if it's worth the extra complexity. |
|
If we're always going to download ILLink - should we remove it from the SDK isntaller then? What's the point of having it there (or maybe I don't understand something). |
I believe this PR does remove it from being bundled in the SDK installer, by removing this line: <BundledSdk Include="Microsoft.NET.ILLink.Tasks" Version="$(MicrosoftNETILLinkTasksPackageVersion)" /> |
| @@ -1182,7 +1182,7 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.VisualBasic.targets" Condition="'$(Language)' == 'VB'" /> | |||
| <Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.FSharp.targets" Condition="'$(Language)' == 'F#'" /> | |||
| <Import Project="$(ILCompilerTargetsPath)" Condition="'$(PublishAot)' == 'true'"/> | |||
| <Import Project="$(ILLinkTargetsPath)" Condition="'$(Language)' != 'C++'" /> | |||
| <Import Project="$(ILLinkTargetsPath)" Condition="'$(ILLinkTargetsPath)' != '' And '$(Language)' != 'C++'" /> | |||
There was a problem hiding this comment.
@sbomer given that the nuget package is now always used, why do we still need this and not just make sure that the targets file gets imported from the nuget package automatically via the file naming convention?
| @@ -15,6 +15,10 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <DefaultCopyToPublishDirectoryMetadata Condition="'$(DefaultCopyToPublishDirectoryMetadata)' == ''">true</DefaultCopyToPublishDirectoryMetadata> | |||
| <_GetChildProjectCopyToPublishDirectoryItems Condition="'$(_GetChildProjectCopyToPublishDirectoryItems)' == ''">true</_GetChildProjectCopyToPublishDirectoryItems> | |||
| <IsPublishable Condition="'$(IsPublishable)' == ''">true</IsPublishable> | |||
| <!-- PublishAot depends on PublishTrimmed. This must be set early enough for the KnownILLinkPack to be restored. --> | |||
| <PublishTrimmed Condition="'$(PublishTrimmed)' == '' And '$(PublishAot)' == 'true'">true</PublishTrimmed> | |||
| <_IsTrimmingEnabled Condition="'$(_IsTrimmingEnabled)' == '' And ('$(PublishTrimmed)' == 'true' Or '$(IsTrimmable)' == 'true')">true</_IsTrimmingEnabled> | |||
There was a problem hiding this comment.
@sbomer dotnet/maui has a netstandard2.0 class library where $(IsTrimmable) is set to true, because it is safe to aggressively trim.
This causes the build error:
Microsoft.NET.Sdk.FrameworkReferenceResolution.targets(90,5): error NETSDK1195: Unable to optimize assemblies for size: a valid runtime package was not found. Either set the PublishTrimmed property to false, or use a supported target framework when publishing.
We had to turn off $(IsTrimmable) for the netstandard2.0 build (this assembly is used from an MSBuild/VS context, but trimming doesn't matter there).
Is this an expected case?
Is that a breaking change? If so, has it been documented? |
I don't think we consider it a breaking change. The change is that instead of being included directly in the .NET SDK, the linker will be downloaded during NuGet restore. That could break offline scenarios, but I don't think that's something we track. |
Up to .NET 7, the analyzers versioned with the SDK (so you would always get the latest analyzers). This allowed using the analyzers (and related options which enable the analyzers) even on older TFMs. #32045 changed to versioning the analyzers with the target framework, but still allowed use of the analyzers on target frameworks where the .NET 7 ILLink pack was made available for backwards compatibility. This change removes such backwards compat by treating the analyzers as unsupported for target frameworks before the framework was annotated with the relevant attributes. This increases the scope of the warning added in #29441 and #34077 to also warn for these unsupported TFMs. The first commit adds testcases to show the old behavior. The second commit implements the new behavior and shows the changes to these testcases. PublishTrimmed/PublishSingleFile/PublishAot used to enable the corresponding analyzers. With this change, we now need to avoid failing PublishTrimmed/PublishSingleFile for older TFMs that supported these publish options, but don't support the analyzers. This change handles that by only defaulting the `Enable*Analyzer` settings based on the publish settings for TFMs where the analyzers are known to be supported.
Use a linker based on the current TargetFramework. This relies on KnownILLinkPack and the implicit packagereference mechanism used also for NativeAot to restore the right version of illink. See the KnownILLinkPack definitions here: dotnet/installer#15106.
Fixes dotnet/linker#3029