Linker into runtime diff - #77569

Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom
tlakollo:LinkerIntoRuntimeDiff
Closed

Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom
tlakollo:LinkerIntoRuntimeDiff

Conversation

@tlakollo

@tlakollotlakollo commented Oct 27, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

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.

tlakolloand others added 14 commits October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo self-assigned this Oct 27, 2022
@tlakollotlakollo added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Oct 27, 2022
@ghost

ghost commented Oct 27, 2022

Copy link
Copy Markdown

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:-

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

ghost commented Oct 27, 2022

Copy link
Copy Markdown

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

tlakollo commented Oct 27, 2022

Copy link
Copy Markdown
ContributorAuthor

Adding @dotnet/runtime-infrastructure for review

@tlakollotlakollo mentioned this pull request Oct 27, 2022
Closed
sbomer
sbomer reviewed Oct 27, 2022
View reviewed changes

@sbomersbomer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threadeng/Subsets.props Outdated
Comment threadeng/Subsets.props Outdated
Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props Outdated
Comment threadeng/Versions.props Outdated
Comment threadeng/pipelines/coreclr/templates/build-job.yml Outdated
Comment threadeng/pipelines/coreclr/templates/build-job.yml Outdated
Comment threadsrc/tools/linker/external/Mono.Options/Options.cs Outdated
ViktorHofer
ViktorHofer reviewed Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props
Comment on lines +341 to +354
<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>

@ViktorHoferViktorHoferOct 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@tlakollotlakolloNov 3, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

ViktorHofer
ViktorHofer reviewed Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props Outdated
ViktorHofer
ViktorHofer reviewed Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md Outdated
```

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).

@ViktorHoferViktorHoferOct 28, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@ViktorHoferViktorHoferNov 8, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@tlakollotlakolloNov 8, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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

ViktorHofer commented Oct 28, 2022•
edited
Loading

Copy link
Copy Markdown
Member

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>

marek-safar
marek-safar reviewed Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props Outdated
Comment threadeng/Versions.props Outdated
Comment threadeng/pipelines/coreclr/templates/build-job.yml Outdated
Comment thread...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.

@marek-safarmarek-safarOct 31, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@ViktorHoferViktorHoferOct 31, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

cc @jkotas

@tlakollotlakolloNov 3, 2022

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

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).

Comment threadsrc/tools/linker/Directory.Build.props Outdated
jkotas
jkotas reviewed Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke self-assigned this Nov 7, 2022
@jkotasjkotas mentioned this pull request Nov 7, 2022
Merged
@tlakollotlakollo mentioned this pull request Nov 8, 2022
Merged
@tlakollo

tlakollo commented Nov 8, 2022

Copy link
Copy Markdown
ContributorAuthor

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

@tlakollotlakollo closed this Nov 8, 2022
This was referenced Nov 8, 2022
Closed
Merged
@ghostghost locked as resolved and limited conversation to collaborators Dec 8, 2022
@tlakollo
tlakollo deleted the LinkerIntoRuntimeDiff branch January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas
+1 more reviewer
@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke

@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
Linker into runtime diff - #77569
Closed
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Closed
Linker into runtime diff#77569
tlakollo wants to merge 17 commits into
dotnet:LinkerIntoRuntimefrom 
tlakollo:LinkerIntoRuntimeDiff
Conversation
@tlakollo
@tlakollotlakollo
commented
Oct 27, 2022•

edited

Loading

Copy link


Copy Markdown

Contributor
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.
tlakolloand others
added 14 commits
October 27, 2022 13:20
@tlakollo
Remove arcade eng\common directory in src\tools\linker since now we w…
4e03f9a
…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
@tlakollo
Add ToolsProjectRoot to identify the tools folder inside runtime
aa86d1d
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
@tlakollo
Run dotnet format on tools\linker
c84d0e5
Tweaks to make dotnet format to work
@tlakollo
Format after cherry-pick
f65daac
@tlakollo
Fix Markdownlint
7fec196
Tlakollo@tlakollo
Remove shared projects from build
629e45f
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
Tlakollo@tlakollo
Change WarningSuppressionWriter to use runtime format instead of Mono's
092ff25
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)
@tlakollo
Add Cecil Pdb library otherwise nativePdb reading will throw and all …
0219b41
…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.
@tlakollo
Update cecil package name to easily differenciate the official packag…
e44ddd0
…e vs the microsoft internal package
@tlakollo
Remove unused gitignore code lines
945e0d9
Typo on Cecil.Pdb package
Update names in csproj's
Add warning for file header mismatch/missing
@tlakollo
Format file headers as runtime
0f79e9c
@tlakollo
Attempt to run linker unit tests in CI
18c088c
Tlakollo@tlakollo
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
Tlakollo@tlakollo
Exclude src/tools from some of the pipelines so they dont run on link…
57e034d
…er changes
Exclude building the clr in linker testing
@tlakollotlakollo
self-assigned this
Oct 27, 2022
@tlakollotlakollo
added
the 
NO-MERGE
The PR is not ready for merge yet (see discussion for detailed reasons)
label
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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:-
@tlakollotlakollo
added

area-Infrastructure

area-Tools-ILLink
.NET linker development as well as trimming analyzers
and removed

area-System.Reflection.Metadata

labels
Oct 27, 2022
@ghost
ghost
commented
Oct 27, 2022

Copy link


Copy Markdown

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
tlakollo
commented
Oct 27, 2022

Copy link


Copy Markdown

ContributorAuthor
Adding @dotnet/runtime-infrastructure for review
@tlakollotlakollo
mentioned this pull request
Oct 27, 2022
 Closed

sbomer
sbomer
reviewed
Oct 27, 2022
View reviewed changes
@sbomersbomer
left a comment


Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
Comment threadeng/Subsets.props
Outdated

Comment threadeng/Subsets.props
Outdated

Comment threadeng/Version.Details.xml
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment threadsrc/tools/linker/external/Mono.Options/Options.cs
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Subsets.props

Comment on lines

+341
 to 
+354

 <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>
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

ViktorHofer
ViktorHofer
reviewed
Oct 28, 2022
View reviewed changes
Comment threadsrc/tools/linker/docs/design/constant-propagation-and-branch-removal.md
Outdated

```

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).
@ViktorHoferViktorHoferOct 28, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@ViktorHoferViktorHoferNov 8, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

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.
@tlakollotlakolloNov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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
ViktorHofer
commented
Oct 28, 2022•

