-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add step to run linker on entire runtime pack during libraries build #40172
Changes from all commits
b473edb
6e5944d
5cf9bd2
7da6456
aa9cd3a
d26e458
618e9c0
050925c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,76 @@ | ||||||||||||
<Project> | ||||||||||||
|
||||||||||||
<Target Name="ILLinkTrimSharedFramework" | ||||||||||||
AfterTargets="Build" | ||||||||||||
DependsOnTargets="SetCommonILLinkArgs"> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<LibrariesTrimmedArtifactsPath>$([MSBuild]::NormalizePath('$(ArtifactsBinDir)', 'ILLinkTrimAssembly', '$(BuildSettings)', 'trimmed-runtimepack'))</LibrariesTrimmedArtifactsPath> | ||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<!-- default action for core assemblies --> | ||||||||||||
<ILLinkArgs>$(ILLinkArgs) -c link</ILLinkArgs> | ||||||||||||
<!-- update debug symbols --> | ||||||||||||
<ILLinkArgs>$(ILLinkArgs) -b true</ILLinkArgs> | ||||||||||||
<!-- suppress warnings with the following codes: | ||||||||||||
IL2006: The generic parameter 'T' from A with dynamically accessed member kinds B is passed into the generic parameter | ||||||||||||
'T' from 'System.Lazy<T>' which requires dynamically accessed member kinds 'PublicParameterlessConstructor' | ||||||||||||
IL2009: Could not find method A in type B specified in resource C | ||||||||||||
IL2025: Duplicate preserve of A in B | ||||||||||||
IL2026: Calling A which has B can break functionality when trimming application code. The target method might be removed. | ||||||||||||
IL2035: Unresolved assembly A in DynamicDependencyAttribute on B | ||||||||||||
IL2050: P/invoke method A declares a parameter with COM marshalling. Correctness of COM interop | ||||||||||||
cannot be guaranteed after trimming. Interfaces and interface members might be removed. | ||||||||||||
--> | ||||||||||||
<ILLinkArgs>$(ILLinkArgs) --nowarn IL2006;IL2009;IL2025;IL2026;IL2035;IL2050</ILLinkArgs> | ||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<!-- Retrieve CoreLib's targetpath via GetTargetPath as it isn't binplaced yet. --> | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand this. Why can't we use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
fwiw, wasm.targets takes a similar approach - runtime/src/mono/wasm/wasm.targets Lines 26 to 30 in b77b242
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My naive thinking is that there should be somewhere in the build (that doesn't rely on tests) that we can hook in where the entire runtimepack is built. But I'm not seeing where that place is. Maybe @safern @ViktorHofer @joperezr or @ericstj might know. But for now, what you have works so let's go with that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SPC and the runtime gets binplaced into libraries runtime pack and testhost as part of runtime.depproj. Runtime.depproj is built as part of pretest.proj which is the last step of the FWIW we could introduce a common target to get SPC target path instead of duplicating the logic to call MSBuild on it with GetTargetPath target. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It feels to me that the correct fix here would be to fully populate the runtimepack earlier, and not make it dependent on testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree with @eerhardt, we have talked about this couple times already. I believe we should binplace the runtime artifacts incrementally after they are built (CoreLib, coreclr.dll, etc). For CI we just need a target that re-binplaces the runtime artifacts (to preserve the existing behavior which allows to build coreclr and libraries in parallel).
I don't understand why binplacing the runtime artifacts in pretest is handy for inner loop development instead of binplacing the runtime assets at the time they are (re-)built, which doesn't require a libraries rebuild. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We've also talked about this a couple of times. If you build for example System.Runtime which has a P2P to SPC, and don't specify RuntimeConfiguration as an argument you would potentially be binplacing a So we should be careful on how we do that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Btw, I agree we should change this if possible, I'm just saying we should be really careful how and where we put this binplacing to happen. Also, I wouldn't like removing the build.pretest binplacing the runtime as it is handy for CI as well as I explained above... if we remove that, then provide another hook to binplace the runtime as an individual step. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think we all agree that we should change when we binplace the runtime artifacts and I agree that it should be done with the highest diligence. We should do this as part of #38034.
That's what I meant with:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I somehow missed that part 🤦
Sounds good. |
||||||||||||
<MSBuild Projects="$(CoreLibProject)" | ||||||||||||
Targets="GetTargetPath"> | ||||||||||||
<Output TaskParameter="TargetOutputs" PropertyName="SystemPrivateCoreLibPath" /> | ||||||||||||
</MSBuild> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<_AssemblyPaths>$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir);$(SystemPrivateCoreLibPath)</_AssemblyPaths> | ||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<ItemGroup> | ||||||||||||
<!-- add references from the libraries directory --> | ||||||||||||
<_DependencyDirectories Include="$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir.TrimEnd('\'))" /> | ||||||||||||
</ItemGroup> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<ILLinkArgs>$(ILLinkArgs) -d @(_DependencyDirectories->'"%(Identity)"', ' -d ')</ILLinkArgs> | ||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<ItemGroup> | ||||||||||||
<_AssembliesToLink Include="System.Private.CoreLib" /> | ||||||||||||
|
||||||||||||
<_LibrariesToLink Include="$(MicrosoftNetCoreAppRuntimePackRidLibTfmDir)*.dll" /> | ||||||||||||
<_AssembliesToLink Include="@(_LibrariesToLink->'%(FileName)')" /> | ||||||||||||
</ItemGroup> | ||||||||||||
|
||||||||||||
<PropertyGroup> | ||||||||||||
<ILLinkArgs>$(ILLinkArgs) -r @(_AssembliesToLink->'%(Identity)', ' -r ')</ILLinkArgs> | ||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<!-- When running from Desktop MSBuild, DOTNET_HOST_PATH is not set. | ||||||||||||
In this case, explicitly specify the path to the dotnet host. --> | ||||||||||||
<PropertyGroup Condition=" '$(DOTNET_HOST_PATH)' == '' "> | ||||||||||||
<_DotNetHostDirectory>$(RepoRoot).dotnet</_DotNetHostDirectory> | ||||||||||||
layomia marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
<_DotNetHostFileName>dotnet</_DotNetHostFileName> | ||||||||||||
<_DotNetHostFileName Condition=" '$(OS)' == 'Windows_NT' ">dotnet.exe</_DotNetHostFileName> | ||||||||||||
layomia marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
</PropertyGroup> | ||||||||||||
|
||||||||||||
<ILLink AssemblyPaths="$(_AssemblyPaths)" | ||||||||||||
RootAssemblyNames="" | ||||||||||||
OutputDirectory="$(LibrariesTrimmedArtifactsPath)" | ||||||||||||
ExtraArgs="$(ILLinkArgs)" | ||||||||||||
ToolExe="$(_DotNetHostFileName)" | ||||||||||||
ToolPath="$(_DotNetHostDirectory)" /> | ||||||||||||
</Target> | ||||||||||||
|
||||||||||||
<Import Project="$([MSBuild]::NormalizePath('$(RepositoryEngineeringDir)', 'illink.targets'))" /> | ||||||||||||
layomia marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -49,6 +49,9 @@ | |
Properties="$(TraversalGlobalProperties)" /> | ||
</Target> | ||
|
||
<Import Condition="'$(BuildTargetFramework)' == '$(NetCoreAppCurrent)'" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: " There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
Project="$(MSBuildThisFileDirectory)\illink-sharedframework.targets" /> | ||
|
||
<Target Name="RunApiCompat" | ||
Condition="'@(ApiCompatProject)' != ''" | ||
AfterTargets="BuildManualShims"> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the change from
IL2041
toIL2050
? Nothing in this change should have changed what warnings are produced for the individual library build. So I think both2041
and2050
can be removed here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I started getting
2050
after syncing with master at the beginning of this PR. Some new code since the initial suppressions check in must have been added which causes these errors:2041
doesn't show up anymore following the sync hence removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that came in last week with dotnet/linker#1382.
cc @MichalStrehovsky
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we now detect COM usage and mark it as unsafe. Apps that end up referencing these methods after trimming will always warn.