Remaining feedback from ILLink merge #79677
Conversation
Move ILLink MSBuild dependencies into Versions.props MSBuild section, and use a consistent MSBuild version Update the readme from linker to state is a project and not a repository Uses a live build of ilasm instead of relying on prebuilt bits from Microsoft.NET.Sdk.IL Remove a unnecesary DefineConstant that is imported via SDK Avoid having hardcoded package versions Removes the illink PATENTS.txt, runtime's PATENTS.txt already takes into account Mono given that it has a mono partition
tlakollo
added
the
area-Tools-ILLink
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsMove ILLink MSBuild dependencies into Versions.props MSBuild section, and use a consistent MSBuild version
|
| @@ -68,7 +68,7 @@ | |||
| </ItemGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="System.Security.Permissions" Version="5.0.0" /> | |||
| <PackageReference Include="System.Security.Permissions" Version="$(SystemSecurityPermissionsVersion)" /> | |||
There was a problem hiding this comment.
It's unfortunate that we don't have a good way to establish a live dependency here without bringing in the entire libs build graph. You clearly stated that when building the linker, you want to avoid to build the "clr+libs" subsets which is why you chose to build against an LKG instead. But by using an LKG, we now bring in these OOB prebuilts that we can either add to dependency flow or just keep pinned to a static version.
There was a problem hiding this comment.
Could we refactor these tests to be part of the libs test subset, or some test subset that does depend on both libs and linker?
There was a problem hiding this comment.
Sure, we can always sequence them very late or make them depend on the entire libs subset to be built but that's not what the linker team wants. They prefer the least number of pre-steps to build and test the linker, and depending on an LKG seems like the right compromise here.
Another thought, we could make sure that these tests depend on the NetCoreAppPrevious asset of these OOB libraries instead of NetCoreAppCurrent which would be achievable via a ProjectReference and a SetTargetFramework=$(NetCoreAppPrevious) metadata on it. Obviously, that would only work for projects that don't contribute to source build (i.e. tests).
ghost
locked as resolved and limited conversation to collaborators
Move ILLink MSBuild dependencies into Versions.props MSBuild section, and use a consistent MSBuild version
Update the readme from linker to state is a project and not a repository
Uses a live build of ilasm instead of relying on prebuilt bits from Microsoft.NET.Sdk.IL
Remove a unnecessary DefineConstant that is imported via SDK
Avoid having hardcoded package versions
Removes the illink PATENTS.txt, runtime's PATENTS.txt already takes into account Mono given that it has a mono partition