Fall back to DLL presence for vcruntime detection - #239
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
frankea
left a comment
frankea
left a comment
There was a problem hiding this comment.
The scoping and gating are exactly as agreed, the tests are good, and the app-target change compiles and reads cleanly. The premise is the problem, and I only found it by probing real prefixes.
Wine ships msvcp140.dll and vcruntime140.dll as builtin PE DLLs and wineboot copies them into every prefix. A fresh prefix booted by the stock engine, with winetricks never run, has all four files in system32 and syswow64 carrying the "Wine builtin DLL" stub. Running your detectsInstallation against a bottle in that state returns true, so on the merged tree every bottle whose log lacks vcrun2019 flips to Installed (heuristic), and DependencyConfigSection hides the Install button for installed dependencies. Users would lose the ability to install the runtime from the panel, and the post-install verification would report success regardless of outcome. The unit tests pass because they create empty files with those names, which is the ambiguous case.
It gets more subtle: on a bottle here where a real redist did run (via a game installer), vcruntime140.dll and mfc140.dll are Microsoft's, but msvcp140.dll is still the Wine builtin. winetricks documents that setup refuses to replace it. So "non-builtin msvcp140" would be wrong in the other direction.
Fix direction: use winetricks' own marker. The vcrun2019 verb sets installed_file1 to system32/mfc140.dll (Libraries/winetricks:13360), which Wine does not ship, is absent in fresh prefixes, and is present only after the x64 redist really ran. Probe for that (and mfc140u.dll if you want to match the verb's second check) instead of the two CRT DLLs. If you would rather keep a DLL-signature approach, rejecting files whose DOS stub contains "Wine builtin DLL" or "Wine placeholder DLL" also works, but the marker is simpler and matches what winetricks itself trusts.
When you push the rework, rebase onto main first: #237 landed a changelog entry at the same spot as yours, keep both. Tests should cover the fresh-prefix case explicitly (builtin CRT DLLs present, no mfc140.dll, must report not installed).
…e detection The winetricks.log entry for a verb is only written once the verb's installer exits, and the vc_redist installer is known to hang under wine after installing successfully. The runtime can therefore be fully in place while verb-based detection keeps reporting "Not Installed" (frankea#233). When (and only when) the vcruntime verbs are missing from the log, probe for winetricks' own installed-file marker for vcrun2019, system32/mfc140.dll, and report the dependency installed with heuristic confidence. Wine ships msvcp140.dll and vcruntime140.dll as builtins in every fresh prefix, so those cannot serve as the signal; mfc140.dll is only ever placed by a real redist install. Bottles whose log already says installed are never probed. Scoped to the vcruntime definition.
1b1411b to
186ecbb
Compare
|
Reworked as suggested — thanks for probing the real prefixes, that builtin detail would have bitten hard. The probe now keys on winetricks' own installed-file marker for vcrun2019: Tests updated accordingly; the new key case is the one you described — a fresh prefix carrying the Wine-builtin msvcp140.dll and vcruntime140.dll in system32 and syswow64 must not count as installed. Scoping and gating are unchanged (vcruntime only, probe only when the log lacks the verb). One drive-by in the CHANGELOG: a stray |
|
the marker file is right, the directory is wrong on win64 prefixes.
the problem is which directory winetricks actually writes it to. that also collides with the failure in #233 specifically. vcrun2019 installs the x86 redist first, then the x64 half, and the reported symptom is the installer hanging. a bottle where the x86 half landed and the x64 half hung ends up with on my win64 bottle with the runtime in place, mfc140.dll is in both directories, so accepting either would not cost accuracy here: suggest probing either directory and inverting that test, or keying the path off the bottle's arch if you would rather mirror winetricks exactly. if you targeted the x64 half on purpose then say so and i will drop it, but the doc comment reads as though system32 is winetricks' own path, and on win64 it is not. |
|
Confirming @dappermint's read, and owning the source of the confusion: the system32 pointer came from my review. I cited Direction, though: keep the system32 probe, and make it deliberate. On win64 the x64 redist writes its mfc140.dll into system32, so the probe as written keys on the x64 half's real payload rather than winetricks' bookkeeping marker, and the bottles above show it matches on full installs (both directories populated). The syswow64-alone state is the one worth being strict about: that is the #233 partial install (x86 landed, x64 hung), and calling it installed would hide the Install button exactly where a rerun (unattended and forced since #237/#244) is the repair path, and would let post-install verification report success after a hung x64 half. On win32 bottles system32 is the x86 location anyway, so the probe stays correct there. So: no directory change needed, and |
The comments claimed system32/mfc140.dll is winetricks' own installed_file1 marker. That reading only holds on win32 prefixes; on win64 W_SYSTEM32_DLLS_WIN expands to syswow64, and the system32 file is the x64 redist's payload. The probe and the tests are unchanged; the docs now state the intent: syswow64-alone is the frankea#233 partial install (x86 landed, x64 hung) and must stay repairable.
|
Doc comments rewritten in 49988e4 — probe and tests untouched. system32 is now described as what it actually is: the x64 redist's payload location on win64 (winetricks' own marker directory only on win32, where the two coincide), and syswow64-alone is called out as the #233 partial state that has to stay repairable. The same misreading had leaked into the CHANGELOG entry and a comment in DependencyManager, so those lines got the same fix. |
frankea
left a comment
frankea
left a comment
There was a problem hiding this comment.
Re-verified the rework hands-on: simulated the merge on main in a scratch worktree, built the app target, and ran the full kit suite (267 tests in 36 suites pass, VCRuntimeFallbackTests 6/6 isolated as well, swiftlint strict and pinned swiftformat both clean). 49988e4 is doc and changelog wording only; the probe and tests are byte-identical to the previously reviewed state.
The logic checks out end to end: the probe keys on system32/mfc140.dll alone (msvcp140/vcruntime140 survive only in comments), fires only for the vcruntime definition when the verb log actually reports it missing, reports at heuristic confidence, and the syswow64-only partial state from #233 stays "Not Installed" so a repair remains possible.
One housekeeping note: the branch predates the 3.7.0 release commits, so the changelog entry lands inside the 3.7.0 section when merged; I'll move it up to Unreleased on main right after. Nothing for you to do.
Thanks for the fast turnarounds on both reworks and for confirming against the real stuck bottle.
The #239 branch predated the 3.7.0 release commits, so the squash merge placed its entry inside the 3.7.0 section. The change ships in the next release, not 3.7.0.
3 participants
Third of the three changes agreed in #233.
The winetricks.log entry for a verb is only written once the verb's installer exits, and the vc_redist installer is known to hang under wine after installing successfully. That leaves the runtime fully in place while verb-based detection keeps reporting "Not Installed": the state my bottle was in.
As discussed:
vcruntimedefinitionheuristicconfidence, so the UI can distinguish it from log-backed detectionThe WhiskyKit tests include the regression case from the issue (DLLs present, log entry missing → detected as installed) plus the scoping and DLL-presence variants.
Includes a CHANGELOG entry.