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

Jump to bottom

Remaining feedback from ILLink merge #79677

Merged
merged 3 commits into from
Dec 22, 2022
Merged

Remaining feedback from ILLink merge #79677

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08f0f94
Remaining feedback from ILLink merge
Dec 14, 2022
71bfb6b
PR feedback about README.md
tlakollo Dec 21, 2022
b328d06
Merge branch 'main' into ILLinkFeedback2
tlakollo Dec 21, 2022
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
9 changes: 6 additions & 3 deletions eng/Versions.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,11 @@
<SystemSecurityCryptographyCngVersion>5.0.0</SystemSecurityCryptographyCngVersion>
<SystemSecurityCryptographyOpenSslVersion>5.0.0</SystemSecurityCryptographyOpenSslVersion>
<SystemSecurityPrincipalWindowsVersion>5.0.0</SystemSecurityPrincipalWindowsVersion>
<SystemSecurityPermissionsVersion>7.0.0</SystemSecurityPermissionsVersion>
<SystemServiceModelPrimitivesVersion>4.9.0</SystemServiceModelPrimitivesVersion>
<SystemTextJsonVersion>8.0.0-alpha.1.22611.2</SystemTextJsonVersion>
<SystemRuntimeCompilerServicesUnsafeVersion>6.0.0</SystemRuntimeCompilerServicesUnsafeVersion>
<SystemThreadingAccessControlVersion>7.0.0</SystemThreadingAccessControlVersion>
<SystemThreadingTasksExtensionsVersion>4.5.4</SystemThreadingTasksExtensionsVersion>
<SystemValueTupleVersion>4.5.0</SystemValueTupleVersion>
<runtimenativeSystemIOPortsVersion>8.0.0-alpha.1.22611.2</runtimenativeSystemIOPortsVersion>
Expand Down Expand Up @@ -172,6 +174,8 @@
<DNNEVersion>1.0.27</DNNEVersion>
<MicrosoftBuildVersion>17.3.2</MicrosoftBuildVersion>
<MicrosoftBuildTasksCoreVersion>$(MicrosoftBuildVersion)</MicrosoftBuildTasksCoreVersion>
<MicrosoftBuildFrameworkVersion>$(MicrosoftBuildVersion)</MicrosoftBuildFrameworkVersion>
<MicrosoftBuildUtilitiesCoreVersion>$(MicrosoftBuildVersion)</MicrosoftBuildUtilitiesCoreVersion>
<NugetProjectModelVersion>6.2.2</NugetProjectModelVersion>
<NugetPackagingVersion>6.2.2</NugetPackagingVersion>
<!-- Testing -->
Expand All @@ -184,6 +188,8 @@
<XUnitVersion>2.4.2</XUnitVersion>
<XUnitAnalyzersVersion>1.0.0</XUnitAnalyzersVersion>
<XUnitRunnerVisualStudioVersion>2.4.5</XUnitRunnerVisualStudioVersion>
<NUnitVersion>3.12.0</NUnitVersion>
<NUnitTestAdapterVersion>4.1.0</NUnitTestAdapterVersion>
<CoverletCollectorVersion>3.1.2</CoverletCollectorVersion>
<NewtonsoftJsonVersion>13.0.1</NewtonsoftJsonVersion>
<NewtonsoftJsonBsonVersion>1.0.2</NewtonsoftJsonBsonVersion>
Expand All @@ -210,9 +216,6 @@
<!-- Mono Cecil -->
<MicrosoftDotNetCecilVersion>0.11.4-alpha.22578.1</MicrosoftDotNetCecilVersion>
<MicrosoftDotNetCecilPdbVersion>0.11.4-alpha.22578.1</MicrosoftDotNetCecilPdbVersion>
<!-- ILLink dependencies -->
<MicrosoftBuildFrameworkVersion>17.0.0-preview-21267-01</MicrosoftBuildFrameworkVersion>
<MicrosoftBuildUtilitiesCoreVersion>17.0.0-preview-21267-01</MicrosoftBuildUtilitiesCoreVersion>
<!-- ICU -->
<MicrosoftNETCoreRuntimeICUTransportVersion>8.0.0-alpha.1.22612.1</MicrosoftNETCoreRuntimeICUTransportVersion>
<!-- MsQuic -->
Expand Down
44 changes: 0 additions & 44 deletions src/tools/illink/PATENTS.TXT
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/tools/illink/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Trimming Tools

This repository hosts various tools and msbuild tasks which are used when trimming managed applications with .NET 5 and newer.
This project hosts various tools and msbuild tasks which are used when trimming managed applications with .NET 5 and newer.
tlakollo marked this conversation as resolved.
Show resolved Hide resolved
tlakollo marked this conversation as resolved.
Show resolved Hide resolved

## IL Trimmer

Expand Down
17 changes: 0 additions & 17 deletions src/tools/illink/ilasm.ilproj
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion src/tools/illink/test/Mono.Linker.Tests.Cases/Mono.Linker.Tests.Cases.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="System.Security.Permissions" Version="5.0.0" />
<PackageReference Include="System.Security.Permissions" Version="$(SystemSecurityPermissionsVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that we don't have a good way to establish a live dependency here without bringing in the entire libs build graph. You clearly stated that when building the linker, you want to avoid to build the "clr+libs" subsets which is why you chose to build against an LKG instead. But by using an LKG, we now bring in these OOB prebuilts that we can either add to dependency flow or just keep pinned to a static version.

