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

Fix illink task not to hard code values for one workload #10999

Open
marek-safar opened this issue Mar 25, 2020 · 4 comments
Open

Fix illink task not to hard code values for one workload #10999

marek-safar opened this issue Mar 25, 2020 · 4 comments

Comments

@marek-safar
Copy link

@marek-safar marek-safar commented Mar 25, 2020

Today there are hardcoded arguments passes to the link which are bound to one of the pre-existing workloads linker is used. This needs to be extracted so it's easier to ignore for other workloads.

<ILLink AssemblyPaths="@(_ManagedAssembliesToLink)"

@sbomer

@vitek-karas

This comment has been minimized.

Copy link
Member

@vitek-karas vitek-karas commented Mar 26, 2020

@sbomer

This comment has been minimized.

Copy link
Member

@sbomer sbomer commented Mar 26, 2020

@marek-safar how is this different from #11006?

@marek-safar

This comment has been minimized.

Copy link
Author

@marek-safar marek-safar commented Mar 26, 2020

It's different in the sense that here we hardcode specific linker behaviour which is not good. One way to fix it (I am not msbuild expert) is to introduce PreLink target (which any workload can override) and do it there. What will happen in PreLink will be setting of public properties defined in #11006

@sbomer

This comment has been minimized.

Copy link
Member

@sbomer sbomer commented Mar 27, 2020

Existing customization options:

  • The SDK was set up so thatResolvedFileToPublish has optional metadata PostprocessAssembly which tells publish-time tools to process them. This includes the linker and R2R. So SDK components should make sure that they correctly set PostprocessAssembly, and these will flow to the linker if the linker is enabled. These inputs are passed to the task's AssemblyPaths, which is effectively -reference.
  • ResolvedFileToPublish has optional IsTrimmable and action metadata. The default is to treat them as not trimmable - which causes them to be rooted (-a) and to have a copy action. To opt more assemblies into trimming, SDK components can set IsTrimmable on the inputs to avoid rooting them or defaulting their action to 'copy'. They can then optionally use action to specify the action per assembly. Any without action metadata will use the default action.

The current logic always set the default action to copyused. It can be overridden by setting _ExtraTrimmerArgs, but that's not ideal. I think a good pattern to adopt would be:

  • Expose DefaultAction on the task, with an MSBuild property _TrimmerDefaultAction (with an underscore because I am assuming we don't want this to be user-facing).
  • The core SDK will set the default to copyused (or something else if we want to change it):
<_TrimmerDefaultAction Condition="'$(_TrimmerDefaultAction)' == ''">copyused</_TrimmerDefaultAction>

This is done late in the evaluation process, when _SetILLinkDefaults runs.

  • Other SDK components can change the default action simply by setting this property. The winner is determined by the property import order - so components will need to cooperate to make sure they don't clobber each other's settings.

With this behavior, the work here would be to:

  • Expose DefaultAction input on the task
  • In the SDK, set _TrimmerDefaultAction to a default if not already set, and pass it to the task.

We can do the same for any other simple boolean/string properties that we want to customize. For custom steps, it would instead be an itemgroup (empty by default) that compenents can add to. How does that sound?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.