Add ILLink targets and tests #3125

sbomer · 2019-04-12T20:30:59Z

This adds linker targets, mostly adapted from https://github.com/mono/linker/blob/9766e2b2406a3d44cbf05e49ab2bd19b535e3e5a/src/ILLink.Tasks/ILLink.Tasks.targets, to run the linker after ComputeFilesToPublish.

A few things still need to change (but I wanted to get what I have so far out for review):

~~Change property names to agreed-upon strings~~
~~Update deps file generation in response to Rewrite deps file generation #3086. I think this needs Merge release/3.0.1xx to master #3113 or another update PR to be merged.~~
~~Update the linker:~~
- ~~to include UsingTask for other tasks in the assembly (namely, ComputeManagedAssemblies)~~
- ~~for ReferencePath support~~
- ~~with a net472 build of the task dll~~

~~When I run these tests locally with a recent version of the linker, the checks pass up to the deps file asserts.~~
All checks are passing.

@nguerrera, @fadimounir, @swaroop-sridhar PTAL

swaroop-sridhar · 2019-04-12T20:45:29Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

+    -->
+  <Target Name="ILTransform"
+          Condition="'$(_TargetFrameworkVersionWithoutV)' >= '3.0' And '$(TargetFrameworkIdentifier)' == '.NETCoreApp'"
+          DependsOnTargets="$(ILTransformDependsOn)" />


Why is this only for .NetCoreApp? How about, if there are whole unused IL files that are not reachable from any public entrypoints in a .netstandard dll? (ex: partial classes) ?

I think you're right that it would make sense for netstandard, and the linker behavior is probably similar to the portable app case.

I think that since linking happens during publish, the normal workflow of packaging a netstandard library would not run the linker, and it would take some work by library authors to package a linked library. I agree that it makes sense not to block it here.

swaroop-sridhar · 2019-04-12T21:11:10Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+          Condition=" '$(LinkDuringPublish)' == 'true' "
+          DependsOnTargets="RunILLink">
+
+    <!-- For now, use ResolvedFileToPublish as input/output. This


The linker works with ResolvedFileToPublish as input, and I think that's the way it should be for now.

But I'm curious why crossgen works differently. Crossgen targets run after _ComputeResolvedFilesToPublishTypes target, and processes/updates the files bifurcated into _ResolvedFileToPublishPreserveNewest and _ResolvedFileToPublishAlways items. However, the processing /input-outputs for the two lists there are similar. So, why doesn't crossgen similarly process ResolvedFileToPublish ?

Good question - @fadimounir can you answer this?

When I started implementing this, i was under the impression that I would need to add some logic to the crossgen task to distinguish between the 2 lists, and only compile things in the PreserveNewest if they were out of date.
Things have evolved since then, and thinking about it now, I believe we should be able to just make crossgen work with the 'ResolvedFileToPublish', before the split that '_ComputeResolvedFilesToPublishTypes' does.
I'll make that change. For the linker, i think this looks correct.
The linker does one operation with multiple input files. Crossgen does multiple operations, each with its own input file, so splitting the 'ResolvedFilesToPublish' by type doesn't make sense here i believe

swaroop-sridhar · 2019-04-12T21:36:55Z

Looks good.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

fadimounir · 2019-04-17T01:19:27Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+      <ResourceCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
+      <RuntimeCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
+      <RuntimeTargetsCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
+      <RuntimePackAsset Remove="@(_RemovedManagedAssemblies)" />


This is the kind of change that would be undesirable if we moved this targets file to the ILLink nuget package, because if the names here change, this would be breaking.
@nguerrera, how can we ensure that name changes here won't break other tools/SDKs that depend on this? CI tests good enough?

if other sdks to depend on it is expected, it should be public and follow backward compact rules

This is exactly why we are putting the targets portion in the SDK. The contract to the linker is pure, no global mutations. Yes, other tools might do this sort of thing, but we specifically cut defining a strict contract for such tools. The linker will be first class and any mutation of the build state will happen in dotnet/sdk. This + sufficient tests will ensure that we don't just break the linker as we used to when it was it's own nuget package. I want to make it possible to write these sort of things outside of the sdk, but it is a hard problem and specifically not one we are going to solve as part of this work.

fadimounir · 2019-04-17T01:23:14Z

Looks good overall, modulo the question regarding where the targets file should be, and any related side-effect.

For dotnet/sdk#3125. This most notably adds a net472 build of ILLink.Tasks, and modifies Sdk.props to import the correct build of the tasks, depending on the MSBuild runtime type. This makes it possible to use the linker tasks on desktop MSBuild once again. Regardless of the MSBuild runtime, illink.dll will run on .NET Core. We set 'rollForwardOnNoCandidateFx' to allow running on .NET Core 3 (even though illink.dl is built targeting netcoreapp2.0). This change removes many workarounds from the old packaging logic, as we can now use NuGet extension points and avoid using a custom .nuspec.

