pwiz: Fixed four vendor readers failing from an installer-based install - #4640
Open
chambm wants to merge 5 commits into
Open
pwiz: Fixed four vendor readers failing from an installer-based install#4640chambm wants to merge 5 commits into
chambm wants to merge 5 commits into
Conversation
6 tasks
chambm
added a commit
that referenced
this pull request
Sep 8, 2026
* Skipped non-Windows natives when staging: msconvert's bin publishes for both RIDs, so the payload carried libMassLynxRaw.so, libbaf2sql_c.so and libtimsdata.so - licensed vendor SDKs the staging filter exists to exclude - plus libhdf5's .so/.dylib set and the Linux apphosts, 46 MB of 97 MB * Stopped Verify-VendorCache throwing on a surviving vendor_api directory; the loader's own flatten deletes it best-effort and keys only off the .ok marker, so asserting it made the tolerant retry unreachable * Stopped treating leftover files as a failed flatten, since the flatten skips x86/mips on purpose - 18 such files for Shimadzu, 8 for Agilent, 3 for ABI Found by /code-review max on #4640. See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
chambm
added this pull request to stack #4647
September 9, 2026 17:25
* Corrected three vendor-SDK pins: Bruker claimed Agilent's MHDAC prefixes, MIDAC carried a trailing dot that matches nothing, and Mobilion listed pwiz-sharp's own shim * Loaded MBI_SDK by full path before the first MobilionShim P/Invoke, which imports it statically where no resolver can see it * Resolved the wiff2 SDK from the vendor cache when it is not app-local, since LoadFromAssemblyPath consults no resolver * Staged the VC140 runtime app-local for Shimadzu, as Agilent already does for VC120 See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
* Added build.ps1 -WithVendorSdks: the bundled-runtime installer plus every Windows vendor SDK pre-extracted into VendorSdkLoader's cache, with a ProgramData marker pointing the app at it * Wired --with-vendor-sdks through build.bat and tcbuild.bat, opt-in so a developer build does not pay for it * Extended Installer.Tests from 4 fixtures across 3 vendors to 9 across 8, and made it accumulate failures instead of aborting on the first * Renamed ProteoWizard-Sharp to ProteoWizard across the installer See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
…I build * Made a failed installer/build.ps1 fatal in build.bat; that branch is only reached when ISCC is present, and warning instead let Installer.Tests go Inconclusive and the build go green with the vendor coverage never run * Retried the vendor_api cleanup, which threw "The directory is not empty" on a TeamCity EC2 agent over a tree holding nothing but empty directories * Kept a leftover file fatal there, since that means the flatten itself failed See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
* Skipped non-Windows natives when staging: msconvert's bin publishes for both RIDs, so the payload carried libMassLynxRaw.so, libbaf2sql_c.so and libtimsdata.so - licensed vendor SDKs the staging filter exists to exclude - plus libhdf5's .so/.dylib set and the Linux apphosts, 46 MB of 97 MB * Stopped Verify-VendorCache throwing on a surviving vendor_api directory; the loader's own flatten deletes it best-effort and keys only off the .ok marker, so asserting it made the tolerant retry unreachable * Stopped treating leftover files as a failed flatten, since the flatten skips x86/mips on purpose - 18 such files for Shimadzu, 8 for Agilent, 3 for ABI Found by /code-review max on #4640. See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
chambm
force-pushed
the
Skyline/work/20260904_vendor_resolution_fixes
branch
from
September 9, 2026 17:25
13ad422 to
f1f64f3
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Installer.Tests’ new “accumulate failures” logic still aborts on non-assert exceptions, and should be adjusted so it consistently reports all vendor failures as intended.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes multiple vendor-reader failures that only occurred in installer-based installs (where vendor SDK DLLs are resolved from the VendorSdkLoader cache rather than app-local), and improves CI/packaging to reliably build and validate installer artifacts (including an offline-friendly vendor-bundled installer variant).
Changes:
- Correct vendor SDK pin prefixes and adjust vendor SDK extraction/resolution so installed builds reliably load Sciex wiff2 and Mobilion dependencies from the cache.
- Add support for an optional “WithVendorSdks” installer variant that pre-populates the shared vendor cache and stamps a cache-root override for offline/container installs.
- Make installer build failures fatal in CI and improve installer payload staging/cleanup behavior.
File summaries
| File | Description |
|---|---|
| pwiz-sharp/tcbuild.bat | Forces CI builds to include --with-vendor-sdks when invoking build.bat. |
| pwiz-sharp/pwiz/test/Installer.Tests/InstallerTests.cs | Expands installer smoke coverage to more vendors and reports multiple vendor failures in one run. |
| pwiz-sharp/pwiz/src/Vendor/Shimadzu/Shimadzu.csproj | Deploys VC140 runtime DLLs app-local for Shimadzu native dependencies. |
| pwiz-sharp/pwiz/src/Vendor/Sciex/Wiff2LoadContext.cs | Resolves Sciex SDK directory from app-local or VendorSdkLoader cache for installed builds. |
| pwiz-sharp/pwiz/src/Vendor/Sciex/Sciex.csproj | Adds a direct reference to Vendor.Common to support explicit cache extraction for wiff2 SDK loading. |
| pwiz-sharp/pwiz/src/Vendor/Mobilion/MobilionShimNative.cs | Ensures MBI_SDK.dll is loaded by full path from the vendor cache before P/Invoke loads MobilionShim.dll. |
| pwiz-sharp/pwiz/src/Vendor/Mobilion/Mobilion.csproj | Adds a direct reference to Vendor.Common to support explicit cache extraction for Mobilion. |
| pwiz-sharp/pwiz/src/Vendor/Bruker/Bruker.csproj | Updates comment describing Vendor.Common direct usage (now shared with Sciex). |
| pwiz-sharp/installer/Setup.iss | Renames product to ProteoWizard and adds an optional vendor-cache install + cache-root stamping for offline installs. |
| pwiz-sharp/installer/build.ps1 | Adds -WithVendorSdks pipeline, improves staging filters (non-Windows payload trimming), and stages msconvert/wiff2 outputs explicitly. |
| pwiz-sharp/installer/build-linux.sh | Renames Linux artifact names to match ProteoWizard naming. |
| pwiz-sharp/build/vendor-sdk-pins.json | Fixes incorrect prefixes (Bruker/Agilent/Mobilion) that broke SDK resolution from the cache. |
| pwiz-sharp/build.bat | Adds --with-vendor-sdks and makes installer build failures fatal to prevent false-green CI runs. |
Review details
- Files reviewed: 7/13 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+439
to
+442
| catch (AssertFailedException ex) | ||
| { | ||
| failures.Add($"{vendor}/{name}: {ex.Message}"); | ||
| } |
Comment on lines
+164
to
+166
| ; uninsneveruninstall because the cache is keyed by SDK version, not by app | ||
| ; version, so side-by-side pwiz-sharp installs share these directories — the | ||
| ; same last-installed-wins / leave-it-alone policy the Explorer verbs use. |
…n machine * Staged the app-local VC++ runtime into the vendor cache at extraction time: a cache DLL is loaded by full path, which drops the application directory from the native search order, so BaseTof.dll could not find the MSVCR120 it imports * Applied it to every vendor, and matched the build-time bundled cache to the same set, so an offline installer cannot disagree with a downloaded one * Cost 0.6 MB on the installer; the eight identical copies compress away Found by the extended Installer.Tests on a clean CI agent. See TODO-20260901_net10_wine_container.md in pwiz-ai/todos Co-Authored-By: Claude <noreply@anthropic.com>
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
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.
Summary
Three related pieces, in the order they were found. #4641 has been merged into this branch, so
this is the single PR on top of #4619.
1. Four vendor readers failed from an installer-based install
All pre-existing. None reproduce in a dev or CI build: those keep the vendor DLLs app-local,
and
VendorSdkLoader's resolvers prefer app-local over the cache. All four were reproduced onnative Windows with the stock installer before being fixed.
vendor-sdk-pins.json.BaseCommon,BaseDataAccess,BaseError,BaseTof) although it ships exactly two DLLs,baf2sql_candtimsdata.FindPinreturns the first prefix match and Bruker precedes Agilent, so aBaseCommonbind resolved to the Bruker archive: "extracted cleanly but contains no BaseCommon.dll".
MIDAC.never matched: prefixes areStartsWithagainst the assembly simple name, andthe assembly is plain
MIDAC. Agilent then failed one assembly later on "Could not loadMIDAC".
MobilionShimis pwiz-sharp's own shim, not something the Mobilion archive carries, solisting it made the staging filter strip a binary nothing could supply.
MobilionShimNativeloadsMBI_SDK.dllby full path in a static constructor.MobilionShimlinksMBI_SDK.lib, so it is a static import resolved by the OS loader —a path no
DllImportResolversees. The error named the wrapper, not the missing dependency.Wiff2LoadContextresolves the wiff2 SDK from the cache when it is not app-local.LoadFromAssemblyPathconsults no resolver. Same shape as Bruker'sCompassXtractActivationContext, which callsEnsureExtractedexplicitly for the same reason.Shimadzu.csprojdeploys VC140 app-local, asAgilent.csprojdoes VC120 andBruker.csprojVC90. A missingMFC140makes the reader return a file with no spectrarather than an error, so any machine carrying the redistributable hides the dependency.
2. A vendor-bundled installer variant (was #4641)
build.ps1 -WithVendorSdksproducesProteoWizard-WithVendorSdks-Setup-<ver>.exe(103.8 MB): the bundled-runtime installer plus every Windows vendor SDK, pre-extracted into
the layout
VendorSdkLoader.EnsureExtractedwould have produced,.okmarkers included, sothe loader neither downloads nor extracts.
Setup.issinstalls that cache to%PROGRAMDATA%\ProteoWizard\vendorand stampsvendor-cache-root.txt, which is what redirects the loader off its per-user default.--with-vendor-sdksthroughbuild.batandtcbuild.bat— opt-in, so a developer builddoes not pay the ~30 s / ~26 MB.
Installer.Testsextended from 4 fixtures across 3 vendors to 9 across 8, and made toaccumulate failures rather than abort on the first. This is what found everything in part 1.
ProteoWizard-SharptoProteoWizardacross the installer.3. A failed installer build was reported as a successful CI build
Found on this PR's own CI run (build 4164785, agent
EC2AMAZ-HH9KVFB):So the installer never got built, both
Installer.Testswent Inconclusive, and the build wentgreen — with the coverage that gates part 1 never actually running.
build.bat: a failedinstaller/build.ps1is now fatal. That branch is only reachedwhen ISCC is present (a machine without it takes the
else), so a failure there is real.build.ps1:244: thevendor_apicleanup retries, and distinguishes what matters. Bythat point everything wanted has been moved out and the directory holds only the x86/mips
leftovers — for Waters, two empty directories, deepest path 108 chars, no reparse points,
nothing read-only. Aborting the installer build over that was the wrong trade. It now retries
5x with backoff; leftover files still throw (the flatten genuinely failed), leftover
empty directories warn and continue.
The delete failure is agent-specific, not a defect in the flatten: two builds of the same
commit with the same step passed on
MacCoss TeamCity Agent 1and failed only on the EC2agent, whose volume behaves unlike local NTFS (the same class of difference that makes
std::filesystem::canonicalthrow there).Test plan
pwiz-sharp/build.bat Release --i-agree-to-the-vendor-licenses --with-vendor-sdks— build succeeded, all three installers produced (78.2 / 21.5 / 103.8 MB), every suite green: Agilent 17, Waters 21, Bruker 15 +1, Thermo 15, Sciex 8, Mobilion 2, Shimadzu 2, UIMF 2, UNIFI 44, BiblioSpec 142, Analysis 177, MsData 85, Common 58, Util 36, plus the restinstaller/build.ps1 -SkipBuild -WithVendorSdkstwice in a row — the reported failure was a second-run reportbuild.batcontrol flow verified separately —gotoout of two nested parenthesized blocks under delayed expansion skips the guard line and propagates the exit code; CRLF endings intactInstaller.Testsper-user fixture — 9 fixtures across 8 vendors from the real installer artifact--network none: 43/43 converted, 40/43 count-match (the 3 diffs areReaderTestConfig::indexRange-truncated references, not regressions)Install_PerMachine_DeploysAndConvertsVendorFileis skipped locally — needs elevationNotes for reviewers
Two things found while investigating, deliberately not fixed here:
Lockmass_..._Cwt_AppliedAndUnappliedfailed once withMassLynx combineScan failed (code 5): File Open Error, then passed 3/3 in isolation and ina clean full re-run.
ATEHLSTLSEK_profile.rawis shared byAnalysis.TestsandWaters.Tests,Run-Tests-Parallel.ps1runs suites as concurrent jobs, and the Waters SDKwrites
lmgt.infinside the.raw. Fixing it means serializing those two suites or givingeach a private copy of the fixture.
Shallow clone enforced, and every vendor pincollapses to the build SHA (
b10f12fe7fe4for all of them) where a full clone gives eacharchive its own. Not a correctness bug — the generated
.csand the cache agree within abuild — but the cache key churns every commit on CI instead of tracking archive content.
Co-Authored-By: Claude noreply@anthropic.com