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

Add ILLink targets and tests #3125

Merged
merged 6 commits into from
Apr 20, 2019
Merged

Add ILLink targets and tests #3125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7470658
Add ILLink targets and tests
sbomer Apr 12, 2019
e3262fc
Enable and test deps file trimming
sbomer Apr 16, 2019
075b137
Support old deps generation logic
sbomer Apr 17, 2019
1c98c83
Update property names and respond to PR feedback
sbomer Apr 17, 2019
e475c44
Respond to PR feedback
sbomer Apr 18, 2019
bdbe81c
Remove left-over ILTransformForPublishDependsOn property
sbomer Apr 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Tasks/Microsoft.NET.Build.Tasks/Microsoft.NET.Build.Tasks.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
<Stage0SdkFile Include="$(_Stage0SdksFolder)\Microsoft.NET.Sdk.Web.ProjectSystem\**" SdkName="Microsoft.NET.Sdk.Web.ProjectSystem" />
<Stage0SdkFile Include="$(_Stage0SdksFolder)\Microsoft.NET.Sdk.Razor\**" SdkName="Microsoft.NET.Sdk.Razor" />
<Stage0SdkFile Include="$(_Stage0SdksFolder)\Microsoft.NET.Sdk.WindowsDesktop\**" SdkName="Microsoft.NET.Sdk.WindowsDesktop" />
<Stage0SdkFile Include="$(_Stage0SdksFolder)\ILLink.Tasks\**" SdkName="ILLink.Tasks" />
<LayoutFile Include="@(Stage0SdkFile)">
<TargetPath>..\%(Stage0SdkFile.SdkName)\%(Stage0SdkFile.RecursiveDir)%(Stage0SdkFile.Filename)%(Stage0SdkFile.Extension)</TargetPath>
</LayoutFile>
Expand Down
120 changes: 120 additions & 0 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
<!--
sbomer marked this conversation as resolved.
Show resolved Hide resolved
***********************************************************************************************
Microsoft.NET.ILLink.targets

WARNING: DO NOT MODIFY this file unless you are knowledgeable about MSBuild and have
created a backup copy. Incorrect changes to this file will make it
impossible to load or build your projects from the command-line or the IDE.

Copyright (c) .NET Foundation. All rights reserved.
***********************************************************************************************
-->
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">

<Import Project="Sdk.props" Sdk="ILLink.Tasks" />

<PropertyGroup>
<IntermediateLinkDir Condition=" '$(IntermediateLinkDir)' == '' ">$(IntermediateOutputPath)linked\</IntermediateLinkDir>
<IntermediateLinkDir Condition=" !HasTrailingSlash('$(IntermediateLinkDir)') ">$(IntermediateLinkDir)\</IntermediateLinkDir>
<!-- Used to enable incremental build for the linker target. -->
<_LinkSemaphore>$(IntermediateOutputPath)Link.semaphore</_LinkSemaphore>
<!-- Disable old deps file generation logic until
https://github.com/dotnet/sdk/issues/3098 is fixed. -->
<DepsFileGenerationMode>new</DepsFileGenerationMode>
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure

</PropertyGroup>

<ItemGroup>
<TrimmerRootDescriptors Include="$(TrimmerRootDescriptors)" />
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

</ItemGroup>

<!--
============================================================
_ILLink

Replace the files to be published with versions that have been
passed through the linker. Also prevent removed files from being
included in the generated deps.json.
============================================================
-->
<Target Name="_ILLink"
Condition=" '$(PublishTrimmed)' == 'true' And
'$(_TargetFrameworkVersionWithoutV)' >= '3.0' And '$(TargetFrameworkIdentifier)' == '.NETCoreApp' "
DependsOnTargets="_RunILLink">

<!-- For now, use ResolvedFileToPublish as input/output. This
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question - @fadimounir can you answer this?

Copy link

Choose a reason for hiding this comment

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

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

should go away in favor of a well-defined set of runtime
assemblies with https://github.com/dotnet/sdk/issues/3109. -->
<ItemGroup>
<_LinkedResolvedFileToPublish Include="@(_LinkedResolvedFileToPublishCandidates)" Condition="Exists('%(Identity)')" />
<ResolvedFileToPublish Remove="@(_ManagedAssembliesToLink)" />
<ResolvedFileToPublish Include="@(_LinkedResolvedFileToPublish)" />
</ItemGroup>

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