nguerrera · 2019-04-18T22:31:42Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+    <!-- Remove assemblies from inputs to GenerateDepsFile. See
+         https://github.com/dotnet/sdk/pull/3086. -->
+    <ItemGroup>
+      <_RemovedManagedAssemblies Include="@(_ManagedAssembliesToLink)" Condition="!Exists('$(IntermediateLinkDir)/%(Filename)%(Extension)')" />


Ensure $(IntermediateLinkDir) has trailing slash, and don't put the / in the condition or other uses of it plus a file. Look at how this is done for the other output paths.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

nguerrera · 2019-04-18T22:42:38Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+
+    <ItemGroup>
+      <_ManagedAssembliesToLink Remove="@(_ManagedResolvedFileToPublish->WithMetadataValue('AssetType', 'resources'))" />
+      <_LinkedResolvedFileToPublishCandidates Include="@(_ManagedAssembliesToLink->'$(IntermediateLinkDir)/%(Filename)%(Extension)')" />


Ensure trailing slashes and don't use slash. $(IntermediateLinkDir)%(Filename)%(Extension) where somewhere else there's a !HasTrailingSlash -> add trailing slash. Look at the other ouptut paths that have this pattern

nguerrera · 2019-04-18T22:48:44Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+      <!-- Also remove them from the old deps file generation logic,
+           until https://github.com/dotnet/sdk/issues/3098 is
+           fixed. -->
+      <_PublishConflictPackageFiles Include="@(_RemovedManagedAssemblies)" />


Switch to new logic based on linker being enabled as discussed.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

nguerrera · 2019-04-18T23:02:48Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets

@@ -175,6 +176,17 @@ Copyright (c) .NET Foundation. All rights reserved.

  </Target>

+  <!--
+    ============================================================
+                                        ILTransformForPublish


I'd rather not advertise an extension point, and treat that as a separate feature.

