-
Notifications
You must be signed in to change notification settings - Fork 833
Fix early/unconditional execution of PackageFSharpDesignTimeTools target #18929
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
✅ No release notes required |
Co-authored-by: T-Gro <[email protected]>
…tion guide Co-authored-by: T-Gro <[email protected]>
…to-end tests Co-authored-by: T-Gro <[email protected]>
Co-authored-by: T-Gro <[email protected]>
tests/EndToEndBuildTests/DesignTimeProviderPackaging/TestDesignTimeProviderPackaging.cmd
Show resolved
Hide resolved
…s job Co-authored-by: T-Gro <[email protected]>
…BasicProvider The tests were too sophisticated, trying to check binlogs for target execution. Simplified to match BasicProvider's approach: - Build first with dotnet build - Pack with --no-build - Check package contents only Also added note that tests require F# compiler to be built first (artifacts/bin/fsc must exist). This resolves the "Build target not found" errors - the issue was that pack without build was trying to call Build/BuiltProjectOutputGroup targets that weren't available when F# SDK wasn't fully resolved. Co-authored-by: T-Gro <[email protected]>
As discussed, binlog checking is problematic: 1. Binary format can't be searched with findstr/strings reliably 2. Package contents are the actual validation we care about Changed all tests to only verify package contents (presence/absence of tools/fsharp41 folder): - Test 1: PlainLib should NOT have tools folder - Test 2: Provider should HAVE tools folder - Test 5: RedirectLib should NOT have tools folder This matches BasicProvider's testing approach and provides a reliable validation that the target ran (or didn't run) as expected. Co-authored-by: T-Gro <[email protected]>
Fixed in commit (pending). Removed unreliable binlog checking with findstr/strings on binary files. Tests now only validate package contents - checking for presence/absence of tools/fsharp41 folder which directly shows whether the target ran. |
|
||
<Target Name="PackageFSharpDesignTimeTools" DependsOnTargets="_GetFrameworkAssemblyReferences"> | ||
<Target Name="PackageFSharpDesignTimeTools" AfterTargets="ResolveReferences"> | ||
<CallTarget Targets="_GetFrameworkAssemblyReferences" Condition="'@(ReferencePath)' == ''" /> |
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.
Generally direct usage of CallTarget
is not ideal - if you need to express a data-dependency then that should be done via DependsOnTargets
. Why does this call need to happen, does ResolveReferences
not implicitly do _GetFrameworkAssemblyReferences
?
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.
That was for the scenario of calling pack --no-build
with a TP, its mainly a safety net just in case _GetFrameworkAssemblyReferences
was not executed.
(also, since its underscored, I/Copilot assumed it is "private" and that we should not depend on any current behavior and/or ordering)
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.
But it might be superfluous.
The main motivation was to get rid of DependsOnTargets
which was causing a bug (dragging this target in when not needed), and make sure it is really only executed when needed.
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.
Pull Request Overview
This PR fixes the early/unconditional execution of the PackageFSharpDesignTimeTools
target that was causing build issues, specifically NETSDK1085 errors and binding redirect problems. The fix implements a hybrid approach: (1) gating the target's inclusion in TargetsForTfmSpecificContentInPackage
based on the IsFSharpDesignTimeProvider
property at evaluation time, and (2) adding an experimental execution-time check for references with design-time provider metadata.
Key Changes:
- Modified target scheduling from
DependsOnTargets
toAfterTargets
with conditional safety call - Added evaluation-time property check to conditionally include the packaging target
- Added experimental execution-time target to check references for design-time provider metadata
- Created comprehensive end-to-end tests validating package contents
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/FSharp.Build/Microsoft.FSharp.NetSdk.targets |
Core fix: conditional target inclusion and modified scheduling |
tests/EndToEndBuildTests/EndToEndBuildTests.cmd |
Integrated new test suite into CI |
tests/EndToEndBuildTests/Directory.Build.props |
Removed duplicate import causing double-import issue |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/TestDesignTimeProviderPackaging.cmd |
Windows test script validating 5 scenarios |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/TestDesignTimeProviderPackaging.sh |
Unix test script validating 5 scenarios |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/PlainLib/* |
Test project: plain library without provider flag |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/Provider/* |
Test project: library with IsFSharpDesignTimeProvider=true |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/Host/* |
Test project: library with ProjectReference to provider |
tests/EndToEndBuildTests/DesignTimeProviderPackaging/RedirectLib/* |
Test project: library with AutoGenerateBindingRedirects |
docs/release-notes/.FSharp.Compiler.Service/10.0.100.md |
Added release note entry |
azure-pipelines-PR.yml |
Added SDK installation step for EndToEndBuildTests |
echo === Test 5: Binding Redirect / App.config Interaction === | ||
echo [Test 5] Testing with AutoGenerateBindingRedirects (MSB3030/app.config regression test)... | ||
echo [Test 5] Command: dotnet pack RedirectLib\RedirectLib.fsproj -o %~dp0artifacts -c %configuration% -v minimal -bl:%~dp0artifacts\redirect.binlog -p:FSharpTestCompilerVersion=coreclr | ||
dotnet pack RedirectLib\RedirectLib.fsproj -o %~dp0artifacts -c %configuration% -v minimal -bl:%~dp0artifacts\redirect.binlog -p:FSharpTestCompilerVersion=coreclr |
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.
This tests the tests/EndToEndBuildTests/DesignTimeProviderPackaging/RedirectLib/RedirectLib.fsproj project based on your issue report.
Could you please let me know if you have any additional thoughts on testing, before we merge this in?
Summary
Fixed the early/unconditional execution of the PackageFSharpDesignTimeTools target that was causing:
PackageFSharpDesignTimeTools
target interacts unpredictably with the rest of the build. #18924: Early inclusion caused premature reference resolution → binding redirect + app.config deletion issues--no-build
#12320: dotnet pack --no-build could trigger unintended build target participation (NETSDK1085 risk)Current Implementation (Experimental Hybrid Approach):
Dual-Phase Approach:
Clean Target Structure:
DependsOnTargets="_GetFrameworkAssemblyReferences"
toAfterTargets="ResolveReferences"
<CallTarget Targets="_GetFrameworkAssemblyReferences" Condition="'@(ReferencePath)' == ''" />
Simplified End-to-End Tests: Test suite validates via package contents only:
dotnet build
dotnet pack --no-build
Testing Verified:
IsFSharpDesignTimeProvider="true"
: tools/fsharp41 folder present in packageExperimental: The target-based reference checking approach tests MSBuild's behavior with execution-time property updates.
Note: Tests require the F# compiler to be built before running (EndToEndBuildTests job must build compiler first).
This pull request was created as a result of the following prompt from Copilot chat.
This pull request was created as a result of the following prompt from Copilot chat.
This pull request was created as a result of the following prompt from Copilot chat.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.