<ResolvedCompileFileDefinitions Remove="@(_RemovedManagedAssemblies)" />
<NativeCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
<ResourceCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
<RuntimeCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
<RuntimeTargetsCopyLocalItems Remove="@(_RemovedManagedAssemblies)" />
<RuntimePackAsset Remove="@(_RemovedManagedAssemblies)" />
Copy link

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

</ItemGroup>

</Target>


<!--
============================================================
_RunILLink

Execute the linker. This target runs incrementally, only executing
if the output semaphore file is out of date with respect to the inputs.
============================================================
-->
<UsingTask TaskName="ILLink" AssemblyFile="$(ILLinkTasksAssembly)" />
<Target Name="_RunILLink"
DependsOnTargets="_ComputeManagedAssembliesToLink"
Inputs="$(MSBuildAllProjects);@(_ManagedAssembliesToLink);@(TrimmerRootDescriptors);@(ReferencePath)"
Outputs="$(_LinkSemaphore)">
sbomer marked this conversation as resolved.
Show resolved Hide resolved

<Delete Files="@(_LinkedResolvedFileToPublishCandidates)" />
nguerrera marked this conversation as resolved.
Show resolved Hide resolved
<ILLink AssemblyPaths="@(_ManagedAssembliesToLink)"
ReferenceAssemblyPaths="@(ReferencePath)"
RootAssemblyNames="@(IntermediateAssembly->'%(Filename)')"
RootDescriptorFiles="@(TrimmerRootDescriptors)"
OutputDirectory="$(IntermediateLinkDir)"
ExtraArgs="$(_ExtraTrimmerArgs) --skip-unresolved true" />

<Touch Files="$(_LinkSemaphore)" AlwaysCreate="true" />

</Target>

<!--
============================================================
_ComputeManagedAssembliesToLink

Compute the set of inputs to the linker. Currently this uses a
heuristic to get the relevant input from ResolvedFileToPublish,
but with https://github.com/dotnet/sdk/issues/3109, this should be
replaced with the exact set of runtime assemblies that will be in
the deps file.
============================================================
-->
<UsingTask TaskName="ComputeManagedAssemblies" AssemblyFile="$(ILLinkTasksAssembly)" />
<Target Name="_ComputeManagedAssembliesToLink">
<ComputeManagedAssemblies Assemblies="@(ResolvedFileToPublish)">
<Output TaskParameter="ManagedAssemblies" ItemName="_ManagedAssembliesToLink" />
</ComputeManagedAssemblies>

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

</Target>


</Project>
6 changes: 4 additions & 2 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ Copyright (c) .NET Foundation. All rights reserved.
-->
<Target Name="ComputeAndCopyFilesToPublishDirectory"
DependsOnTargets="ComputeFilesToPublish;
_ILLink;
CreateReadyToRunImages;
CopyFilesToPublishDirectory" />

Expand Down Expand Up @@ -732,11 +733,12 @@ Copyright (c) .NET Foundation. All rights reserved.
DependsOnTargets="_ComputeExcludeFromPublishPackageReferences;
_ParseTargetManifestFiles">

<!-- If there are no excluded packages or runtime store packages, then we can reuse the deps file from the build, -->
<!-- If there are no excluded packages or runtime store packages, and we are not linking, then we can reuse the deps file from the build, -->
<PropertyGroup>
<_UseBuildDependencyFile Condition="'@(_ExcludeFromPublishPackageReference)' == '' and
'@(RuntimeStorePackages)' == '' and
'$(PreserveStoreLayout)' != 'true'">true</_UseBuildDependencyFile>
'$(PreserveStoreLayout)' != 'true' and
'$(PublishTrimmed)' != 'true'">true</_UseBuildDependencyFile>
</PropertyGroup>

</Target>
Expand Down
1 change: 1 addition & 0 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,4 +753,5 @@ Copyright (c) .NET Foundation. All rights reserved.
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.CSharp.targets" Condition="'$(Language)' == 'C#'" />
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.VisualBasic.targets" Condition="'$(Language)' == 'VB'" />
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.Sdk.FSharp.targets" Condition="'$(Language)' == 'F#'" />
<Import Project="$(MSBuildThisFileDirectory)Microsoft.NET.ILLink.targets" />
</Project>
Loading