It feels weird that this is tied to the TFM as well. Also, publishing for .NETSTandard is very weird (I wish it didn't work), and there is no .NETStandard 3.0, the current version aligned with 3.0 timeframe is 2.1. I think we should limit to NETCoreApp 3.0+ alone. But I think this limit should be applied directly to the linker and not to some generic extensibility point.

I removed this, and made _ILLink a direct dependency of ComputeAndCopyFilesToPublishDirectory.

nguerrera · 2019-04-18T23:04:00Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+             RootAssemblyNames="@(IntermediateAssembly->'%(Filename)')"
+             RootDescriptorFiles="@(TrimmerRootDescriptors)"
+             OutputDirectory="$(IntermediateLinkDir)"
+             ExtraArgs="$(ExtraLinkerArgs) --skip-unresolved true" />


Should this be ExtraTrimmerArgs if the other public facing options are "Trimmer"?

Named it _ExtraTrimmerArgs. I don't want to advertise this as a public facing option, and am hoping we can stop using it in the future in favor of defaults that do the right thing. Taking a property here makes it easier to test and makes it easier for us to give users workarounds in case they encounter issues.

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

sbomer · 2019-04-19T18:19:14Z

@nguerrera I've responded to all of your feedback. Does it look good to merge? I'd like to get it in so that I can make follow-up changes independently.

Deps file trimming doesn't work on ProjectReferences, so this changes the referenced test project to be a PackageReference. We also disable use of the build dependency file when linking, as the publish dependency file needs to be generated based on what the linker decided to remove.

The current public-facing linker properties are now: - PublishTrimmed - TrimmerRootDescriptors The IL transform extension point has been named ILTransformForPublish, to make it clear that it only runs on publish. This also enables incremental build for project file changes, and lets the linker run on projects targeting .NETStandard.

- Ensure trailing slash in linker output directory - Disable old deps file generation logic - Don't support netstandard - ExtraLinkerArgs -> _ExtraTrimmerArgs - Remove ILTransformForPublish extension point

sbomer · 2019-04-19T22:02:18Z

Looks like the SDK uptake issue has been unblocked: #3153. Removing the workarounds from this PR.

nguerrera · 2019-04-19T23:01:07Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+    <_LinkSemaphore>$(IntermediateOutputPath)Link.semaphore</_LinkSemaphore>
+    <!-- Disable old deps file generation logic until
+         https://github.com/dotnet/sdk/issues/3098 is fixed. -->
+    <DepsFileGenerationMode>new</DepsFileGenerationMode>


I don't think you need this anymore if we can merge #3152 first

@nguerrera mind if I do that in a follow-up PR? I'd like to get this change in sooner rather than later.

nguerrera · 2019-04-19T23:39:32Z

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets

+  </PropertyGroup>
+
+  <ItemGroup>
+    <TrimmerRootDescriptors Include="$(TrimmerRootDescriptors)" />


Are these files? It is weird to have a "public" item and property of the same name. Is there a reason why the user wouldn't provide these directly as items?

Also, item names are generally singular (though it is not 100% consistent) so this is more of a nit.

They are files. In the OOB package we supported this to make it easy to test on the command-line. I'll get rid of this in a follow-up and we can iterate there.

sbomer · 2019-04-19T23:52:21Z

@nguerrera I know you're very busy atm - thank you for finding time to review this!

sbomer · 2019-04-20T00:30:20Z

@nguerrera please merge when you get a chance!

….7 (#3125) - Microsoft.DotNet.Cli.Runtime - 5.0.100-alpha1.19506.7

For dotnet/sdk#3125. This most notably adds a net472 build of ILLink.Tasks, and modifies Sdk.props to import the correct build of the tasks, depending on the MSBuild runtime type. This makes it possible to use the linker tasks on desktop MSBuild once again. Regardless of the MSBuild runtime, illink.dll will run on .NET Core. We set 'rollForwardOnNoCandidateFx' to allow running on .NET Core 3 (even though illink.dl is built targeting netcoreapp2.0). This change removes many workarounds from the old packaging logic, as we can now use NuGet extension points and avoid using a custom .nuspec. Commit migrated from dotnet/linker@010f98e

sbomer changed the title ~~Add ILlink targets and tests~~ Add ILLink targets and tests Apr 12, 2019

swaroop-sridhar reviewed Apr 12, 2019

View reviewed changes

sbomer mentioned this pull request Apr 15, 2019

Package linker for SDK dotnet/linker#532

Merged

sbomer force-pushed the illink branch 2 times, most recently from 4a9b55c to 3f83d19 Compare April 16, 2019 20:49

fadimounir reviewed Apr 17, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets Show resolved Hide resolved

fadimounir reviewed Apr 17, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets Outdated Show resolved Hide resolved

fadimounir reviewed Apr 17, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets Show resolved Hide resolved

fadimounir reviewed Apr 17, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets Show resolved Hide resolved

fadimounir reviewed Apr 17, 2019

View reviewed changes

swaroop-sridhar mentioned this pull request Apr 17, 2019

Publish to Single-File #3132

Merged

sbomer force-pushed the illink branch from bb85bec to 71fc49c Compare April 18, 2019 18:05

nguerrera reviewed Apr 18, 2019

View reviewed changes

sbomer force-pushed the illink branch from c9f3a5d to 326d1b5 Compare April 19, 2019 00:00

nguerrera reviewed Apr 19, 2019

View reviewed changes

src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets Outdated Show resolved Hide resolved

sbomer force-pushed the illink branch from 4bcba18 to 85ecdc0 Compare April 19, 2019 00:32

sbomer added 6 commits April 19, 2019 14:59

Add ILLink targets and tests

7470658

Support old deps generation logic

075b137

Respond to PR feedback

e475c44

- Ensure trailing slash in linker output directory - Disable old deps file generation logic - Don't support netstandard - ExtraLinkerArgs -> _ExtraTrimmerArgs - Remove ILTransformForPublish extension point

Remove left-over ILTransformForPublishDependsOn property

bdbe81c

sbomer force-pushed the illink branch from 85ecdc0 to bdbe81c Compare April 19, 2019 22:00

nguerrera reviewed Apr 19, 2019

View reviewed changes

nguerrera approved these changes Apr 19, 2019

View reviewed changes

nguerrera merged commit 2db4b66 into dotnet:master Apr 20, 2019

sbomer mentioned this pull request Apr 20, 2019

ILLink cleanup #3156

Merged

wli3 pushed a commit that referenced this pull request Feb 7, 2020

Update dependencies from https://github.com/dotnet/cli build 20191006…

b690435

….7 (#3125) - Microsoft.DotNet.Cli.Runtime - 5.0.100-alpha1.19506.7

sbomer mentioned this pull request Feb 15, 2024

Avoid double writes to PDB in NAOT publish dotnet/runtime#98342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ILLink targets and tests #3125

Add ILLink targets and tests #3125

sbomer commented Apr 12, 2019 •

edited

Loading

swaroop-sridhar Apr 12, 2019

sbomer Apr 12, 2019

sbomer Apr 15, 2019

swaroop-sridhar Apr 12, 2019

sbomer Apr 12, 2019

fadimounir Apr 17, 2019

swaroop-sridhar commented Apr 12, 2019

fadimounir Apr 17, 2019

wli3 Apr 17, 2019

nguerrera Apr 17, 2019

fadimounir commented Apr 17, 2019

nguerrera Apr 18, 2019

nguerrera Apr 18, 2019

nguerrera Apr 18, 2019

nguerrera Apr 18, 2019

sbomer Apr 18, 2019

nguerrera Apr 18, 2019

sbomer Apr 18, 2019

sbomer commented Apr 19, 2019

sbomer commented Apr 19, 2019

nguerrera Apr 19, 2019

sbomer Apr 19, 2019

nguerrera Apr 19, 2019

nguerrera Apr 19, 2019

sbomer Apr 19, 2019

sbomer commented Apr 19, 2019

sbomer commented Apr 20, 2019

Add ILLink targets and tests #3125

Add ILLink targets and tests #3125

Conversation

sbomer commented Apr 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fadimounir commented Apr 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer commented Apr 19, 2019

sbomer commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbomer commented Apr 19, 2019

sbomer commented Apr 20, 2019

sbomer commented Apr 12, 2019 •

edited

Loading