Drop/manifest sidecar#74
Merged
facontidavide merged 12 commits intodevelopmentfrom Apr 26, 2026
Merged
Conversation
ExtensionManager::isInstalled previously trusted the in-memory installed_ QMap, with no cross-check against the on-disk path. When a user removed a plugin's .so or .dll outside the marketplace UI (rm, package cleanup, etc.) the map kept reporting the extension as installed and the UI rendered phantom "Installed" badges. Make isInstalled validate the path on read: if the cache says yes but the file is gone, evict the stale entry and emit a new extensionEvictedExternally(QString) signal so downstream consumers (the PJ4 ExtensionCatalogService catalog vectors, marketplace dialog) can refresh their views without an explicit reload trip. Add reconcileInstalledWithDisk() as a public batch variant — walks every entry once and evicts all stale ones. Used by the marketplace dialog's Refresh button and showEvent (next commit). Cost: ~30 LOC. const_cast is the price of preserving the const-correct public API; every UI caller (populateCards) iterates per-paint, so a non-const variant would have rippled through call sites.
Hook the existing Refresh button to reconcileInstalledWithDisk() so a manual click evicts stale entries. Also override QDialog::showEvent to reconcile every time the dialog opens — cheap (one stat per installed entry), and means the user never sees a phantom "Installed" card without taking explicit action. When reconciliation actually evicts something, set installations_changed_ so MainWindow's post-close catalog.reload() pipeline runs and the streaming combo updates too.
The on-disk manifest.json was being abused for two purposes: plugin self-description (capabilities, file_extensions, name, encodings) AND host bookkeeping (id, version, install_date). Plugin self-description is already authoritatively exposed by the .so's vt_->manifest_json (see PJ_data_source_vtable_t in data_source_protocol.h:251), and that's the only thing ExtensionCatalogService::loadAndRegister* reads. Sidecar manifest.json was therefore redundant for the plugin-loading path AND a divergence risk: nothing prevents a plugin author's compile-time vtable manifest from disagreeing with the bundled manifest.json. Phase 1 split: keep host bookkeeping in a host-owned pj_meta.json (id, version, install_date) that ExtensionManager writes itself at install completion, and stop reading the plugin-author manifest.json entirely. Phase 2 (a separate doc/spec change) will tell plugin authors they can drop manifest.json from their install ZIPs. Concrete changes in ExtensionManager.cpp: - doInstall completion: drop the manifest.json read that was overriding the registry struct's version (lines 129-135). The Extension struct passed to install() is now the authoritative version source. Write pj_meta.json into extensions_dir_/<id>/ via the new writeInstalledMeta() helper. Same pattern for the staging branch (writes pj_meta.json into pending_dir_/<id>/). - applyPendingInstalls: directory name IS the id (install always creates pending_dir_/<ext.id>/), so no file-read is needed to recover it. Version is read from pj_meta.json with a manifest.json fallback for staged installs from before this change. Adds an empty-dir guard so a bare staging directory is skipped instead of producing a phantom installation. - applyPendingUninstalls and hasPendingInstall: same — directory name is the id, no file read. - loadState: directory name = id; version comes from pj_meta.json with manifest.json fallback for legacy installs. The fallback can be removed once all users have re-installed at least once. - savePendingMeta (which was dead code — never called) is renamed and rewired to writeInstalledMeta(ext, dir), parameterized on the target directory so it can serve both pending-staging and direct-install paths. Tests: - dummyPluginZip no longer bakes manifest.json; the test artifact is just the placeholder binary, matching the new packaging convention. - legacyPluginZipWithManifest helper added for backward-compat tests. - New LoadStateFallsBackToManifestJsonForLegacyInstalls test exercises the legacy-install path: a directory with only manifest.json (no pj_meta.json) must still be visible in installedExtensions() after a fresh ExtensionManager loads its state. - ApplyPendingInstallsSkipsDirectoryWithoutMetaFile renamed to ApplyPendingInstallsSkipsEmptyStagingDirectory and updated to assert the empty-dir guard rather than the manifest-presence check. The pre-existing UpdateReinstallsWithNewVersion failure is unaffected (unrelated to manifest reading; involves the update->backup->reinstall flow).
…source of truth
Eliminate every local plugin metadata sidecar. The host already dlopens each
installed extension at startup; the manifest_json string compiled into the DSO
vtable is the authoritative source for id/name/version/family. Reading that
directly removes a class of "phantom card" / drift bugs the prior commits on
this branch were chasing.
pj_plugins:
- Replace .pjmanifest.json sidecar discovery with scanPluginDsos(), which
dlopens each candidate, probes the four family vtables, parses the embedded
manifest_json, and returns descriptors plus per-file diagnostics. A single
broken DSO no longer aborts discovery.
- Require id, name, version on every embedded manifest; message parsers also
require encoding. Missing keys produce a diagnostic, not a silent skip.
- Delete cmake/PjPluginManifest.cmake (pj_emit_plugin_manifest helper) and all
call sites; first-class plugins now embed "id" in their manifest_json.
pj_marketplace:
- ExtensionManager no longer writes or reads pj_meta.json/manifest.json.
Installed state is rebuilt from disk by scanning plugin DSOs.
- Validate registry id and version against the embedded manifest on every
install, update, and pending-restart promotion. Mismatch fails cleanly and
removes the freshly extracted directory.
- Defer Windows staged-install validation to applyPendingInstalls() at next
startup, when the staged DSO becomes the loaded one (the post-restart load
IS the validation). Persist registry intent in a .pj_pending_install marker
so promotion can revalidate id+version.
- hasPendingInstall is a pure marker-existence check; no const_cast, no
signal emission, no recursive directory walk. Closes the latent recursive
re-entry bug where populateCards -> hasPendingInstall -> emit ->
populateCards.
- Drop pending_restart_ids_ from MarketplaceWindow; predicate-only model.
- populateCards caches installedExtensions() once before the per-card loop and
avoids recomputing hasUpdate per ext.
- Templatize the three direct-vtable family probes in plugin_catalog.cpp via
a shared probeDirectVtable helper while keeping the named try* functions
for call-site readability.
Tests:
- extension_manager_test.cpp rewritten around real plugin DSO fixtures
instead of ZIP-local JSON. Adds coverage for embedded id/version mismatch,
intra-directory id conflicts, broken DSO rejection, staged-promotion
validation, and external eviction. 51/51 tests pass.
- New DSO fixtures: missing_id_data_source_plugin.cpp,
mock_data_source_v2_plugin.cpp.
- Mock*Manager classes deleted; the rewritten tests no longer need them.
pj_base:
- Expected<T> now uses ErrorStorage to disambiguate variant<T,E> when T==E
(e.g. Expected<std::string>); test added.
- SDK base classes assert "id" presence in the embedded manifest at vtable
construction.
Documentation in pj_marketplace/ and pj_plugins/ updated to describe embedded
DSO manifest as the source of truth and to remove all references to local
sidecars.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gnostic sink
PR-review follow-up. The bulk of the change is in three layers:
Marketplace install/uninstall hardening (pj_marketplace):
- Update-All queue advanced twice on every failure because emitInstallFailure
fires both installError and installFinished and both UI handlers called
processInstallQueue; the queue now advances exactly once per outcome.
- Category combo data values now match the registry schema
(data_streamer / parser); previously "data_stream" / "message_parser"
silently filtered out every matching extension.
- ExtensionManager::initComponents drains pending install/uninstall queues
before computing the installed snapshot, so all three MarketplaceWindow
ctors (and any host-managed mgr) get the same restart-recovery semantics.
- schedulePendingUninstall now returns bool; uninstall() no longer mutates
installed_ or emits uninstallPendingRestart if the marker write fails.
- Failed staged installs that cannot be removed are renamed to
.pj_quarantine_<name>_<uuid>/ so the next start does not re-validate the
same payload forever; quarantine entries are skipped by applyPendingInstalls.
- applyPendingUninstalls reports a diagnostic when removeRecursively fails
(was silently dropped, leading to invisible uninstall stalls).
- Settings dialog rejects invalid registry URLs (non-http/https/file scheme)
before persisting them to QSettings.
- Pending-install intent file id/version are validated against safe-path /
semver-regex rules to defend against tampered intent files.
- Lambda-capture asymmetry fixed: failed/cancelled handlers now capture
transaction_root by value like the finished handler.
- Sticky startup error is cleared on successful registry fetch so progress
messages aren't suppressed.
- mkpath(extensions_dir_) failure is surfaced as a startup diagnostic.
- doInstall now re-validates the DSO from its FINAL location after the
rename, catching rpath/dep issues that hold in the staging area but break
in extensions/.
- Standalone CMake build synthesizes a pj_plugin_catalog target instead of
inlining sources, eliminating a duplicate-symbol risk for downstream
consumers that link both the standalone marketplace and pj_plugin_catalog.
- Renamed .extension_windows_staging -> .extension_staging across code,
tests, and all docs; the path is no longer Windows-specific now that the
staging area is the validation gate on every OS.
Plugin catalog (pj_plugins):
- scanPluginDsos no longer terminates the whole scan on the first per-entry
filesystem error; each unreadable subtree is reported as a diagnostic and
iteration continues.
pj_proto_app plugin loader:
- loadAndRegister{DataSource,MessageParser,Toolbox} now narrow catch(...) to
nlohmann::json::exception, log the actual error, and refuse to register
plugins whose embedded manifest is missing required string fields. The
previous "fabricate id from filename" fallback produced ids that could
never match a registry entry, leaving plugins effectively undeletable
from the marketplace UI.
Unified diagnostic-propagation API (new):
- pj_base/include/pj_base/diagnostic_sink.hpp — header-only PJ::Diagnostic /
PJ::DiagnosticLevel / PJ::DiagnosticSink (std::function), zero-Qt, with
a teeSink composition helper.
- ExtensionManager forwards through an optional sink in addition to its
existing diagnostics() ring buffer and diagnosticReported Qt signal —
hosts can subscribe to one chronological stream covering both marketplace
and non-marketplace events.
- PluginRegistry takes an optional sink and routes 27+ std::cerr lines
through it with appropriate levels; success messages are info-level and
stay in the diagnostics dialog only, errors flash in the status bar.
- pj_proto_app::QtDiagnosticBridge marshals every sink event onto the Qt
thread via QueuedConnection (QPointer-guarded for outlive safety) and
re-emits as a Qt signal.
- MainWindow now has a QStatusBar plus a Diagnostics dialog (200-entry
cap), wired to the bridge; MarketplaceWindow continues to use the
marketplace-internal signal as before.
Tests: 51/52 pass (1 pre-existing GTK leak in dialog_engine_test, unrelated).
Two new capturing-sink assertions in proto_plugin_registry_test pin the
GUI propagation contract.
Docs updated: marketplace ARCHITECTURE / USER_MANUAL / spec / REQUIREMENTS,
pj_plugins ARCHITECTURE.md (new "Host-side diagnostic propagation" section).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SDK / ABI: - Collapse PJ_DIALOG_PLUGIN_VTABLE into a single PJ_DIALOG_PLUGIN that works for both standalone and co-resident DSOs. The boot-level pj_plugin_abi_version export now lives at file scope in pj_base/plugin_abi_export.h with weak linkage (selectany on MSVC) so duplicate definitions across family macros in one DSO collapse to a single COMDAT entry. - pj_proto_app PluginRegistry: parser "encoding" manifest key is now required and accepts either a string or an array of strings; new test pins the string form through mock_json_parser_plugin. Build tooling: - New cmake/PjPluginManifest.cmake helper emits an inspection-only <target>.pjmanifest.json sidecar next to plugin DSOs (augmenting the embedded manifest with abi_major + family). Runtime discovery ignores the file; it exists for packaging diagnostics and dev tools. Validates id/name/version at configure time. Documentation audit (13 files): - Add the now-required "id" key to manifest schema tables, examples, and protocol-header doc comments across data_source / message_parser / toolbox / dialog families. - pj_datastore USER_GUIDE: replace the removed appendArrowIpc bulk import section with the v4 Arrow C Data Interface equivalent (appendArrowStream + ArrowStreamHolder). - pj_plugins ARCHITECTURE.md + plugin_data_api.h: rewrite the boot-ABI symbol description to credit the shared header and weak linkage, not per-family macros. - pj_marketplace ARCHITECTURE / USER_MANUAL / spec: PlantUML install flow now shows the .pj_install_<id>_<uuid>/ transaction directory and post-promotion DSO re-validation; .extension_staging/ no longer flagged Windows-only (used on all platforms as the validation gate); .pj_pending_install nesting corrected; transaction-folder location clarified per platform. - Drop a stale, never-existed test path reference from toolbox-guide; add a Manifest Schema section to the dialog-plugin-guide that other family guides already had. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
applyPendingInstalls() previously called removeRecursively() on the existing extensions/<id>/ directory before promoting the staged update, so a Windows update silently destroyed the prior install with no recovery path. Linux/macOS already moved the previous version to .backup/<id>-<oldversion>/ synchronously inside update(); the Windows restart-time path lacked that symmetry. This change moves the existing dir to PlatformUtils::backupDir() before the rename, mirroring the Linux/macOS behavior. If the staged-dir promotion fails after the backup move, the marketplace attempts to roll the backup back into place; if that also fails the diagnostic surfaces both paths so the user can recover manually. Edge cases handled: - Existing extension's manifest unreadable -> backup gets a uuid suffix so we never silently overwrite an older recovery point. - Pre-existing backup at .backup/<id>-<version>/ (leftover from an earlier failed update) -> uuid-suffixed to keep both copies. - pending_backup_path_ cleared between iterations so it can't leak the previous extension's backup into a later failure diagnostic. Test: ApplyPendingInstallsBacksUpExistingExtensionBeforePromotion installs v1, manually stages v2 with intent file (the on-disk shape Windows leaves before restart), runs applyPendingInstalls(), asserts v2 is active AND v1 survives at .backup/mock-data-source-1.0.0/. Docs updated to drop the "Linux-only backup" framing in ARCHITECTURE, USER_MANUAL, and the spec — the mechanism now applies on every platform. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two unrelated CI failures on PR #74, both fixed here. 1) Windows CI: pj_base/plugin_abi_export.h failed with error C2496: '__declspec(selectany)' can only be applied to data items with external linkage. Root cause: a namespace-scope const variable has internal linkage by default in C++. extern "C" only sets language linkage (name mangling), not internal/external linkage, so 'extern "C" const T x = val;' is still internal-linkage in C++ — and MSVC refuses selectany on internal-linkage data. GCC happily accepted it but the construct was technically broken there too (weak fold relies on external linkage). Fix: drop const (the symbol is host-read once via dlsym during ABI handshake; nothing inside any plugin DSO writes to it) and use the block form 'extern "C" { ... }' so GCC does not parse the declaration as extern-with-initializer and trip -Wextern-initializer. 2) Linux CI: 4 ExtensionManagerTest cases (UpdateReinstallsWithNewVersion, UpdateBacksUpOldVersionOnSuccess, UpdateKeepsBackupWhenInstallFails, ApplyPendingInstallsBacksUpExistingExtensionBeforePromotion) failed at ASSERT_TRUE(local_ext_dir.isValid()). Root cause: each test constructs a QTemporaryDir under PlatformUtils::configDir() (= ~/.local/share/plotjuggler/) so that QDir::rename() into backupDir() is a same-filesystem atomic move. QTemporaryDir refuses to create itself when the parent directory does not exist — exactly the state of a fresh CI runner. The tests passed locally only because that directory already existed from regular use. Fix: SetUp() now QDir().mkpath(PlatformUtils::configDir()) before constructing the fixture's temp dirs, so the precondition is explicit instead of inherited from host filesystem state. Verified: full ctest (52/52) passes both with the existing config dir and under a fresh XDG_DATA_HOME=\$(mktemp -d) simulating the CI runner. Co-Authored-By: Claude Opus 4.7 (1M context) <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
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.
No description provided.