-
Notifications
You must be signed in to change notification settings - Fork 831
Fix ILLink.Substitutions.xml not being embedded in F# assemblies #19124
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: T-Gro <[email protected]>
Co-authored-by: T-Gro <[email protected]>
✅ No release notes required |
tests/FSharp.Build.UnitTests/GenerateILLinkSubstitutionsTests.fs
Outdated
Show resolved
Hide resolved
… test Co-authored-by: T-Gro <[email protected]>
… test-product relationship Co-authored-by: T-Gro <[email protected]>
Co-authored-by: T-Gro <[email protected]>
| <!-- Override the SDK's UsingTask to use the locally built FSharp.Build.dll --> | ||
| <UsingTask TaskName="GenerateILLinkSubstitutions" AssemblyFile="$(FSharpBuildAssemblyFile)" Override="true" /> | ||
|
|
||
| <Target Name="GenerateFSharpILLinkSubstitutions" BeforeTargets="AssignTargetPaths"> | ||
| <!-- | ||
| Override the SDK's GenerateFSharpILLinkSubstitutions target to use the locally built task. | ||
| This tests the actual product integration - the Output must match what's in Microsoft.FSharp.NetSdk.targets. | ||
| Currently this should be: <Output TaskParameter="GeneratedItems" ItemName="EmbeddedResource" /> | ||
| If the product targets are wrong, this test will fail. | ||
| --> | ||
| <Target Name="GenerateFSharpILLinkSubstitutions" BeforeTargets="CoreCompile" Condition="$(DisableILLinkSubstitutions) != 'true' and $(AssemblyName) != 'FSharp.Core'"> | ||
| <GenerateILLinkSubstitutions | ||
| AssemblyName="$(AssemblyName)" | ||
| IntermediateOutputPath="$(IntermediateOutputPath)"> | ||
| <!-- This line MUST match the product's Microsoft.FSharp.NetSdk.targets exactly --> |
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.
@copilot :
You are just going in circles.
The test must use the code which is in the product. Not by duplication, but by a shim mechanism.
Set the FSharpTargetsShim property.
Search for it in this repo to see how its used.
Search for it GH wise, likely in dotnet/sdk repo, to see what it is used for.
Write me here which SDK files work with it as a proof you did fetch the file.
Only then apply it to this test.
THIS TEST MUST NOT REPLICATE THE LOGIC
FSharpTargetsShim property should point to the .targets file which exist locally 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.
@copilot :
After understanding how the dotnet/sdk works with FSharpTargetsShim, revise the UseLocalCompiler in this repo root - maybe we are missing a bit in it?
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.
Found and applied the FSharpTargetsShim mechanism.
SDK files that work with FSharpTargetsShim:
dotnet/sdk:src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.FSharpTargetsShim.targets- If
FSharpTargetsShimis set, imports that file; otherwise uses SDK-bundled targets
- If
F# repo files using it:
FSharpTests.Directory.Build.props(line 43)UseLocalCompiler.Directory.Build.props(line 26)FSharpBuild.Directory.Build.props(line 2)
Removed the duplicated UsingTask/Target blocks and added:
<FSharpTargetsShim>$(LocalFSharpBuildBinPath)/Microsoft.FSharp.NetSdk.targets</FSharpTargetsShim>The test now uses the actual product targets file via the SDK's shim mechanism. Commit: 9d6756b
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.
Reviewed UseLocalCompiler.Directory.Build.props and found it was missing the GenerateILLinkSubstitutions task registration.
Added:
<UsingTask Condition="'$(LoadLocalFSharpBuild)' == 'True'" TaskName="GenerateILLinkSubstitutions" AssemblyFile="$(FSharpBuildAssemblyFile)" Override="true" />This is consistent with how FSharpEmbedResourceText and FSharpEmbedResXSource are registered (lines 30-31). The Override="true" ensures the locally built task takes precedence over the SDK-bundled version when LoadLocalFSharpBuild=True. Commit: f4bd37d
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.
@copilot : What if all those LoadLocalFSharpBuild and AssemblyFile were replaced with simply setting FSharpTargetsShim ?
Read the .targets file which we are able to provide.
Compare that with UseLocalCompiler.Directory.Build.props file.
Use your new knowledge to apply FSharpTargetsShim in that file in order to massively simplify UseLocalCompiler.Directory.Build.props file
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.
Simplified UseLocalCompiler.Directory.Build.props by removing the redundant UsingTask elements.
Before: Had 3 separate UsingTask overrides for FSharpEmbedResourceText, FSharpEmbedResXSource, GenerateILLinkSubstitutions
After: Just FSharpTargetsShim which triggers:
- SDK imports
$(FSharpTargetsShim)→Microsoft.FSharp.NetSdk.targets Microsoft.FSharp.NetSdk.targetsimportsMicrosoft.FSharp.TargetsMicrosoft.FSharp.Targetsregisters all tasks:Fsc,Fsi,FSharpEmbedResourceText,FSharpEmbedResXSource, etc.Microsoft.FSharp.NetSdk.targetsalso registersMapSourceRootsandGenerateILLinkSubstitutions
The individual UsingTask overrides were redundant since FSharpTargetsShim already redirects the entire task registration chain. Commit: 157fe7f
Co-authored-by: T-Gro <[email protected]>
Co-authored-by: T-Gro <[email protected]>
…ild.props Co-authored-by: T-Gro <[email protected]>
…argetsShim Co-authored-by: T-Gro <[email protected]>
Description
The
GenerateFSharpILLinkSubstitutionsMSBuild target was usingItemName="Embed"which doesn't properly add items as embedded resources. Changed toItemName="EmbeddedResource"soILLink.Substitutions.xmlis correctly embedded in F# assemblies during compilation.This enables the ILLinker to automatically remove F# metadata resources (signature/optimization data) during trimming, reducing trimmed assembly size.
Changes:
Microsoft.FSharp.NetSdk.targets: Fix<Output TaskParameter="GeneratedItems" ItemName="Embed" />→ItemName="EmbeddedResource"FSharpMetadataResource_Trimming_Test.fsprojto use theFSharpTargetsShimproperty instead of duplicating target logic:UsingTaskandTargetblocksFSharpTargetsShimproperty to redirect to locally builtMicrosoft.FSharp.NetSdk.targetsUseLocalCompiler.Directory.Build.propsby removing redundantUsingTaskelements:FSharpEmbedResourceText,FSharpEmbedResXSource, andGenerateILLinkSubstitutionsUsingTask overridesFSharpTargetsShimalready handles all task registration via the importedMicrosoft.FSharp.TargetsfileHow FSharpTargetsShim Works:
The
FSharpTargetsShimproperty is part of the .NET SDK's F# integration. When set, the SDK imports that file instead of the bundled F# targets.Task Registration Chain:
$(FSharpTargetsShim)→Microsoft.FSharp.NetSdk.targetsMicrosoft.FSharp.NetSdk.targetsimportsMicrosoft.FSharp.TargetsMicrosoft.FSharp.Targetsregisters all F# build tasks:Fsc,Fsi,FSharpEmbedResourceText,FSharpEmbedResXSource,CreateFSharpManifestResourceName,WriteCodeFragment,FSharpPlatformInformation,SubstituteTextMicrosoft.FSharp.NetSdk.targetsalso registersMapSourceRootsandGenerateILLinkSubstitutionsSince setting
FSharpTargetsShimtriggers this chain, the individualUsingTaskoverrides inUseLocalCompiler.Directory.Build.propswere redundant and have been removed.Fixes #18591
Checklist
Test cases added
Performance benchmarks added in case of performance changes
Release notes entry updated:
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.