Linker into runtime diff2 #78049
Merged
Linker into runtime diff2 #78049
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 5 commits
November 8, 2022 08:27
Remove arcade eng\common directory in src\tools\illink since now we will use the runtime arcade infra Remove build.cmd/build.sh and lint.cmd/lint.sh in src\tools\illink directory since now they will execute via a subset Remove/Merge common files from src\tools\illink root: - .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\illink\eng: - Build.props - Publishing.props - Signing.props - SourceBuild.props - SourceBuildPrebuiltBaseline.xml - Tools.props - Version.Details.xml - Versions.props
Add subsets tools.illink and tools.illinktests for building illink and unitest it Add Microsoft.DotNet.Cecil dependencies to runtime and to illink projects Some workarounds to be able to build illink Delete some cecil information from the external folder since now its a package
Test projects use to have relative paths based on the current working directory to know where to find stuff, now that the project is in a different place things are not found, this commit changes to instead use MSBuild variables to calculate where things are Add the cecil package to tests Change a cecil test that verify the official package name to only care about the important pieces
Add a variable to recognize when illink contains a change, and set an exclusion of the src/tools/* for other repos Add a pipeline file to run illink unitests when there are illink changes
tlakollo
added
NO-MERGE
tlakollo
requested review from
AaronRobinsonMSFT,
ViktorHofer,
agocke,
am11,
jkotas,
marek-safar,
sbomer and
vitek-karas
ViktorHofer
reviewed
Nov 8, 2022
| @@ -1,6 +1,3 @@ | |||
| # top-most EditorConfig file | |||
| root = true | |||
|
|
|||
Member
There was a problem hiding this comment.
Do we want to consolidate the code styling in src/tools/illink with the rest of the repo? If yes, can you please open a tracking issue to remove this .editorconfig later?
Contributor
Author
There was a problem hiding this comment.
Once the linker repo is archived and no cherry picking is being made the editorconfig will be customized to only contain a small subset of overrides from runtime global config #78050
This was referenced Nov 8, 2022
sbomer
approved these changes
Nov 8, 2022
Member
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
LGTM with some minor comments. Thanks!
| <BuildOutputTargetFolder>tools</BuildOutputTargetFolder> | ||
| <!-- Recommended by arcade for tools projects. generates an assembly version that includes patch number derived from date and build revision --> | ||
| <AutoGenerateAssemblyVersion>true</AutoGenerateAssemblyVersion> | ||
| <AutoGenerateAssemblyVersion>false</AutoGenerateAssemblyVersion> |
Member
There was a problem hiding this comment.
Why is this changing here and in the analyzer? If we change it the comment should at least be updated.
Contributor
Author
There was a problem hiding this comment.
Added a comment pointing to the issue that tracks setting the feature to true again #78076
am11
reviewed
Nov 8, 2022
src/tools/illink/test/Mono.Linker.Tests/Mono.Linker.Tests.csproj
Outdated
Show resolved
Hide resolved
agocke
reviewed
Nov 8, 2022
| <Description>MSBuild tasks for running the IL Linker</Description> | ||
| <IsPackable>true</IsPackable> | ||
| <PackageId>Microsoft.NET.ILLink.Tasks</PackageId> | ||
| <!-- Removing the package id to be able to build the linker inside runtime |
Contributor
Author
There was a problem hiding this comment.
There is a Microsoft.NET.ILLink.Tasks package included in the Versions.props and Version.Details.xml since this PackageId generates another package with the same name then the build system will believe that there is a cycle. Once we have live bits being feed into runtime we can delete the package in Versions.props and Version.Details.xml and uncomment the PackageId in this project. Opened #78073 to track this
tlakollo
removed
the
NO-MERGE
Member
|
Thanks! 🎉 (uff, only noticed now that it's still in separate branch and not main 🥺) |
Contributor
Author
Here is the PR targeting main, hopefully will be merged today #78077 |
tlakollo
added a commit
that referenced
this pull request
Nov 15, 2022
* Merge and remove common files Remove arcade eng\common directory in src\tools\illink since now we will use the runtime arcade infra Remove build.cmd/build.sh and lint.cmd/lint.sh in src\tools\illink directory since now they will execute via a subset Remove/Merge common files from src\tools\illink root: - .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\illink\eng: - Build.props - Publishing.props - Signing.props - SourceBuild.props - SourceBuildPrebuiltBaseline.xml - Tools.props - Version.Details.xml - Versions.props * Create subsets to be able to build illink Create a variable for the tools folder in runtime Add subsets tools.illink and tools.illinktests for building illink and unitest it Add Microsoft.DotNet.Cecil dependencies to runtime and to illink projects Some workarounds to be able to build illink Delete some cecil information from the external folder since now its a package * Refactorings to make test work Test projects use to have relative paths based on the current working directory to know where to find stuff, now that the project is in a different place things are not found, this commit changes to instead use MSBuild variables to calculate where things are Add the cecil package to tests Change a cecil test that verify the official package name to only care about the important pieces * Enable pipeline Add a variable to recognize when illink contains a change, and set an exclusion of the src/tools/* for other repos Reuse the dotnet-linker-tests pipeline file to also run illink unitests every time there are illink changes * Fix Markdown lint * Remove checked-in binaries * Use nunit for linker tests and fix cecil version test
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Updating the consolidation branch into a new branch called LinkerIntoRuntime2 inside the runtime repo
The difference with #77569 is that this PR contains the latest linker changes and leaves outside changes that are not functional and are difficult to review (formatting the repo, moving docs, formatting the headers). Also, this PR takes into account the latest feedback about renaming linker to illink.
For more information about these changes please refer to #75278.