Skip to content

QVAC-25104 fix: stop publishing platform-dependent values in fabric's package config - #4539

Merged
jpgaribotti merged 2 commits into
mainfrom
fix/qvac-25104-fabric-config-no-platform-values
Sep 18, 2026
Merged

jpgaribotti merged 2 commits into
mainfrom
fix/qvac-25104-fabric-config-no-platform-values

Conversation

@jpgaribotti

@jpgaribotti jpgaribotti commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

  • qvac-fabricConfig.cmake publishes QVAC_FABRIC_ABI_VERSION and QVAC_FABRIC_OWNS_CXX_RUNTIME, which describe the platform they were configured for — ON on a Linux release link, OFF on Android, empty on darwin, iOS and win32.
  • The file carrying them is not per-platform. It installs to share/qvac-fabric/cmake, every prebuild leg produces it, and the merge job downloads all nine legs with merge-multiple: true, so the published package keeps whichever leg finished last. @qvac/fabric@0.16.0 ships nine platform directories and one prebuilds/share/qvac-fabric/cmake/qvac-fabricConfig.cmake, and qvac_addon_use_fabric points qvac-fabric_DIR at exactly that copy.
  • 0.16.0 happens to carry ON, so today's consumer builds are right by artifact ordering rather than by construction — and only 2 of the 9 legs produce that value. The draw is re-rolled on every release. 0.16.1 still published them.
  • The collision predates this: 0.15.0's config defined no QVAC_FABRIC_* variables and the merged qvac-fabricTargets.cmake has no platform-specific content, so share/ was effectively identical across legs. 0.16.0 was the first release to put a platform-varying value on a shared path.

📝 How does it solve it?

  • Removes both variables from the published config. Publishing them per triplet was the alternative, but there is nothing left to publish for: a consumer asserts the pin against the .bare for its own triplet, where __cxa_throw is either defined under a version node or is not, so QVAC-25104 fix: assert addons pin fabric's C++ runtime and narrow exports #4519 stopped reading either variable and nothing else in the repository ever did.
  • QVAC_FABRIC_ABI_VERSION stays as a build variable in CMakeLists.txt, still stamped onto the version node so the name and the script cannot drift.
  • Leaves a comment in the template recording why nothing platform-dependent may go back into that file.

🧪 How was it tested?

  • Generated the config through configure_package_config_file with fabric's template and loaded it from a separate consumer project: find_package(qvac-fabric CONFIG REQUIRED) resolves, the version file matches, the targets include runs, and both variables come back undefined.
  • Confirmed against the published tarball that the shared path holds one merged copy, and that the non-shared, per-triplet content is untouched by this change.

💥 Breaking Changes

Removes two variables from the package config's public surface. Both were introduced in 0.16.0, describe the wrong platform for 7 of the 9 published legs, and have no consumer in this repository. The changelog entry sits under [Unreleased]; the version is deliberately not stamped here.

Rebased onto main after #4519, #4566, and the 0.16.1 addon rebuilds landed.

@github-actions

Copy link
Copy Markdown
Contributor

License compliance — clean

No new dependency license findings in this PR.

Warn-only (shadow) mode — this check does not block merges yet.

Updated automatically by the canonical license compliance workflow.

NOTICE presence (advisory)

Missing NOTICE (advisory, does not block):

  • ./docs/website
  • ./packages/fabric/test/integration
  • ./packages/llm-llamacpp/benchmarks/server
  • ./packages/llm-llamacpp/benchmarks/performance
  • ./packages/inference-addon-cpp/mobile
  • ./packages/asr-ggml/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/performance
  • ./packages/sdk/e2e
  • ./packages/vla-ggml/sim/server
  • ./.github/actions/release-merge-guard

… config

QVAC_FABRIC_ABI_VERSION and QVAC_FABRIC_OWNS_CXX_RUNTIME describe the
platform they were configured for, but the file carrying them does not:
qvac-fabricConfig.cmake installs to share/qvac-fabric/cmake, every prebuild
leg produces it, and the merge step downloads all nine with
merge-multiple: true, so the published package holds whichever leg finished
last. ON from either Linux leg, OFF from Android, empty from darwin, iOS or
win32 -- and a consumer had no way to tell a value meant for it from one
that was not. The published 0.16.0 carries ON by that ordering rather than
by construction, and the draw is re-rolled on every release.

Publishing them per triplet was the alternative. There is nothing left to
publish for: consumers assert the pin against the .bare for their own
triplet, where __cxa_throw is either defined under a version node or is
not, so the in-tree addon template stopped reading either variable and
nothing else in the repository ever did. A config value with no consumer
that is wrong 7 legs out of 9 is better removed than relocated.

QVAC_FABRIC_ABI_VERSION stays as a build variable, still asserted against
symbols.map so the name and the script cannot drift. The comment left in
the template records why nothing platform-dependent may go back in.

Follow-up to the review on #4519.
@jpgaribotti
jpgaribotti force-pushed the fix/qvac-25104-fabric-config-no-platform-values branch from ef13046 to 5b3f1a1 Compare September 18, 2026 13:23
@jpgaribotti
jpgaribotti marked this pull request as ready for review September 18, 2026 13:42
@jpgaribotti
jpgaribotti requested review from a team as code owners September 18, 2026 13:42
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ✅ APPROVED
Approvals so far: Team Lead: 2, Member: 1

@jpgaribotti
jpgaribotti merged commit 7a8d96f into main Sep 18, 2026
96 checks passed
@jpgaribotti
jpgaribotti deleted the fix/qvac-25104-fabric-config-no-platform-values branch September 18, 2026 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants