Linker into runtime diff #77569

tlakollo · 2022-10-27T20:57:07Z

Updating the consolidation branch into a new branch called LinkerIntoRuntime inside the runtime repo and adding on top of all the commits in #77149 for an easier updated review.
For more information about these changes please refer to #75278.

…ill use the runtime arcade infra Remove build.cmd/build.sh and lint.cmd/lint.sh in src\tools\linker directory since now they will execute via a subset Remove/Merge common files from src\tools\linker root: - .editorconfig - .gitattributes - .gitignore - .github - .gitmodules - after.illink.sln.targets - code_of_conduct.md - global.json - LICENSE.txt - NuGet.config - THIRD-PARTY-NOTICES.TXT Remove/Merge common files from src\tools\linker\eng: - Publishing.props - Signing.props - SourceBuild.props - SourceBuildPrebuiltBaseline.xml - Tools.props - Version.Details.xml - Versions.props

Create a subsets tools.linker and tools.linkertests that build and test the different csproj files Add arcade cecil package to Version.Details.xml and Versions.props Delete cecil from the external folder and its related files Add PackageReference of cecil in Mono.Linker.csproj Tweaks to make things build

Tweaks to make dotnet format to work

Set UsingToolMicrosoftNetILLinkTasks to true to not use the recently build package in CI Remove the PackageId name to workaround the cyclic dependency in the dependency graph Do not use relative paths to find stuff instead use msbuild properties Fix cecil test that checks for an old cecil package version

Change source lines since now we are using spaces instead of tabs (pending issue with pdbs and symbols on assemblies) Workaround the deletion of RefSafetyRulesAttribute and their associated types injected by the compiler For now add extra ExpectedWarnings on warnings being duplicated by the analyzer (pending investigation)

…related tests will fail Upgrade to MicrosoftCodeAnalysisVersion 4.5+ generates a different type of analysis callbacks, the CheckAttributeInstantiation method is no longer needed. And a bug in which a warning was printed twice by the analyzer is fixed.

…e vs the microsoft internal package

Typo on Cecil.Pdb package Update names in csproj's Add warning for file header mismatch/missing

Add condition for test group to any change in src/tools/linker Remove extra tool in global.json Add a dummy line change to test if CI triggers

…er changes Exclude building the clr in linker testing

ghost · 2022-10-27T20:59:28Z

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author:	tlakollo
Assignees:	tlakollo
Labels:	`NO-MERGE`, `area-System.Reflection.Metadata`
Milestone:	-

ghost · 2022-10-27T21:00:39Z

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

Updating the consolidation branch into a new branch called LinkerIntoRuntime inside the runtime repo and adding on top of all the commits in #77149 for an easier updated review.
For more information about these changes please refer to #75278.

Author:	tlakollo
Assignees:	tlakollo
Labels:	`NO-MERGE`, `area-System.Reflection.Metadata`, `area-Infrastructure`, `area-Tools-linker`
Milestone:	-

tlakollo · 2022-10-27T21:00:48Z

Adding @dotnet/runtime-infrastructure for review

sbomer

It might be helpful to adopt the runtime's code style settings in dotnet/linker to make it easier to port commits over after the initial change.

eng/Subsets.props

eng/Version.Details.xml

eng/Versions.props

eng/pipelines/coreclr/templates/build-job.yml

src/tools/linker/external/Mono.Options/Options.cs

ViktorHofer · 2022-10-28T08:19:51Z

eng/Subsets.props

+  <ItemGroup Condition="$(_subset.Contains('+tools.linkertests+'))">
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\Mono.Linker.Tests\Mono.Linker.Tests.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\Mono.Linker.Tests.Cases\Mono.Linker.Tests.Cases.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\Mono.Linker.Tests.Cases.Expectations\Mono.Linker.Tests.Cases.Expectations.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\ILLink.Tasks.Tests\ILLink.Tasks.Tests.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\ILLink.RoslynAnalyzer.Tests\ILLink.RoslynAnalyzer.Tests.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+    <ProjectToBuild Include="$(ToolsProjectRoot)linker\test\ILLink.RoslynAnalyzer.Tests.Generator\ILLink.RoslynAnalyzer.Tests.Generator.csproj"
+      Test="true" Category="tools" Condition="'$(DotNetBuildFromSource)' != 'true'"/>
+  </ItemGroup>


I don't think that you need the DotNetBuildFromSource condition here as that subset won't be built automatically. Similar to the libs.tests subset which isn't built automatically either.

For now, I think we don't want the burden of everyone building linker even if they won't use it and therefore I'm not including it in the DefaultSubsets. I will remove the conditions for DotNetBuildFromSource

eng/Versions.props

ViktorHofer · 2022-10-28T08:23:00Z

src/tools/linker/docs/design/constant-propagation-and-branch-removal.md

