aspnetcore
aspnetcore copied to clipboard
dotnet
Update to net8.0 SDK
Blocked on getting an SDK from https://github.com/dotnet/installer/pull/15151
Here's a new one, will look tomorrow:
/Users/runner/work/1/s/.dotnet/sdk/8.0.100-alpha.1.23054.7/Sdks/Microsoft.NET.Sdk/targets/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. [/Users/runner/work/1/s/src/Servers/Connections.Abstractions/src/Microsoft.AspNetCore.Connections.Abstractions.csproj::TargetFramework=netstandard2.1]
What do you mean by disable trimming? Our projects are annotated to help trimming succeed, but that's it.
Trimming happens when publishing an app. Who is publishing?
Who is publishing?
I think our Helix tests publish. Not sure if they would be impacted by this.
Trimming happens when publishing an app. Who is publishing?
This error happens very early on, during framework reference resolution. Presumably the SDK is doing some scanning that sees we have PublishTrimmed=true for projects with a netstandard2.1 configuration, and errors out. I'm proposing setting that to false whenever TargetFramework=netstandard2.1
@sbomer looks like this was caused by https://github.com/dotnet/sdk/pull/29441 - is it intentional that we get these errors for netstandard2.1 projects that enable trimming?
https://github.com/dotnet/aspnetcore/pull/45879#issuecomment-1371754009
PublishTrimmed is incorrect for a netstandard2.1 library both because we don't support publishing self-contained trimmed libraries, and because trimming wasn't available until netcoreapp3.0.
@wtgodbe Is it definitely the case that PublishTrimmed is set, or could these projects have IsTrimmable or EnableTrimAnalyzer set instead? netstandard2.1 didn't have annotations for trimming, but the 7.0 SDK didn't block enabling the trim analyzer, and it was possible to get warnings by for example compiling a copy of RequiresUnreferencedCodeAttribute into user code.
@vitek-karas is this considered a supported scenario? If so, we might need to add a KnownILLinkPack for netstandard2.1, or factor out the build-only targets from the ILLink pack, to make this work with the approach in https://github.com/dotnet/sdk/pull/29441.
@wtgodbe Is it definitely the case that PublishTrimmed is set, or could these projects have IsTrimmable or EnableTrimAnalyzer set instead? netstandard2.1 didn't have annotations for trimming, but the 7.0 SDK didn't block enabling the trim analyzer, and it was possible to get warnings by for example compiling a copy of RequiresUnreferencedCodeAttribute into user code.
They set IsTrimmable. Maybe these projects could conditionally set PublishTrimmed=false when TargetFramework=netstandard2.1
https://github.com/dotnet/aspnetcore/blob/5b43ed1b8355c9fdebb44b0035d215a563435562/src/Servers/Connections.Abstractions/src/Microsoft.AspNetCore.Connections.Abstractions.csproj#L10
Setting IsTrimmable to false would prevent the library from being trimmed when consumed in apps, so it may not be what you want. I think our team needs to discuss the intended UX around this. For now, I can suggest a workaround:
<Target Name="_FixKnownILLinkPack"
BeforeTargets="ProcessFrameworkReferences">
<ItemGroup>
<KnownILLinkPack Include="@(KnownILLinkPack)"
Condition="'%(TargetFramework)' == 'net7.0'"
TargetFramework="netstandard2.1"
ILLinkPackVersion="%(KnownILLinkPack.ILLinkPackVersion)" />
</ItemGroup>
</Target>
This should keep it working as before (by adding a netstandard2.1 KnownILLinkPack matching the 7.0 version).
Setting IsTrimmable to false would prevent the library from being trimmed when consumed in apps, so it may not be what you want. I think our team needs to discuss the intended UX around this. For now, I can suggest a workaround:
I'm suggesting keeping IsTrimmable=true everywhere, but also adding PublishTrimmed=false when TFM=netstandard2.1. Would that not work? If not I'll try your suggested workaround
No, I think it will hit the same error because some of the logic for IsTrimmable is imported from the KnownILLinkPack.
Remaining errors are all:
##[error]src/Tools/LinkabilityChecker/LinkabilityChecker.csproj(47,5): error MSB4036: (NETCORE_ENGINEERING_TELEMETRY=Build) The "ILLink" task was not found. Check the following: 1.) The name of the task in the project file is the same as the name of the task class. 2.) The task class is "public" and implements the Microsoft.Build.Framework.ITask interface. 3.) The task is correctly declared with <UsingTask> in the project file, or in the *.tasks files located in the "/Users/runner/work/1/s/.dotnet/sdk/8.0.100-alpha.1.23055.1" directory.
@sbomer are you aware of any other recent linker changes that might have caused this to start happening?
The same change https://github.com/dotnet/sdk/pull/29441 removes ILLink as a bundled SDK - it now needs to be downloaded based on the TFM - but it looks like aspnetcore was relying on it being present in the SDK layout. I think someone from aspnetcore needs to look into this. Probably https://github.com/dotnet/aspnetcore/blob/main/src/Tools/LinkabilityChecker/LinkabilityChecker.csproj needs to set some properties to tell the SDK to download ILLink - I think it would suffice to add PublishTrimmed there (even if the project itself doesn't need to be published).
Removing ILLink sounds like a regression, especially if the SDK doesn't treat references to the SDK in the same way it does missing targeting packs. What's the mid-term story here❔
@dougbu It sounds like ILLink isn't in the SDK by default, but it is automatically downloaded when <PublishTrimmed>true</PublishTrimmed> is set on a project. What we're doing here, calling the ILLink task directly, is something people would rarely do and is an acceptable tradeoff for a smaller SDK size (I'm guessing that's why).
All we need to do is add <PublishTrimmed>true</PublishTrimmed> to WasmLinkerTest.csproj and LinkabilityChecker.csproj.
@sbomer Correct me if I'm wrong here.
f you want to avoid this for .NET 9 and onwards, consider setting DisableImplicitFrameworkReferences to true in RepoTasks.csproj as presumably that project doesn't need the framework references and the underlying targeting pack.
Thanks, testing that out pre-SDK update here: https://github.com/dotnet/aspnetcore/pull/45971
Don't think that'll work since our RepoTasks do use stuff from runtime. We can probably just apply the FrameworkReference workaround we give to non-Tools projects to RepoTasks to fix this going forward.
I don't think we need to fix this right now and would instead love to see this go in. Should we file an issue to track the follow-up work?
ILLink isn't in the SDK by default, but it is automatically downloaded when <PublishTrimmed>true</PublishTrimmed> is set on a project. [...] is an acceptable tradeoff for a smaller SDK size (I'm guessing that's why).
Just some additional context - the main reason we made the change is for versioning. We need to download a linker that matches the TFM, and it would bloat the SDK if we bundled a version of the linker for each supported TFM.
Hi @sbomer. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.
@wtgodbe this change broke official builds. See https://dev.azure.com/dnceng/internal/_build/results?buildId=2083576&view=results for example. Please immediately either revert this change or update the binlogtosln tool version we use.
I see, this did bring in the new SourceIndexPackageVersion from https://github.com/dotnet/arcade/issues/11995, but we overwrite that in ci.yml with an old version from 2021. We should probably just start using the default from Arcade. I'll open a PR now
Never mind, can't remove that because it's used directly before we get to eng/common. Opened https://github.com/dotnet/aspnetcore/pull/45982 to just update it