search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.67k stars 4.58k forks source link

Eliminate duplication with shipping ILLink targets #31359

Open ericstj opened 4 years ago

ericstj commented 4 years ago

We should avoid defining our own ILLink targets and instead extend/configure the shipping ones. We still have a lot that is unique to corefx (trimming libraries not apps, diffing the result) but we can avoid bugs by sharing the invokation logic from the official targets. See https://github.com/dotnet/corefx/pull/42266

/cc @sbomer @ViktorHofer @eiriktsarpalis

ViktorHofer commented 2 years ago

@ericstj do you still think this is feasible given that we do extensive ILLink testing in sfx.proj and in oob.proj in addition to us running the linker in library mode over our compiled assemblies?

Since this issue was opened, the illink.targets logic grew as it now also handles suppression files and supports the above mentioned cases.

ghost commented 2 years ago

This issue has been marked needs-author-action and may be missing some important information.

ericstj commented 2 years ago

I'm still do not like maintaining a copy of the target. It's ok to have a targets file to contain all of our common customizations, but we should avoid duplicating the target itself. We don't keep a copy of the CoreCompile target. Our customizations should be seen as a signal of things users might want to control in their linker usage. @eerhardt @vitek-karas @sbomer how feasible would it be to remove the customization here and rely on in-build functionality. https://github.com/dotnet/runtime/blob/35f66ed7cb42a3fbcc2c56604c31f8a64dbe119b/eng/illink.targets

sbomer commented 2 years ago

I don't see any big problems with it at first glance. It will require a little care to make sure we don't accidentally pass some unwanted options for our use case. We use the linker in a configuration that we don't consider a customer scenario ("library-mode" trimming), and it's pretty different from what the SDK does, but I agree that we should see this as a signal of what users might want to do.