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

Use the .NET 8 linker only for .NET 8 #16125

Closed
rolfbjarne opened this issue Sep 26, 2022 · 2 comments
Closed

Use the .NET 8 linker only for .NET 8 #16125

rolfbjarne opened this issue Sep 26, 2022 · 2 comments
Labels
dotnet An issue or pull request related to .NET (6) dotnet-external-dependency .NET: this issue/pull request is blocked on external work dotnet-pri0 .NET 6: required for stable release
Milestone

Comments

@rolfbjarne
Copy link
Member

Currently .NET's linker is implemented so that if you're using .NET 7 to build a project, you'll get the .NET 7 linker, even if the project is building for .NET 6 (say using the net6.0-ios TargetFramework).

This is unfortunate for a couple of reasons:

  • The .NET 6 codebase (sdk, ref, runtime packs) was not tested with the .NET 7 linker (we'd need a time machine for that). Testing the .NET 7 linker with .NET 6 projects is more feasible, except that it increases our testing matrix quite a lot (will we have to test the .NET 8 linker with .NET 6 + .NET 7 + .NET 8 projects?) In any case our tests won't cover every scenario, and it's entirely possible that customers will run into issues with the .NET 7 linker that isn't covered by our tests.
  • In order to support building from Windows, we poke into the internals of the linker's MSBuild logic:
    <!-- Overrides Core SDK target to remote the ILLink execution -->
    <!-- https://github.com/dotnet/sdk/blob/cdf57465e1cba9e44b5c9a76a403d41b1b8f178c/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets#L76-L132 -->
    <Target Name="_RunILLink"
    DependsOnTargets="_ComputeManagedAssemblyToLink;PrepareForILLink"
    Inputs="$(MSBuildAllProjects);@(ManagedAssemblyToLink);@(TrimmerRootDescriptor);@(ReferencePath)"
    Outputs="$(_LinkSemaphore)">
    <PropertyGroup>
    <!-- We need to use netX.Y because when building from VS it sets MSBuildRuntimeType to Core and will pick net472 (which doesn't contain the illink assembly) -->
    <_RemoteILLinkPath>$(ILLinkTasksAssembly.Replace('$(NetCoreRoot)', '$(_DotNetRootRemoteDirectory)').Replace('net472', 'net$(BundledNETCoreAppTargetFrameworkVersion)').Replace('$([System.IO.Path]::GetFileName('$(ILLinkTasksAssembly)'))', 'illink.dll'))</_RemoteILLinkPath>
    </PropertyGroup>
    <!-- Include Debug symbols as input so those are copied to the server -->
    <ItemGroup>
    <!-- @(_PDBToLink) comes from the _ComputeManagedAssemblyToLink target, which is a dependency of this target and is included in Microsoft.NET.ILLink.targets -->
    <!-- @(_PDBToLink) should contain the current assembly pdb and the project reference pdbs -->
    <_ILLinkDebugSymbols Include="@(_PDBToLink)" />
    </ItemGroup>
    <Delete SessionId="$(BuildSessionId)" Files="@(_LinkedResolvedFileToPublishCandidate)" />
    <Xamarin.iOS.Tasks.ILLink
    SessionId="$(BuildSessionId)"
    AssemblyPaths="@(ManagedAssemblyToLink)"
    ReferenceAssemblyPaths="@(ReferencePath)"
    RootAssemblyNames="@(TrimmerRootAssembly)"
    TrimMode="$(TrimMode)"
    DefaultAction="$(TrimmerDefaultAction)"
    RemoveSymbols="$(TrimmerRemoveSymbols)"
    FeatureSettings="@(_TrimmerFeatureSettings)"
    CustomData="@(_TrimmerCustomData)"
    BeforeFieldInit="$(_TrimmerBeforeFieldInit)"
    OverrideRemoval="$(_TrimmerOverrideRemoval)"
    UnreachableBodies="$(_TrimmerUnreachableBodies)"
    UnusedInterfaces="$(_TrimmerUnusedInterfaces)"
    IPConstProp="$(_TrimmerIPConstProp)"
    Sealer="$(_TrimmerSealer)"
    Warn="$(ILLinkWarningLevel)"
    NoWarn="$(NoWarn)"
    TreatWarningsAsErrors="$(ILLinkTreatWarningsAsErrors)"
    WarningsAsErrors="$(WarningsAsErrors)"
    WarningsNotAsErrors="$(WarningsNotAsErrors)"
    SingleWarn="$(TrimmerSingleWarn)"
    CustomSteps="@(_TrimmerCustomSteps)"
    RootDescriptorFiles="@(TrimmerRootDescriptor)"
    OutputDirectory="$(IntermediateLinkDir)"
    DumpDependencies="$(_TrimmerDumpDependencies)"
    ExtraArgs="$(_ExtraTrimmerArgs)"
    ToolExe="$(_DotNetHostFileName)"
    ToolPath="$(_DotNetHostDirectory)"
    ILLinkPath="$(_RemoteILLinkPath)"
    LinkerItemsDirectory="$(_LinkerItemsDirectory)"
    DebugSymbols="@(_ILLinkDebugSymbols)"
    ContinueOnError="ErrorAndContinue">
    <Output TaskParameter="ExitCode" PropertyName="_ILLinkExitCode" />
    <Output TaskParameter="LinkedItems" ItemName="_XamarinLinkedItems" />
    <Output TaskParameter="LinkerOutputItems" ItemName="_XamarinLinkerItems" />
    </Xamarin.iOS.Tasks.ILLink>
    <Touch Files="$(_LinkSemaphore)" AlwaysCreate="true" Condition=" '$(_ILLinkExitCode)' == '0' " />
    </Target>

    This leads to bugs when the linker's internals changes in .NET 7, and we're still using poking logic from .NET 6 (because we still haven't invented a time machine). Example: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047?src=WorkItemMention&src-action=artifact_link

I believe Android found an issue with the .NET 7 linker as well (iirc Cecil fixed a bug that turned out to not be backwards bug-compatible).

The fix seems easy: we should look into whether the linker can be used from the target .NET version (i.e. when building a net6.0-ios app use the .NET 6 linker). This is something the linker/runtime team should do, so this issue is just a tracking issue for our side.

@rolfbjarne rolfbjarne added the dotnet An issue or pull request related to .NET (6) label Sep 26, 2022
@rolfbjarne rolfbjarne added this to the .NET 8 milestone Sep 26, 2022
@rolfbjarne rolfbjarne added the dotnet-pri0 .NET 6: required for stable release label Sep 26, 2022
@rolfbjarne rolfbjarne changed the title Use the .NET 7 linker only for .NET 7 Use the .NET 8 linker only for .NET 8 Sep 26, 2022
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue Sep 26, 2022
… isn't set.

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for .NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so we'll
have to wait a bit to see what happens then.

Ref: xamarin#16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Sep 26, 2022
… isn't set.

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for .NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so we'll
have to wait a bit to see what happens then.

Ref: xamarin#16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.
vs-mobiletools-engineering-service2 pushed a commit to vs-mobiletools-engineering-service2/xamarin-macios that referenced this issue Sep 26, 2022
… isn't set.

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for .NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so we'll
have to wait a bit to see what happens then.

Ref: xamarin#16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.
dalexsoto pushed a commit that referenced this issue Sep 26, 2022
…tAction isn't set. (#16130)

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it
will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for
.NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so
we'll
have to wait a bit to see what happens then.

Ref: #16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.


Backport of #16126

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
rolfbjarne added a commit that referenced this issue Sep 27, 2022
… isn't set. (#16126)

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for .NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so we'll
have to wait a bit to see what happens then.

Ref: #16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.
rolfbjarne added a commit that referenced this issue Sep 27, 2022
…merDefaultAction isn't set. (#16129)

The .NET 7 linker targets will set _TrimmerDefaultAction instead of
TrimmerDefaultAction, so if TrimmerDefaultAction isn't set (which it
will be
for .NET 6), then use _TrimmerDefaultAction (which should be set for
.NET 7).

Unfortunately for .NET 8 it seems I've misplaced my crystal ball, so
we'll
have to wait a bit to see what happens then.

Ref: #16125.

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1621047.

Backport of #16126

Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@marek-safar
Copy link
Contributor

This should be address in linker with dotnet/linker#3029

@rolfbjarne rolfbjarne added the dotnet-external-dependency .NET: this issue/pull request is blocked on external work label Oct 13, 2022
@rolfbjarne
Copy link
Member Author

This has been addressed in the linker.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
dotnet An issue or pull request related to .NET (6) dotnet-external-dependency .NET: this issue/pull request is blocked on external work dotnet-pri0 .NET 6: required for stable release
Projects
None yet
Development

No branches or pull requests

2 participants