Conversation
Import repo root Directory.Build.props|targets Remove some of the duplicated package settings defined in the repo root Directory.Build.props Use shared new Roslyn ApiCompat infrastructure
Disable some warnings using pragmas since code is safe
tlakollo
added
the
area-Tools-ILLink
ghost
added
the
linkable-framework
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsDelete remaining files inside the runtime/src/tools/illink/eng folder
|
src/tools/illink/src/ILLink.Shared/TypeSystemProxy/WellKnownType.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/src/ILLink.Shared/TypeSystemProxy/WellKnownType.cs
Outdated
Show resolved
Hide resolved
| <ProduceReferenceAssembly>false</ProduceReferenceAssembly> | ||
| <ProduceOnlyReferenceAssembly>true</ProduceOnlyReferenceAssembly> | ||
| <!-- Used by Arcade to compute OutputPath, IntermediateOutputPath, etc. early in the import chain. --> | ||
| <OutDirName>$(MSBuildProjectName)/ref</OutDirName> |
There was a problem hiding this comment.
Should not this align with other names we have in artefacts folder?
/cc @ViktorHofer
There was a problem hiding this comment.
Yes. We already have infrastructure for
- building reference assemblies (referenceAssemblies.props)
- referencing them from the source project (resolveContract.targets)
- validating them against the implementation (APICompat)
- re-generating them (GenAPI)
in libraries. Let's make sure to use that instead of duplicating it here. @tlakollo I can help with the remaining stuff.
PR feedback
…tions test since it uses constant propagation that is not supported by the analyzer
…erriden at build time
There was a problem hiding this comment.
I just submitted #79268 which helps this PR and allows you to remove all the msbuild code related to reference assembly projects. I would wait for that one to be merged and then update (rebase/merge) this PR.
EDIT: It's already merged so I just provided new feedback to clean this up in the linker msbuild code.
…ngFeedbackPart1
| Version version = Environment.Version; | ||
| // The linker always runs on .NET Core, even when using desktop MSBuild to host ILLink.Tasks. | ||
| _illinkPath = Path.Combine (Path.GetDirectoryName (taskDirectory), "net7.0", "illink.dll"); | ||
| _illinkPath = Path.Join (AppDomain.CurrentDomain.BaseDirectory, $"net{version.Major}.{version.Minor}", "illink.dll"); |
There was a problem hiding this comment.
This doesn't look right. I think AppDomain.CurrentDomain.BaseDirectory is the location of the current process, not the task DLL. My understanding of this code is that the task wants to find the location of illink.dll relative to itself. I think the Assembly.Location use was probably correct here, and the suppression is probably appropriate too as we can be confident that MSBuild will never be a single-file app.
There was a problem hiding this comment.
Debugging using the TestErrorHandling test these are the values:
Assembly.GetExecutingAssembly ().Location
"runtime\artifacts\bin\ILLink.Tasks.Tests\Debug\net7.0\ILLink.Tasks.dll"
AppDomain.CurrentDomain.BaseDirectory
"runtime\artifacts\bin\ILLink.Tasks.Tests\Debug\net7.0\"
_illinkPath
"runtime\artifacts\bin\ILLink.Tasks.Tests\Debug\net7.0\illink.dll"
So the only part that would be incorrect is adding the "net7.0" folder because it will already be included from getting either Assembly.Location() or BaseDirectory.
There was a problem hiding this comment.
I talked with Sven about the BaseDirectory location when not executed in the test environment. He agrees that the process location might variate and therefore the right approach is to use the Assembly.Location() and the suppression.
| </ProjectReference> | ||
| </ItemGroup> | ||
| <!-- Import reference assembly and ApiCompat logic --> | ||
| <Import Project="$(RepositoryEngineeringDir)resolveContract.targets" Condition="'$(IsSourceProject)' == 'true'" /> |
There was a problem hiding this comment.
Is the property IsSourceProject defined anywhere here? It is for all src/libraries projects but I don't think that any projects under src/tools/illink set it. That makes this target to never be imported and APICompat and other tools to never run.
ghost
locked as resolved and limited conversation to collaborators
6 participants
Delete remaining files inside the runtime/src/tools/illink/eng folder
Import repo root Directory.Build.props|targets
Use shared new Roslyn ApiCompat infrastructure
Reuse some of the config from the repo root instead of rewriting it again in illink
Analyzer.props from illink disabled the StyleCop.Analyzers, by using the runtime root's Directory.Build.props we include both the analyzer.props from runtime which does enable StyleCop.Analyzers and also we include the CodeAnalysis.src.globalconfig which includes a warning for several IDE codes. In order to avoid having migration problems with the old linker repo this PR disables explicitly the codes using the illink editorconfig until both codebases migrate to use runtime analyzers and format.