Skip to content

QVAC-25104 infra: treat the shared addon CMake template as native input - #4542

Merged
jpgaribotti merged 4 commits into
mainfrom
fix/qvac-25104-native-detect-addon-template
Sep 18, 2026
Merged

jpgaribotti merged 4 commits into
mainfrom
fix/qvac-25104-native-detect-addon-template

Conversation

@jpgaribotti

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

cmake/qvac-addon/ decides link lines, exported symbols and build-time assertions for every addon that includes it, so an edit there changes the built module exactly as an edit to the package's own CMakeLists.txt does. CI did not treat it that way, on three counts, all silent:

  • detect-native-changes matched only ${WORKDIR} sources, CMakeLists.txt, the vcpkg files and vcpkg-overlays, so a template edit reported native_changed=false.
  • native_hash covered the same set, so the key stayed byte-identical and prebuild-artifact-reuse could serve prebuilds built before the template changed. The comment above the hash argues that a wrong false can never reuse a stale binary because the key would not match — which holds only while the key covers everything affecting the binary.
  • on-pr-nx.yml triggered on packages/** only, so a template-only PR ran no native CI at all.

Measured on #4519, which edits the template and six consumers: its latest run reused prebuilds for all four packages it built, with the per-platform matrix and merge jobs skipped.

📝 How does it solve it?

  • Adds cmake/qvac-addon/* to the detection predicate and cmake/qvac-addon to the git ls-tree pathspec plus its grep -E alternation, keeping the two in the lockstep the comment there requires.
  • Adds cmake/qvac-addon/** to on-pr-nx.yml's paths:.
  • Makes the directory an nx project that the six consumers declare as an implicit dependency, alongside the inference-addon-cpp and lint-cpp entries they already carry. Without this the trigger alone would start the workflow and then find nothing affected — nx reports [] for a lone cmake/qvac-addon/ path. The project carries no build target, so it never lands in the matrix itself; only its dependents do.

🧪 How was it tested?

  • Ran the case predicate over a sample of paths: both template files report native_changed=true, while cmake/other/thing.cmake and docs/foo.md stay false and a package source file is unchanged in behaviour.
  • Confirmed the template files now fall inside the hashed git ls-tree set.
  • nx show projects --affected -t build --stdin with only cmake/qvac-addon/qvac-addon.cmake: all six consumers plus their dependents, where it previously returned []. An unrelated path still returns [], and qvac-addon-cmake itself never appears.
  • Both YAML files parse.

💥 Breaking Changes

Two intended costs. native_hash changes once for every package, which invalidates the existing prebuild caches, and any cmake/qvac-addon/ edit now rebuilds the six consumers natively.

Touches DevOps-owned paths (.github/workflows/, .github/actions/ per .github/teams/devops.json).

@jpgaribotti
jpgaribotti requested review from a team as code owners September 17, 2026 13:30
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Status

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

@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Workflow security (shadow mode)

zizmor found 1103 finding(s) in .github/ (highest severity: high). This check is warn-only and does not block the merge.

Findings are annotated inline on the changed files and listed in the job summary.

Reproduce locally:

pipx run zizmor==1.27.0 --offline .github/

@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

cmake/qvac-addon/ decides link lines, exported symbols and build-time
assertions for every addon that includes it, so an edit there changes the
built module exactly as an edit to the package's own CMakeLists.txt does.
CI did not see it that way on any of three counts, and all three are
silent:

  * detect-native-changes matched only ${WORKDIR} sources, CMakeLists.txt,
    the vcpkg files and vcpkg-overlays, so a template edit reported
    native_changed=false.
  * native_hash covered the same set, so the key stayed byte-identical and
    prebuild-artifact-reuse could serve a package prebuilds built before
    the template changed. The comment above the hash argues a wrong "false"
    can never reuse a stale binary because the key would not match, which
    holds only while the key covers everything that affects the binary.
  * on-pr-nx.yml triggered on packages/** only, so a template-only PR ran
    no native CI at all.

Measured on #4519, which edits the template and six consumers: the last
run reused prebuilds for all four packages it built, the per-platform
matrix and merge jobs skipped.

The trigger needs the nx graph to agree, or a template-only push would
start the workflow and then find nothing affected -- measured: nx reports
[] for a lone cmake/qvac-addon/ path. So the directory becomes an nx
project the six consumers declare as an implicit dependency, the same way
they already depend on inference-addon-cpp and lint-cpp. It carries no
build target, so it never lands in the matrix itself; only its dependents
do.

Costs, both intended: native_hash changes once for every package, which
invalidates the existing prebuild caches, and any cmake/qvac-addon/ edit
now rebuilds the six consumers natively.

Reported in review on #4519.
llm-llamacpp now includes cmake/qvac-addon, so it has to declare the same
implicit dependency as the other consumers. on-pr-ts-nx must trigger on
that path or the nx await times out, and classification/vla still live on
bespoke orchestrators that on-pr-nx will not start.
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.

3 participants