edited

Loading

Copy link


Copy Markdown

Member
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> 
marek-safar
marek-safar
reviewed
Oct 31, 2022
View reviewed changes
Comment threadeng/Versions.props
Outdated

Comment threadeng/Versions.props
Outdated

Comment threadeng/pipelines/coreclr/templates/build-job.yml
Outdated

Comment thread...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.
@marek-safarmarek-safarOct 31, 2022

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

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

Could you unify these files with copies under src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ ?
@ViktorHoferViktorHoferOct 31, 2022

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

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

cc @jkotas
@tlakollotlakolloNov 3, 2022

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

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

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).
Comment threadsrc/tools/linker/Directory.Build.props
Outdated

jkotas
jkotas
reviewed
Nov 4, 2022
View reviewed changes
Comment thread.github/CODEOWNERS
@agockeagocke
self-assigned this
Nov 7, 2022
@jkotasjkotas
mentioned this pull request
Nov 7, 2022
 Merged

@tlakollotlakollo
mentioned this pull request
Nov 8, 2022
 Merged

@tlakollo
tlakollo
commented
Nov 8, 2022

Copy link


Copy Markdown

ContributorAuthor
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
@tlakollotlakollo
closed this
Nov 8, 2022

This was referenced Nov 8, 2022

Closed

 Merged

@ghostghost
locked as resolved and limited conversation to collaborators
Dec 8, 2022
@tlakollo
tlakollo
deleted the

LinkerIntoRuntimeDiff

branch
January 16, 2023 05:40
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.


Reviewers

@marek-safarmarek-safarmarek-safar left review comments
@sbomersbomersbomer left review comments
@jkotasjkotasjkotas left review comments
@ViktorHoferViktorHoferViktorHofer left review comments
@agockeagockeAwaiting requested review from agocke
@vitek-karasvitek-karasAwaiting requested review from vitek-karas

+1 more reviewer

@am11am11am11 left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@agockeagocke
@tlakollotlakollo

Labels

area-Tools-ILLink.NET linker development as well as trimming analyzersNO-MERGEThe PR is not ready for merge yet (see discussion for detailed reasons)

Projects


None yet


Milestone


No milestone


Development

Successfully merging this pull request may close these issues.

8 participants

@tlakollo@ViktorHofer@marek-safar@agocke@sbomer@am11@jkotas@teo-tsirpanis