Skip to content

ci: propagate dispatched Libneo revision - #507

Open
krystophny wants to merge 5 commits into
mainfrom
agent/propagate-libneo-ref
Open

ci: propagate dispatched Libneo revision#507
krystophny wants to merge 5 commits into
mainfrom
agent/propagate-libneo-ref

Conversation

@krystophny

Copy link
Copy Markdown
Member

Ensures the full reverse-dependency workflow tests the dispatched Libneo SHA in all build paths, including the editable Python package and deterministic regression reconfiguration. Previously those stages silently fell back to SIMPLE's default Libneo pin.

Verification: actionlint .github/workflows/main.yml passes. Bare fo reaches a pre-existing fpm duplicate-module bootstrap failure before tests.

This PR is intentionally ready-for-review and must remain open/unmerged.

@slopqueue slopqueue Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review verdict: Request changes

Summary: PR #507 pins libneo to commit afa0e42, drops the NetCDF/HDF5 fallback in favor of the required fortio::fortio target, propagates a dispatched libneo_ref through CI, and renames the libneo test-toggle cache variable. The direction is sound and consistent, but the new CI/regression propagation is silently defeated by a duplicate -DLIBNEO_REF.

Findings:

  1. [major] Makefile:77 + .github/workflows/main.yml:119-130 — a dispatched libneo revision is silently ignored for regression/golden tests. make test-regression LIBNEO_REF=X sets FLAGS to -DLIBNEO_REF=X (Makefile:9), then the target appends -DLIBNEO_REF=$(GOLDEN_LIBNEO_REF) (line 77), yielding two -DLIBNEO_REF flags; CMake resolves last-one-wins, so the golden SHA always overrides the dispatched ref. The CI step added in commit 4aaf84d is therefore ineffective, and any make test-regression LIBNEO_REF=<candidate> also silently builds against the golden SHA. Fix: apply the golden override only when LIBNEO_REF was not given on the command line.

  2. [minor] cmake/Util.cmake:98 — default libneo revision is hardcoded to afa0e42, replacing get_branch_or_main, which is now dead code; feature-branch builds no longer match a same-named libneo branch. Remove the unused function and document the pinning policy.

  3. [minor] cmake/Util.cmake:66,101 — LIBNEO_ENABLE_TESTS renamed to LIBNEO_BUILD_TESTING; couldn't verify against libneo (no network). Confirm libneo's CMake option name before merge.

  4. [minor] CMakeLists.txt:180-186 — removing the NetCDF fallback for a hard fortio::fortio requirement is coherent with the pin, but users overriding to older libneo revisions now get a hard failure instead of the fallback; deliberate compatibility cut.

Verdict: Request changes — the dispatched libneo_ref propagation for the regression/golden suite (the purpose of the CI change) is defeated by the duplicate -DLIBNEO_REF in the test-regression target.

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.

1 participant