Warn on an unpatched build; stop the smoke test hiding the Cecil resolver - #299
Conversation
PatchedFileOverlay.Apply now returns -1 when the assembly holds no overlay resources at all, which happens on a build without -p:EmbedPatchedFiles=true. The launcher prints a clear warning in that case instead of silently starting the server on the downloaded vanilla assemblies. A build that looks fine but runs unpatched vanilla is how #276 was hit.
The Stratum Cecil resolver freezes its search-directory list at construction, so a directory that appears later (a zip code mod unpacked mid-load, an output folder populated late) is never searched and the mod is dropped. A ResolveFailure handler re-enumerates the same paths and reads the assembly by name before Cecil throws, and returns null to let Cecil raise its own error if nothing matches. This adds a behavioural line to a previously fixup-only patch.
Drops the cd "$server_dir" wrapper the bash script used to route around the Cecil resolver: with the resolver in place the server reaches RunGame from repo_root, and running the test from a neutral directory is what keeps that covered (#276). Brings smoke-test.ps1 in line: it still preferred VintagestoryServer.exe, still built without -p:EmbedPatchedFiles=true, so Windows boot-tested unpatched vanilla whenever that binary existed.
The bare 'dotnet build VintageStory.slnx -c Release' in BUILDING.md and CONTRIBUTING.md, including the pre-PR gate step, produces a build that runs the downloaded vanilla assemblies unpatched. Every command now carries -p:EmbedPatchedFiles=true, with a line saying why.
Pixnop
left a comment
There was a problem hiding this comment.
The diagnosis is the best part of this: proving the run was on vanilla VintagestoryLib.dll instead of chasing the resolver a second time, keeping the negative control, and retracting two claims from the original issue. Dropping the cd "$server_dir" wrapper is right too, the workaround was hiding the exact condition the gate exists to catch.
Two things on the docs side. docs/BUILDING.md:19 and CONTRIBUTING.md:64 and :73 tell a fresh clone to run one dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=true. StratumServer.csproj:41-53 pulls the sibling outputs in by raw path with no ProjectReference, which is why Makefile:38-46 builds twice and pack-release.ps1 builds before it publishes. Straight after bootstrap, with nothing in the sibling bin folders yet, that single command can race the projects it embeds, and Windows readers have no make line to fall back on. Either document the two passes or give StratumServer a real dependency edge.
The body says every command now carries the flag. Five still do not: scripts/bootstrap.sh:541 and bootstrap.ps1:369 (the "Bootstrap complete. Run:" line), README.md:124, .github/PULL_REQUEST_TEMPLATE.md:16, and the first Makefile pass, which is intentional. The bootstrap line is the one people actually copy.
Smaller: the new warning goes to stderr, and scripts/smoke-test.ps1:99 sends stderr to smoke-test-err.log, which the script never reads (only $logFile, at 112 and 152). So on Windows the gate this PR tightens on Linux stays blind.
Two questions. In the ResolveFailure handler (patch line 60), should ReadAssembly be wrapped the way Cecil's own SearchDirectory catches BadImageFormatException and keeps looking, so a half-unpacked DLL in a mod folder does not end the search early? And PatchedFileOverlay.cs:29 returns -1 before the marker check, so a tree overlaid by an earlier flagged build is told it is about to run vanilla while the patched DLLs are still in place. Worth softening that wording?
Pixnop
left a comment
There was a problem hiding this comment.
Approving. What I would still fix is docs and body only: the two-pass build in BUILDING.md and CONTRIBUTING.md, the five flagless commands, and smoke-test.ps1 reading stderr. None of it blocks the code.
Re-review requestedThe requested changes are pushed and verified. Please re-review the current head before merge. |
Summary
The server in that run was executing vanilla
VintagestoryLib.dll, which has no Cecil resolver at all. The1fe074bModAssemblyLoaderfix is correct; it just was not loaded.Root cause
The repro's second line is
dotnet build VintageStory.slnx -c Releasewith no-p:EmbedPatchedFiles=true. Without that flagStratumServer.csprojembeds no overlay resources,PatchedFileOverlay.Applyreturns without a word when it finds none, andVanillaBootstrap's downloadedVintagestoryLib.dllis what runs.scripts/smoke-test.shonly auto-builds when the binary is missing, so its own-p:EmbedPatchedFiles=truefallback never fired.Vanilla
ModAssemblyLoader.LoadAssemblyDefinitionisAssemblyDefinition.ReadAssembly(path)with noReaderParameters. Mono.Cecil 0.11.6'sBaseAssemblyResolverthen searches only{ ".", "bin" }, relative to the process working directory, which wasrepo_rootforbash scripts/smoke-test.shat the time this was filed.VintagestoryAPI.dlllives in the server output directory, which is neither.Reproduced against the real
Lib/Mono.Cecil.dlland the builtMods/VSCreativeMod.dll:The vanilla path succeeds only when run from the server's own output directory. The Stratum resolver succeeds from every directory tested.
Two corrections to the original diagnosis: the resolver is not version-strict (resolving
VintagestoryAPI, Version=9.9.9.9from a directory holding 1.22.7.0 returns the 1.22.7.0 assembly), and the mod is not dropped silently,ModContainer.LoadModInfo's catch logs it, which is where the stack trace in the issue came from. What was missing was any signal that the build was unpatched.A correctly built tree (
make build, ordotnet build ... -p:EmbedPatchedFiles=true) reaches RunGame from a neutral working directory with zero resolve errors on currentindev.Change
StratumServer/PatchedFileOverlay.cs,StratumServer/Program.cs,Applyreturns-1when the build carries no embedded patched files, distinct from0for "already current". The launcher then prints a warning that the server is about to run the vanilla assemblies unpatched and how to rebuild.scripts/pack-release.ps1always passes the flag, so this never fires in a shipped build.patches/VintagestoryLib/Vintagestory.Common/ModAssemblyLoader.cs.patch, the Stratum Cecil resolver freezes its search-directory list at construction. AResolveFailurehandler re-enumerates the same paths at failure time (covering a directory that appears later: a zip code mod unpacked mid-load, an output folder populated late) and reads the assembly by name before Cecil throws. Returns null to let Cecil raise its own error if nothing matches. This turns a previously fixup-only patch into one with a marked behavioural change.scripts/smoke-test.sh, drops thecd "$server_dir"wrapper (added to route around this). The server reaches RunGame fromrepo_root, and running the test from a neutral directory keeps the resolver covered on every run: a machine where it genuinely fails now fails the gate instead of passing via the workaround.scripts/smoke-test.ps1, same three latent preconditions this issue needs, none of which were ever fixed here: it preferredVintagestoryServer.exe, built without-p:EmbedPatchedFiles=true, and (like the new.sh) launches from the repo root. First two fixed; the launch directory is already neutral, which is now the intended behaviour. Untested from here, no Windows runner in this environment.docs/BUILDING.md,CONTRIBUTING.md, the baredotnet build VintageStory.slnx -c Release, including the pre-PR gate step, produces the unpatched build this issue is an instance of. Every command now carries-p:EmbedPatchedFiles=truewith a line saying why.Gates
dotnet build VintageStory.slnx -c Releasethen... -p:EmbedPatchedFiles=true: green, zero new warnings on the touched files (the 4 solution warnings are the pre-existing vanillaNU1904).bash scripts/smoke-test.shfromrepo_rootwith thecdwrapper removed:PASS: server reached RunGame, no fatal errors, 8 console command(s) verified.3 mods loaded, zeroFailed to resolve assemblyin the boot log.scripts/extract-patches.sh: the only patch changed isModAssemblyLoader.cs.patch. Nosources/change.Limitations
indev. It is reproducible under the repro's exact command sequence, which is the condition the issue describes and which the doc and warning changes remove.smoke-test.ps1changes ship untested (mechanical, mirrorsmoke-test.sh).LoadAssetson/mnt/ccan exceed the default 60s stall patience. That is aSMOKE_TEST_PATIENCEmatter, not a resolver regression; a warm run passes well under it. Bumping the default is separate scope.NullReferenceExceptioninWorldConfig.loadWorldConfigValuesFromPlaystyledownstream of "no mods loaded" is left alone deliberately. It is a symptom of an already-unusable server state, andWorldConfig.cs.patchis currently fixup-only. Worth its own issue: "no-mods-loaded should fail with a diagnostic, not an NRE."Type
Checklist
scripts/extract-patches.shran clean (only the one patch this change touches).dotnet build VintageStory.slnx -c Release -p:EmbedPatchedFiles=trueis green.// Stratummarker (ModAssemblyLoader.cs;StratumServer/*is Stratum-only).Related issues
Fixes #276