@@ -12,7 +12,7 @@ Method can return constant value if its code will always return the same value (
    public bool Is32Bit { get => false; }
 ```

-On 64bit platforms the property is compiled with constant value, and ILLInk can determine this. It's also possible to use substitutions to overwrite method's return value to a constant via the [substitutions XML file](../data-formats.md#substitution-format). 
+On 64bit platforms the property is compiled with constant value, and ILLInk can determine this. It's also possible to use substitutions to overwrite method's return value to a constant via the [substitutions XML file](../data-formats.md#substitution-format).


When we consolidated corefx, coreclr, core-setup and mono into dotnet/runtime, we also moved all the docs into a consolidated location: runtime/docs/. It might make sense to move these into the consolidated location as well.

You marked this comment as resolved but didn't respond. The proposed docs consolidation doesn't need to happen now (or ever, if others disagree) but I would like to know what you think it about my suggestion.

In this PR I actually moved the docs, that's why I marked it as resolved. But a couple of people in the team tried to review the PR and it was difficult due to non-functional changes. So we decided to keep things that don't affect build/test for follow-up. Sorry for the confusion, I opened #78052 to track moving the docs

ViktorHofer · 2022-10-28T08:25:43Z

With these changes, the tools.linket subset doesn't build automatically. Is that intentional as part of this change? If not, you want to add the subset to the DefaultSubsets property:

runtime/eng/Subsets.props

Line 34 in cac1b59

<DefaultSubsets>clr+mono+libs+host+packs</DefaultSubsets>

eng/Versions.props

eng/pipelines/coreclr/templates/build-job.yml

marek-safar · 2022-10-31T09:40:16Z

...r/external/corert/src/System.Private.CoreLib/shared/System/Reflection/RuntimeAssemblyName.cs

@@ -1,123 +1,125 @@
 // Licensed to the .NET Foundation under one or more agreements.


Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?

I gave a shot to this very briefly and the NativeAOT files start to add more dependencies very quickly, so I will leave it as a separate item that can be investigated after the first commit (see #77868).

src/tools/linker/Directory.Build.props

.github/CODEOWNERS

tlakollo · 2022-11-08T18:39:05Z

I will close this PR to give priority to #78049 which is a newer sync with linker, does not include non functional changes and renames the linker to illink

tlakollo and others added 14 commits October 27, 2022 13:20

Run dotnet format on tools\linker

c84d0e5

Tweaks to make dotnet format to work

Format after cherry-pick

f65daac

Fix Markdownlint

7fec196

Update cecil package name to easily differenciate the official packag…

e44ddd0

…e vs the microsoft internal package

Remove unused gitignore code lines

945e0d9

Typo on Cecil.Pdb package Update names in csproj's Add warning for file header mismatch/missing

Format file headers as runtime

0f79e9c

Attempt to run linker unit tests in CI

18c088c

Actually create a test group otherwise CI wont trigger

196ddd0

Add condition for test group to any change in src/tools/linker Remove extra tool in global.json Add a dummy line change to test if CI triggers

Exclude src/tools from some of the pipelines so they dont run on link…

57e034d

…er changes Exclude building the clr in linker testing

tlakollo requested review from agocke, sbomer and vitek-karas October 27, 2022 20:57

tlakollo self-assigned this Oct 27, 2022

tlakollo added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 27, 2022

dotnet-issue-labeler bot added the area-System.Reflection.Metadata label Oct 27, 2022

tlakollo added area-Infrastructure area-Tools-ILLink .NET linker development as well as trimming analyzers and removed area-System.Reflection.Metadata labels Oct 27, 2022

tlakollo mentioned this pull request Oct 27, 2022

Move linker to runtime #77149

Closed

sbomer reviewed Oct 27, 2022

View reviewed changes

ViktorHofer reviewed Oct 28, 2022

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

ViktorHofer reviewed Oct 28, 2022

View reviewed changes

teo-tsirpanis removed the area-Infrastructure label Oct 28, 2022

marek-safar reviewed Oct 31, 2022

View reviewed changes

Tlakollo added 3 commits October 31, 2022 16:19

PR feedback

a800782

Move linker documentation

487dfb2

More PR feedback

7fbeec8

jkotas reviewed Nov 4, 2022

View reviewed changes

.github/CODEOWNERS Show resolved Hide resolved

agocke self-assigned this Nov 7, 2022

jkotas mentioned this pull request Nov 7, 2022

Add linker to area owners #77983

Merged

tlakollo mentioned this pull request Nov 8, 2022

Linker into runtime diff2 #78049

Merged

tlakollo closed this Nov 8, 2022

This was referenced Nov 8, 2022

Rename linker to illink #78059

Closed

Linker into runtime2 #78077

Merged

ghost locked as resolved and limited conversation to collaborators Dec 8, 2022

tlakollo deleted the LinkerIntoRuntimeDiff branch January 16, 2023 05:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Linker into runtime diff #77569

Linker into runtime diff #77569

tlakollo commented Oct 27, 2022 •

edited

Loading

ghost commented Oct 27, 2022

ghost commented Oct 27, 2022

tlakollo commented Oct 27, 2022

sbomer left a comment

ViktorHofer Oct 28, 2022

tlakollo Nov 3, 2022

ViktorHofer Oct 28, 2022

ViktorHofer Nov 8, 2022

tlakollo Nov 8, 2022

ViktorHofer commented Oct 28, 2022 •

edited

Loading

marek-safar Oct 31, 2022

ViktorHofer Oct 31, 2022

tlakollo Nov 3, 2022

tlakollo commented Nov 8, 2022

		@@ -1,123 +1,125 @@
		// Licensed to the .NET Foundation under one or more agreements.

Linker into runtime diff #77569

Linker into runtime diff #77569

Conversation

tlakollo commented Oct 27, 2022 • edited Loading

ghost commented Oct 27, 2022

ghost commented Oct 27, 2022

tlakollo commented Oct 27, 2022

sbomer left a 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

ViktorHofer commented Oct 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlakollo commented Nov 8, 2022

tlakollo commented Oct 27, 2022 •

edited

Loading

ViktorHofer commented Oct 28, 2022 •

edited

Loading