Fix Roslyn version mismatch causing CS8795 build failures (#970), harden PR CI - #971
Open
M-r-A wants to merge 32 commits into
Open
Fix Roslyn version mismatch causing CS8795 build failures (#970), harden PR CI#971M-r-A wants to merge 32 commits into
M-r-A wants to merge 32 commits into
Conversation
) The 5.3.0 analyzer packages require Roslyn 5.3.0+, which is only bundled in .NET SDK 10.0.300+. However, build-beta.yml specifies dotnet 9.x, which bundles Roslyn 4.14.0. This causes CS9057 analyzer loading failures and cascading CS8795 partial method implementation errors. Version 4.13.0 is compatible with Roslyn 4.14+ (bundled in 9.x SDKs) and also compatible with 10.x and 11.x SDKs, allowing builds to succeed on all.
Specify SDK 9.0.0 with rollForward: latestMinor to ensure all developers build against 9.0.x locally, matching the CI workflow's baseline version. This prevents accidental SDK mismatches between local and CI environments.
Add "Display SDK version" step to all pipeline workflows for better observability and troubleshooting: - build-beta.yml: Shows SDK in each matrix job (9.x, 10.x) - pr-check.yml: Shows SDK version when validating PRs - build-workshop.yml: Shows SDK when building releases This ensures CI logs explicitly show which .NET SDK version was used, making it easier to diagnose SDK-related issues and verify that intended versions are being tested.
Display the .NET SDK version used in the build for better observability and troubleshooting in CI logs.
Cache ~/.nuget/packages using packages.lock.json hash. Expected 70-90% cache hit rate, reducing workflow time by 2-3 minutes per run.
Cache ~/.nuget/packages using packages.lock.json hash. Expected 70-90% cache hit rate, reducing PR validation time by 2-3 minutes per run.
Test builds on ubuntu-latest, windows-latest, and macos-latest in parallel. Catches platform-specific issues (CRLF, path separators, line endings). Job names now display the OS being tested via matrix label. Uses fail-fast: false to report all platform failures, not just the first.
Add explicit TRX logger to dotnet test step so TestResults directory is created on all platforms. This allows the artifact upload step to successfully capture test results for failed runs, enabling post-mortem debugging across Ubuntu, Windows, and macOS.
Upgrade all 4 GitHub Actions to latest stable releases:
- actions/checkout: v4 → v7.0.1 (3d3c42e)
- actions/setup-dotnet: v4 → v6.0.0 (a98b568)
- actions/cache: v4 → v6.1.0 (55cc834)
- actions/upload-artifact: v4 → v7.0.1 (043fb46)
Pin by full commit SHA instead of floating tags to prevent supply-chain
risks and ensure reproducible builds. Add version tags in comments for
readability.
All major version bumps (up to 3 versions per action) are backward-compatible
for this workflow: runtime Node 20→24 (GitHub runners already support),
checkout fork-blocking doesn't apply (uses plain pull_request trigger),
and setup-dotnet/cache inputs unchanged.
Fix artifact collision bug: upload-artifact v4+ requires unique artifact names
when used in a matrix. Change fixed name "test-results" to include OS via
"test-results-${{ matrix.os }}" so 3 matrix legs produce 3 distinct artifacts
instead of colliding.
Neither the job nor any step had a timeout, so a hang anywhere in restore/build/test would run until GitHub's 6-hour default before being killed. We already saw Tests.ServerTest.Test() hang on macOS and only stop because NUnit's own 3s test-level timeout caught it, which is not a CI-level safety net. Cap the job at 15 minutes.
No concurrency group existed, so every push to a PR branch started a fresh 3-way OS matrix without cancelling the previous run. A few quick force-pushes could pile up 9+ simultaneous jobs. Group runs by workflow and ref, cancelling in-progress runs so only the latest push keeps running.
actions/upload-artifact defaults to if-no-files-found: warn. This is exactly why the earlier "No files were found with the provided path: **/TestResults/**" bug sat unnoticed in run logs instead of failing CI - we only caught it by reading warnings manually. Set to error so a missing-artifact regression fails the build instead of hiding in warnings.
No permissions block was set, so the workflow ran with whatever the repo/org default GITHUB_TOKEN scope is, which can be broad (e.g. contents: write). This workflow only checks out code and runs tests - it never needs write access. Confirmed against dotnet/aspnetcore: all 12 of their hand-authored workflows declare this explicitly, scoped to exactly what each one does, with zero exceptions.
The repo has no packages.lock.json anywhere and RestorePackagesWithLockFile
is not set, so hashFiles('**/packages.lock.json') in the manual
actions/cache step always evaluated to an empty string. Confirmed
directly in run 30247575118's logs: the cache key resolved to a bare
trailing dash ("key: Linux-nuget-", etc.) on all 3 legs, and Ubuntu's
post-job cleanup logged "Cache hit occurred on the primary key
Linux-nuget-, not saving cache." The cache was permanently frozen on
its first-ever save and would never update again.
Remove the manual actions/cache step and use actions/setup-dotnet's
built-in cache instead, pointed at cache-dependency-path: '**/*.csproj'
so the key changes whenever a package or version is added or bumped.
Same pattern dotnet/aspnetcore uses for npm caching via setup-node's
cache: 'npm' input.
Split configuration into a second matrix axis alongside os, so Debug and Release build and test in parallel instead of sequentially in one leg (3 OS x 2 config = 6 legs). Testing only Debug was a real gap: this repo ships Release builds (build-beta.yml), and Release-mode JIT optimizations can surface bugs that never reproduce in Debug. The test suite takes ~8s, so testing both configs is negligible added cost.
dotnet-version: 9.x was hardcoded in the workflow while global.json already pins the SDK to 9.0.0 with rollForward: latestMinor for local dev, so the two could silently drift apart. Add a sdk matrix axis: one leg reads the version from global.json (single source of truth, tracks whatever's pinned there automatically), the other explicitly targets 10.0.x to catch forward-compatibility issues against the next SDK early. rollForward: latestMinor keeps the global.json leg within 9.x, so the two legs stay meaningfully distinct. This is a third matrix axis alongside os and configuration (3 x 2 x 2 = 12 legs). Artifact names include the sdk leg to avoid collisions.
"9.0.0" isn't a valid full SDK version: Microsoft's global.json docs
require major.minor.featureband+patch (e.g. 10.0.100) and explicitly
list "10.0" as an invalid example producing this same error class.
actions/setup-dotnet's global-json-file parser enforces this strictly
and failed once F6 started actually reading this file in CI.
Use 9.0.316. rollForward: latestMinor is documented as major-version-
scoped ("matches the requested major"), so this still only ever
resolves within 9.x - it can't roll forward to 10.x.
The committed global.json always wins SDK resolution, so the "10" matrix leg either crashed (Windows) or silently tested the pinned version instead of 10.x (Ubuntu/macOS) - confirmed via run 30252784775 logs, where dotnet --version printed the wrong version on every "10" leg. Fix per https://github.com/actions/setup-dotnet#matrix-testing: rewrite global.json before Set up .NET runs, anchored to this leg's own major version.
Verified locally that dotnet format --verify-no-changes exits non-zero against the current codebase (236/449 files, mostly WHITESPACE/CHARSET violations) despite an existing .editorconfig, so gating on it would fail every PR regardless of what it touches. Add it as a separate single-run job (formatting doesn't depend on os/configuration/sdk, so running it 12x in the build matrix would be wasteful) with continue-on-error: true, so violations show up in the job's log without blocking merges until the existing debt is cleaned up separately.
Every step name is now imperative verb + object, sentence case, with short parallel qualifiers only where needed to disambiguate ((pinned)/ (override)) or flag job-level behavior ((non-blocking)). Renames the builds job to build-test / "Build, Test" since the old name undersold what it does, and names both checkout steps explicitly.
Both env vars were scoped only to the "Set up .NET" steps, not Restore/Build/Test. Per Microsoft's telemetry docs, DOTNET_NOLOGO only suppresses the disclosure text and "has no effect on telemetry opt out" - the opt-out itself is re-checked on every CLI invocation with no persisted/sentinel behavior. So telemetry was actually still being sent on every restore/build/test/format call across all legs, despite the apparent intent to disable it. Move both to the top-level workflow env block so they apply to every step in every job, matching actions/setup-dotnet's own documented usage pattern.
Researched actions/setup-dotnet issue rwmt#740 first: v5.3.0 had a bug where global.json + rollForward silently skipped installing a pinned prerelease SDK. Confirmed our pinned v6.0.0 SHA is 8 commits ahead of the fix (PR rwmt#742), so it doesn't affect us. Confirmed A.B.x version format and global.json's default allowPrerelease already include preview builds without extra config. Mark the '11' leg continue-on-error at the job level: previews can have real upstream bugs unrelated to this repo (e.g. a confirmed dotnet-tool crash in .NET 11 preview 2), and this leg exists to catch upcoming breaks early, not to gate merges on preview-SDK issues.
A generic version anchor + rollForward doesn't reliably match a preview-only install (dotnet/sdk#12335, reproduced for the "11" leg). Pin global.json to the exact resolved version instead, captured via setup-dotnet's own output. Also assert the resolved SDK's major version matches what each leg expects, so a repeat of the earlier silent wrong-SDK bug fails loudly instead of requiring a manual log read.
Neither job pushes anything, so there's no reason for the checked-out git config to retain a usable credential. Found in dotnet/aspnetcore's runtime-sync.yml during earlier research; reduces blast radius if a build step or dependency ever tries to misuse the token.
Add a package-artifacts job that builds the mod (Release) and publishes the server for win-x64/linux-x64, uploading both as plain workflow artifacts - no write permission needed, works for fork PRs. Runs once on ubuntu-latest rather than as a conditional matrix step.
path: output/Multiplayer/ strips "Multiplayer" itself and flattens its contents to the zip root - verified against actions/toolkit's zip spec logic. Stage each output under its own parent directory instead and point path: at that parent, so Multiplayer/ and Server/ survive as the zip's top-level folder.
Keeps zips traceable after download, once GitHub's run-scoped context is gone. E.g. Multiplayer-mod-pr123-a1b2c3d.
PR check workflow enhancements
Keep this workflow's only change to Display SDK version, as originally scoped - the caching addition didn't belong here. Also restores the "Setup Dotnet" step name, which an earlier commit renamed to "Setup .NET" as an unrelated side effect of matrix work that was since reverted.
test-results-* is keyed by matrix values, not run attempt, so re-running a flaky test leg without a new push hit a 409 conflict against the first attempt's artifact instead of replacing it. Scoped to test-results only - a package-artifacts failure is rarer and a fresh push is the more normal recovery path there.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #970: builds failed with cascading
CS9057/CS8795errors becauseSourceGen.csprojpinnedMicrosoft.CodeAnalysis.CSharp/.Analyzersto5.3.0, which requires Roslyn 5.3.0+ (bundled starting in .NET SDK 10.0.300+), while CI and most local setups run .NET 9.x, which only bundles Roslyn 4.14.0. Downgrading to4.13.0restores compatibility with 9.x while remaining forward-compatible with 10.x and 11.x — verified directly in CI across all three.Everything else in this PR is CI infrastructure that grew out of diagnosing and guarding against that failure: a pinned local SDK version so contributors don't hit environment drift, SDK-version visibility in every pipeline's logs, and a substantially hardened
pr-check.ymlthat turned up (and fixed) several pre-existing correctness bugs along the way.1. The actual fix
Source/SourceGen/SourceGen.csproj:Microsoft.CodeAnalysis.CSharp/.Analyzers5.3.0→4.13.0.2. Supporting changes (small, all three workflows)
global.json(new): pins the local/CI SDK baseline to9.0.100withrollForward: latestMinor, so contributors and CI resolve the same SDK family without needing an exact patch match.build-beta.yml,build-workshop.yml: added a one-lineDisplay SDK version(dotnet --version) step to each, purely for log visibility when diagnosing future SDK-related failures. No other behavior changes to either file.3.
pr-check.ymloverhaulThis is the bulk of the diff. Originally scoped as "add SDK visibility," it expanded into a full hardening pass once several real bugs surfaced during the work — each one confirmed via live GitHub Actions runs, not just locally.
Action & permission hygiene
actions/checkout,setup-dotnet,cache,upload-artifactto exact commit SHAs instead of floating tagspermissions: contents: readpersist-credentials: falseon checkout steps that never pushconcurrencygroup to cancel stale runs on new pushes to the same PRReliability fixes
packages.lock.jsonthat doesn't exist in this repo, so the cache silently locked onto its first-ever save and never updated again. Switched tosetup-dotnet's built-in cache keyed off**/*.csproj.if-no-files-founddefaulted towarn; a missing-artifact regression could pass CI unnoticed. Changed toerror.DOTNET_CLI_TELEMETRY_OPTOUTwasn't actually opting out of anything. It was scoped only to the SDK-setup step, not therestore/build/testinvocations that actually trigger telemetry. Moved to workflow-levelenv.timeout-minutes: 15added — a hung test previously had no CI-level backstop and could in theory run for hours.global.jsonwas silently overriding matrix SDK selection.global.jsonis read by thedotnetCLI for every invocation regardless of what a step installs — so when the matrix tried to test SDK 10/11, it either crashed or (worse) silently built against the wrong SDK the whole time, a false-positive green run. Fixed by rewritingglobal.jsonto each leg's resolved SDK before build steps, plus an explicit assertion step that now fails loudly if this regresses."11.0.100"withrollForwarddoesn't roll forward to an installed11.0.100-preview.6.xbuild even withallowPrereleaseon by default (matches dotnet/sdk#12335). Fixed by pinning to the exact resolved version string instead of a guessed one.Coverage
global.json-pinned 9.x, 10, 11-preview) = 18 legs. Release is included because this repo ships Release builds and Release-mode JIT optimizations can hide/reveal bugs Debug never shows..NET 11legs arecontinue-on-error: true— previews can carry real, unrelated upstream bugs; this exists to catch forward-compat breaks early, not to block merges on them.dotnet format --verify-no-changescheck. Verified locally first: the repo currently has pre-existing formatting debt (236/449 files) despite an existing.editorconfig, so this reports violations in the log rather than gating every future PR on unrelated debt.New: downloadable PR build artifacts
package-artifactsjob that builds the mod (Release) and publishes the dedicated server for win-x64/linux-x64, uploading both as workflow artifacts (Multiplayer-mod-pr<N>-<sha>,Multiplayer-server-pr<N>-<sha>) — no GitHub Release, no write permission needed, works for PRs from forks. Lets reviewers grab and test-install the exact build from any PR.Known pre-existing issue (unrelated to this PR)
The expanded matrix surfaced an intermittent flake in the networking test suite (
SendRaw() called with invalid connection state ... Disconnected, a disconnect race in server keep-alive ticks). It reproduces on both macOS (as a timeout) and Windows (as this assertion) and predates this PR — nothing here touches test infrastructure or timing. Flagging separately rather than folding a fix in here, since it's a product-code issue, not a CI one.