Copy link
Member

Choose a reason for hiding this comment

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

cc @ericstj

Copy link
Member

Choose a reason for hiding this comment

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

Could we refactor these tests to be part of the libs test subset, or some test subset that does depend on both libs and linker?

Copy link
Member

@ViktorHofer ViktorHofer Dec 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, we can always sequence them very late or make them depend on the entire libs subset to be built but that's not what the linker team wants. They prefer the least number of pre-steps to build and test the linker, and depending on an LKG seems like the right compromise here.

Another thought, we could make sure that these tests depend on the NetCoreAppPrevious asset of these OOB libraries instead of NetCoreAppCurrent which would be achievable via a ProjectReference and a SetTargetFramework=$(NetCoreAppPrevious) metadata on it. Obviously, that would only work for projects that don't contribute to source build (i.e. tests).

</ItemGroup>

</Project>
29 changes: 13 additions & 16 deletions src/tools/illink/test/Mono.Linker.Tests/Mono.Linker.Tests.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

<PropertyGroup>
<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
<DefineConstants Condition="'$(Configuration)' == 'Debug'">$(DefineConstants);DEBUG</DefineConstants>
</PropertyGroup>

<ItemGroup>
Expand All @@ -18,30 +17,25 @@
<PackageReference Include="Microsoft.DotNet.Cecil.Pdb" Version="$(MicrosoftDotNetCecilPdbVersion)" PrivateAssets="All" Publish="True" />
<PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="$(MicrosoftCodeAnalysisVersion)" />
<ProjectReference Include="$(RepoRoot)\src\coreclr\tools\ILVerification\ILVerification.csproj" />
<PackageReference Include="nunit" Version="3.12.0" />
<PackageReference Include="NUnit3TestAdapter" Version="4.1.0" />
<PackageReference Include="nunit" Version="$(NUnitVersion)" />
<PackageReference Include="NUnit3TestAdapter" Version="$(NUnitTestAdapterVersion)" />
<!-- This reference is purely so that the linker can resolve this
dependency of mscorlib. It is not actually required to build
the tests. -->
<PackageReference Include="System.Threading.AccessControl" Version="5.0.0" />
<PackageReference Include="System.Threading.AccessControl" Version="$(SystemThreadingAccessControlVersion)" />

<ProjectReference Include="..\..\src\linker\Mono.Linker.csproj" SkipUseReferenceAssembly="true" />
<ProjectReference Include="..\Mono.Linker.Tests.Cases\Mono.Linker.Tests.Cases.csproj" />
<ProjectReference Include="..\Mono.Linker.Tests.Cases.Expectations\Mono.Linker.Tests.Cases.Expectations.csproj" />
</ItemGroup>

<Target Name="PrepareTools" BeforeTargets="Build">
<!-- Restore ilasm using ilasm.ilproj. Restore must be done
separately from copy with a different set of input
properties, to force MSBuild to re-evaluate using new props
and targets from the restored package. -->
<PropertyGroup>
<IlasmProject>$(MSBuildThisFileDirectory)../../ilasm.ilproj</IlasmProject>
</PropertyGroup>

<MSBuild Projects="$(IlasmProject)" Targets="Restore" />
<MSBuild Projects="$(IlasmProject)" Targets="CopyILAsmTool" />
</Target>
<!-- Tests require ilasm so we make sure that it is live built before executing the tests. -->
<ItemGroup>
<ProjectReference Include="$(CoreClrProjectRoot)runtime.proj"
AdditionalProperties="ClrILToolsSubset=true;Configuration=$(CoreCLRConfiguration)"
ReferenceOutputAssembly="false"
SkipGetTargetFrameworkProperties="true" />
</ItemGroup>
jkoritzinsky marked this conversation as resolved.
Show resolved Hide resolved

<ItemGroup>
<None Update="TestCases\Dependencies\PInvokesExpectations.json">
Expand All @@ -57,6 +51,9 @@
<RuntimeHostConfigurationOption Include="Mono.Linker.Tests.ArtifactsDir">
<Value>$(ArtifactsDir)</Value>
</RuntimeHostConfigurationOption>
<RuntimeHostConfigurationOption Include="Mono.Linker.Tests.ILToolsDir">
<Value>$(CoreCLRArtifactsPath)</Value>
</RuntimeHostConfigurationOption>
<RuntimeHostConfigurationOption Include="Mono.Linker.Tests.ArtifactsBinDir">
<Value>$(ArtifactsBinDir)</Value>
</RuntimeHostConfigurationOption>
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/test/Mono.Linker.Tests/TestCasesRunner/ILCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,9 @@ protected virtual NPath LocateIlasm ()
#if NETCOREAPP
var extension = RuntimeInformation.IsOSPlatform (OSPlatform.Windows) ? ".exe" : "";

var toolsDir = Path.Combine((string)AppContext.GetData("Mono.Linker.Tests.ArtifactsDir")!, "tools");
var toolsDir = (string)AppContext.GetData("Mono.Linker.Tests.ILToolsDir")!;

var ilasmPath = Path.GetFullPath (Path.Combine (toolsDir, "ilasm", $"ilasm{extension}")).ToNPath ();
var ilasmPath = Path.GetFullPath (Path.Combine (toolsDir, $"ilasm{extension}")).ToNPath ();
if (ilasmPath.FileExists ())
return ilasmPath;

Expand Down