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.

Sign up for GitHub

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

Jump to bottom

PublishTrimmed should use a linker that matches the TargetFramework #3029

Closed
sbomer opened this issue Sep 8, 2022 · 3 comments · Fixed by dotnet/sdk#29441
Closed

PublishTrimmed should use a linker that matches the TargetFramework #3029

sbomer opened this issue Sep 8, 2022 · 3 comments · Fixed by dotnet/sdk#29441
Assignees
@sbomer
Milestone
8.0.0

Comments

@sbomer
Copy link
Member

sbomer commented Sep 8, 2022

We need a long-term solution for back-compat issues when using a newer SDK to trim apps targeting older TFMs. Some issues encountered so far:

We will hit a similar situation with planned breaking changes in 8.0, where it would be difficult to maintain a back-compat path. Maybe there are more examples, but this one comes to mind:

The solution will most likely (pending the outcome of broader discussion) be to use the 6.0 linker when targeting .net6, etc. This issue tracks the technical side of that proposed solution.
@sbomer sbomer added this to the .NET 8.0 milestone Sep 8, 2022
@sbomer sbomer self-assigned this Sep 8, 2022
@LakshanF LakshanF mentioned this issue Sep 12, 2022
Closed
@sbomer
Copy link
Member Author

sbomer commented Sep 14, 2022

The current plan is to add an implicit PackageReference in the SDK targets, which would use the standard mechanism for importing targets from NuGet packages.

We could take the version of the PackageReference from a new KnownILLinkPack that we would add to the bundled versions. KnownILLinkPack would completely bypass the logic in ProcessFrameworkReferences that is used for other "runtime packs" like crossgen. This does mean that we would need to duplicate some logic from ProcessFrameworkReferences (into MSBuild .targets) if we want the linker version to respect RuntimeFrameworkVersion and similar properties. For reference, this property is respected by crossgen.

The KnownILLinkPack version should probably be the same as the other runtime packs like crossgen. This version matches the runtime pack, so it would be easier to do this once the linker has migrated into dotnet/runtime and can share the version number. But we probably need to manually update the version number for the 7.0 linker package which does not version with dotnet/runtime.

As for the behavior when targeting net6.0 or lower, my plan is to continue using the 7.0 linker going forward, because that is the configuration we are planning to ship in .NET 7. But if we get negative feedback from this, we could instead use the 6.0 linker. @vitek-karas any thoughts on this?

@vitek-karas
Copy link
Member

I think it makes sense to keep 7.0 as-is, unless we get lot of feedback otherwise.

@jtschuster
Copy link
Member

Leaving a note for once this is finalized to check on the linker benchmark in dotnet/performance and make sure that the benchmark will still use the latest version of the linker.

@marek-safar marek-safar mentioned this issue Oct 12, 2022
Closed
This was referenced Oct 19, 2022
Closed
Closed
This was referenced Dec 8, 2022
Merged
Merged
sbomer added a commit to dotnet/installer that referenced this issue Dec 21, 2022
@sbomer
Add KnownILLinkPack to bundled versions (#15106)
fe452cf 
Part of dotnet/linker#3029. With
https://github.com/dotnet/sdk/pull/29441/files, this will enable the
SDK to use a different version of illink depending on the TFM.

The latest 7.0 illink package is used when trimming net7.0 and
earlier, which matches the configuration we shipped in the .NET 7 SDK.

- The 7.0 version is taken from the latest 7.0 SDK branch:
   https://github.com/dotnet/sdk/blob/release/7.0.2xx/eng/Versions.props#L89.
- The 8.0 version is the latest in the SDK's main branch:
   https://github.com/dotnet/sdk/blob/main/eng/Versions.props#L88

These versions will quickly get out of date and need to be kept
updated. For the latest (8.0) version it would be possible to
set up dependency flow from linker -> installer to automate this, but
this will not be necessary after the linker move to dotnet/runtime, which will
allow us to use $(MicrosoftNETCoreAppRuntimePackageVersion) for the
latest illink pack.

Unfortunately the 7.0 version will still need manual updates. We might
want to consider changing our version numbers to match dotnet/runtime,
but AFAIK that will still be a manual process (to update the patch
number in the release/7.0 branch of dotnet/linker every SDK servicing
release).
dotnet-bot pushed a commit to dotnet/dotnet that referenced this issue Dec 21, 2022
@sbomer @dotnet-maestro
[installer] Sync fe452cf: Add KnownILLinkPack to bundled versions (#1…
afda671 
…5106)

Part of dotnet/linker#3029. With
https://github.com/dotnet/sdk/pull/29441/files, this will enable the
SDK to use a different version of illink depending on the TFM.

The latest 7.0 illink package is used when trimming net7.0 and
earlier, which matches the configuration we shipped in the .NET 7 SDK.

- The 7.0 version is taken from the latest 7.0 SDK branch:
   https://github.com/dotnet/sdk/blob/release/7.0.2xx/eng/Versions.props#L89.
- The 8.0 version is the latest in the SDK's main branch:
   https://github.com/dotnet/sdk/blob/main/eng/Versions.props#L88

These versions will quickly get out of date and need to be kept
updated. For the latest (8.0) version it would be possible to
set up dependency flow from linker -> installer to automate this, but
this will not be necessary after the linker move to dotnet/runtime, which will
allow us to use $(MicrosoftNETCoreAppRuntimePackageVersion) for the
latest illink pack.

Unfortunately the 7.0 version will still need manual updates. We might
want to consider changing our version numbers to match dotnet/runtime,
but AFAIK that will still be a manual process (to update the patch
number in the release/7.0 branch of dotnet/linker every SDK servicing
release).

Original commit: dotnet/installer@fe452cf

[[ commit created by automation ]]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@sbomer sbomer

Labels
None yet
Projects
AppModel
Archived in project
Milestone
8.0.0
Development

Successfully merging a pull request may close this issue.

Use linker matching TFM sbomer/sdk
3 participants
@sbomer @vitek-karas @jtschuster