Skip to content

Conversation

@agocke
Copy link
Member

@agocke agocke commented Jun 14, 2022

In general, a task invocation in MSBuild and the task definition are
inseparable. Adding parameters to the task will either result in errors
if the target is updated and too new, or silently skipping the parameter
if the target is too old.

Best practice is to ship both as a pair to prevent this problem. Testing is
harder, but we can add some limited testing if necessary to the linker repo.

In general, a task invocation in MSBuild and the task definition are
inseparable. Adding parameters to the task will either result in errors
if the target is updated and too new, or silently skipping the parameter
if the target is too old.

Best practice is to ship both as a pair to prevent this problem. Testing is
harder, but we can add some limited testing if necessary to the linker repo.
@agocke agocke requested a review from sbomer as a code owner June 14, 2022 00:06
@agocke
Copy link
Member Author

agocke commented Jun 14, 2022

Companion SDK change is dotnet/sdk#25993

@@ -0,0 +1,365 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also rename this file to Microsoft.NET.ILLink.Tasks.targets to match the props?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would cause it to be imported automatically, which is not what I think we want (I think we need to control the targets import order relative to other targets in publish).

I'm not sure how best to communicate the naming decisions here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is that when it's referenced as a nuget package, the props are imported per normal nuget semantics, but the targets are imported in the order defined by the SDK?

That would make sense to me, maybe just leave a comment in the header here to explain this.

@agocke agocke merged commit e6685e5 into dotnet:main Jun 14, 2022
@agocke agocke deleted the add-illink-targets branch June 14, 2022 23:04
agocke added a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
…linker#2837)

In general, a task invocation in MSBuild and the task definition are
inseparable. Adding parameters to the task will either result in errors
if the target is updated and too new, or silently skipping the parameter
if the target is too old.

Best practice is to ship both as a pair to prevent this problem. Testing is
harder, but we can add some limited testing if necessary to the linker repo.

Commit migrated from dotnet/linker@e6685e5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants