osprey: Pin LangVersion to 14, which the tree already requires - #4594
osprey: Pin LangVersion to 14, which the tree already requires#4594maccoss wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a build break in pwiz_tools/Osprey/Osprey.Scoring by restoring the missing LINQ import needed for SequenceEqual on string[] in PickLdaModel.LoadFromFile.
Changes:
- Add
using System.Linq;to enabledto.Features.SequenceEqual(ExpectedFeatures)to compile.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
* Osprey.Scoring/PickLdaModel.cs calls string[].SequenceEqual(string[]) with only "using System;" imported. That binds to System.MemoryExtensions.SequenceEqual through the C# 14 implicit span conversion in extension receiver position; under C# 13 and earlier the only candidate is System.Linq.Enumerable.SequenceEqual, which the file does not import. Osprey has therefore required C# 14, and the .NET 10 SDK, since dd9e845 * <LangVersion>latest</LangVersion> hid that: it resolves to whatever the installed SDK offers, so the same source built on VS 2026 and failed on a .NET 9 SDK with "CS1061: 'string[]' does not contain a definition for 'SequenceEqual'" pointing at unrelated-looking source. Naming the version fails instead as "CS1617: Invalid option '14' for /langversion", which says what is actually wrong * Matches how the rest of pwiz_tools declares this: Directory.Build.props pins 8.0 and several csproj files pin 7.3 or 8.0, rather than tracking the SDK Adding "using System.Linq;" was the other way to make the build work, but it is redundant under C# 14 and ReSharper flags it, so it would not survive inspection. Verified with the .NET 10.0.400 SDK: Osprey.sln builds clean, Osprey.Test passes 586/586 on net472 and net8.0. With the .NET 9.0.315 SDK the build now stops at CS1617. See ai/todos/active/TODO-20260820_osprey_scoring_linq_build_fix.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
928b319 to
8144386
Compare
|
Brendan is right - this was my machine, not master. I was building with the .NET 9 SDK. Root cause, reproduced on a clean That also explains the ReSharper warning - the using genuinely is redundant on your I have dropped the using and repurposed this PR to pin Re-verified on SDK 10.0.400: Two things I did not do, in the PR description: a repo-root Sorry for the noise - the branch name still says "linq_build_fix", which no longer |
|
Closing this - not needed. The build requirement is already VS 2026 and the .NET 10 SDK, so #4595 has been rebased off this branch and now targets Recording the diagnosis here since it cost some time and will cost the next person the on a line that looks perfectly fine, naming neither the SDK nor the language version. So Thanks Brendan for pushing back rather than letting the wrong fix through. |
* The repo-root global.json pins the SDK to the 8.0.4xx band, whose compiler tops out at C# 12, while build.ps1 drives the VS toolset, which offers C# 14. LangVersion "latest" therefore meant a different language to each of Osprey's two build paths: the package.ps1 publish failed CS1061 on PickLdaModel.cs's string[].SequenceEqual, which binds to MemoryExtensions only under C# 14, seconds after the other path compiled the same file and passed 586 tests. That is why TeamCity fails here and never on master, which has no root global.json * Added the using System.Linq that the C# 12 binding needs. Pinning is what makes it acceptable: ReSharper reports it redundant only while the language version is "latest", which is why PR #4594 dropped it and closed * Verified both paths: Osprey.sln compiles, 586/586 tests pass, the win-x64 publish succeeds, and the inspection reports nothing in PickLdaModel.cs See ai/todos/active/TODO-20260818_commonutil_winforms_split.md Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TjTPKoE2UJrjnvwtqrQZsT
What changed since the first version of this PR
The original premise was wrong, and Brendan was right to push back: master is not
broken. It builds and tests green on TeamCity, in VS 2026, and on his machine. The
failure was mine, from building with the .NET 9 SDK.
This PR is now a one-property change that makes the real requirement explicit.
Diagnosis
Osprey.Scoring/PickLdaModel.csvalidates a pick model's feature list:Both operands are
string[], and the file importsSystembut notSystem.Linq.string[]implicitly converts toReadOnlySpan<string>in extensionreceiver position (the "first-class span types" feature), so this binds to
System.MemoryExtensions.SequenceEqual, already covered byusing System;. Addingusing System.Linq;is redundant, which is why ReSharper flags it.System.Linq.Enumerable.SequenceEqual, which the file does not import, so it fails.So Osprey has required C# 14, and therefore the .NET 10 SDK, since dd9e845 introduced
the file.
<LangVersion>latest</LangVersion>concealed that: it means "whatever this SDKsupports", so the requirement was never stated and never checked.
Reproduced both directions on a clean
origin/masterworktree, no other change:latestresolves toCS1061: 'string[]' does not contain a definition for 'SequenceEqual'The diagnostic is the real problem here. It points at source that looks correct and says
nothing about language versions, so it reads as "master is broken" rather than "your SDK
is too old". That is exactly the wrong conclusion to hand a new contributor, or an agent.
The change
pwiz_tools/Osprey/Directory.Build.props:<LangVersion>latest</LangVersion>becomes<LangVersion>14</LangVersion>, with a comment recording what requires it.Now an SDK that cannot build Osprey says so directly:
This also matches how the rest of the tree declares it.
pwiz_tools/Directory.Build.propspins
8.0;CommonFileDialogs,CommonMsData,PanoramaClientandBullseyeSharppin8.0or7.3. Osprey was the outlier in tracking whatever SDK was installed.I have upgraded to the .NET 10.0.400 SDK, and everything below was re-verified on it.
Verification
Osprey.slnbuilds clean, 0 warnings, 0 errorsOsprey.Test586/586 on net8.0, 586/586 on net472Not done here, worth deciding separately
global.jsonwould state the SDK floor once instead of leaving each projectto imply it through
LangVersion. That is a repo-wide call, not an Osprey one.<LangVersion>latest</LangVersion>users (PortableUtil, theDevToolsprojects) may have picked up C# 13/14 dependencies the same way, without anyone
choosing to. Worth an audit.
#4595 is stacked on this branch and has been rebased onto it.
See
ai/todos/active/TODO-20260820_osprey_scoring_linq_build_fix.md.