[msbuild-quality] Guard ValueTupleImplicitPackageVersion default so consumers can override it - #6
Conversation
The implicit System.ValueTuple package version was assigned unconditionally in Microsoft.FSharp.NetSdk.props, so a project setting $(ValueTupleImplicitPackageVersion) had its value silently discarded. Add the standard Condition="'$(Prop)' == ''" default guard, matching every other overridable default in this file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
🤖 This is an automated response from Repo Assist. I independently verified the premise of this PR against HEAD The core claim holds, with one correction worth folding into the PR description. Repro — a folder containing: <!-- Directory.Build.props -->
<Project><PropertyGroup><ValueTupleImplicitPackageVersion>9.9.9</ValueTupleImplicitPackageVersion></PropertyGroup></Project><!-- a.fsproj -->
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup><TargetFramework>net472</TargetFramework></PropertyGroup>
</Project>
Correction: the PR body states "There is no way to pin a different <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup><TargetFramework>net472</TargetFramework><ValueTupleImplicitPackageVersion>7.7.7</ValueTupleImplicitPackageVersion></PropertyGroup>
</Project>→ So the affected scenario is specifically repo-wide pinning via I have not built the compiler to validate the change end-to-end; CI is the right place for that. Leaving the merge decision to a human maintainer.
Add this agentic workflows to your repoTo install this agentic workflow, run |
Found by the MSBuild file-quality review of the shipped F# SDK build logic.
Problem
src/FSharp.Build/Microsoft.FSharp.NetSdk.propsassigns the implicitSystem.ValueTuplepackage version unconditionally:This file is imported by every F# project, and the property feeds the two implicit
System.ValueTuplePackageReferenceitem groups in the same file. Because the assignment has no condition, a project (or aDirectory.Build.props) that sets$(ValueTupleImplicitPackageVersion)has its value silently discarded — MSBuild's property pass always runs this line last within the import, before the item pass reads the property. There is no way to pin a differentSystem.ValueTupleversion short ofDisableImplicitSystemValueTupleReference=true.Every other overridable default in this file (
EnableDefaultCompileItems,Prefer32Bit,WarningLevel,FsiExec,FscToolPath,DisableImplicitFSharpCoreReference, ...) already carries theCondition="'$(Prop)' == ''"guard, so this one is the odd one out rather than a deliberate hard override — there is no comment suggesting otherwise.Change
Add the standard default guard:
One line, no behavioural change for any project that does not set the property: the default value and evaluation order are identical. The only difference is that an explicit value now survives.
Validation
XML well-formedness checked locally. A full
./build.shexceeds this workflow's timeout, so full validation is deferred to the PR CI checks — keeping this as a draft for a human to sign off.Related
The remaining findings from this review pass are unchanged from #4 (
CoreCompileDependsOnoverwrite, missing@(FileWrites)registration inGenerateFSharpILLinkSubstitutions, shim import guards). Those all require judgement about intent or target ordering and are deliberately not auto-fixed here.