Skip to content

Use fortio through libneo for scientific I/O - #173

Open
krystophny wants to merge 14 commits into
mainfrom
agent/fortio-io-migration
Open

Use fortio through libneo for scientific I/O#173
krystophny wants to merge 14 commits into
mainfrom
agent/fortio-io-migration

Conversation

@krystophny

@krystophny krystophny commented Jul 30, 2026

Copy link
Copy Markdown
Member

What changed

  • Pins the final tested Libneo/Fortio migration revision by default while preserving LIBNEO_REF overrides.
  • Removes direct NetCDF-Fortran discovery, include paths, and linkage.
  • Removes direct HDF5 linkage from the multispecies merge tool.

This keeps NEO-2 on its existing compatibility APIs while moving their implementation to Fortio's deliberately limited ITP-use-case subset. It does not add general HDF5 support.

Validation

  • Clean remote CMake FetchContent build of Libneo and Fortio: passes.
  • Focused boozmn_read_test: passes against the Fortio-backed stack.
  • Libneo's downstream gate has independently completed NEO-2 successfully against this migration.
  • Bare fo reaches the pre-existing missing FGSL archive (fgsl/src/FGSL/.libs/libfgsl.a) after static analysis; that external build prerequisite is unchanged by this migration.

@krystophny
krystophny marked this pull request as ready for review July 30, 2026 16:26
@krystophny
krystophny enabled auto-merge (squash) July 30, 2026 17:07
@krystophny
krystophny disabled auto-merge July 30, 2026 17:45

@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: This PR migrates NEO-2's scientific I/O from system HDF5/NetCDF libraries to libneo's Fortio-backed hdf5_tools. It removes find_package(HDF5), the NetCDF discovery/linkage block in ProjectConfig.cmake.in, ${NETCDF_LIBS} from the common target, and drops the libhdf5-dev libnetcdf-dev libnetcdff-dev apt packages from the unit-test workflows, while pinning libneo to a fixed commit. The HDF5 half of the migration is sound (hdf5_tools/hdf5_tools_f2003 now resolve from LIBNEO::hdf5_tools), but the PR removes all system-NetCDF support while the Booz-xform netcdf reader was never migrated, leaving the tree uncompilable.

Findings:

  1. [blocker] COMMON/neo_sub.f90:2593 — neo_read_boozmn() still does use netcdf, only : nf90_open, ... (and subsequent nf90_* calls). This is reachable from the main code path: neo_initneo_readneo_read_boozmn when inp_swi == INP_SWI_BOOZMN, invoked from COMMON/neo_magfie.f90:341 (both QL and PAR), tools/create_surfaces/src/create_surfaces.f90, and the unit test. The PR removes find_package(HDF5) (CMakeLists.txt), the nf-config include/link setup and ${NETCDF_FLIBS} (COMMON/ProjectConfig.cmake.in), and ${NETCDF_LIBS} (COMMON/CMakeLists.txt:105) — so the netcdf Fortran module is neither on the include path (netcdf.mod no longer found) nor linked (-lnetcdff gone). The common static library can no longer compile or link. Required fix: migrate neo_read_boozmn and its callers to libneo's Fortio/booz-xform reader and update TEST/test_boozmn_read.f90 accordingly, or keep the NetCDF configuration/linkage for this path.

  2. [blocker] TEST/test_boozmn_read.f90:39-41 and 449+ — this committed unit test links common and uses the system NetCDF module extensively (nf90_create, nf90_def_var, nf90_put_var, etc.) to build its fixtures, and exercises exactly the neo_read_boozmn path that finding #1 removes. With the NetCDF packaging dropped in .github/workflows/unit-tests.yml:44 and .github/workflows/unit-tests-coverage.yml:38 and the link flags removed, make test can no longer build this test, so CI will fail. Required fix: convert this test to the same Fortio/libneo backend used for production I/O, or keep NetCDF available and linked.

  3. [minor] .github/workflows/test-on-pr.yml:44-45 — the added comment claims the "candidate build ... uses dependency-free Fortio through libneo," but the candidate build still compiles neo_read_boozmn against system NetCDF (finding #1), so the build is not actually NetCDF-dependency-free. The comment is misleading; moreover the PR CI's golden-record tests (lorentz/ql) use .bc inputs and never execute neo_read_boozmn, so they cannot detect this regression — only make test would, and that workflow had the NetCDF packages removed. Fix the comment and add CI/unit coverage for the .nc Booz-xform input path.

  4. [minor] Readme.md:14 and Build.md:27-28 — the prerequisites still instruct users to apt install ... libhdf5-dev libnetcdf-dev libnetcdff-dev and list hdf5_tools. After this PR HDF5/NetCDF come from libneo's Fortio backend and these packages are no longer required (and, once finding #1 is fixed, NetCDF won't be needed at all). Update the documentation to reflect the reduced dependency set.

Verdict: Request changes — the PR removes the system NetCDF toolchain while neo_read_boozmn() (the Booz-xform .nc input reader) and its unit test still depend on it unconditionally, so the common library and the unit tests can no longer build.

@krystophny

Copy link
Copy Markdown
Member Author

Dependency follow-up: this branch now pins libneo bridge 9f1e9dcf5ab6800213415eac485e85a6d395f72a from libneo PR itpplasma/libneo#413, which consumes standalone Fortio 11afd0bd1af0c99ea4e9a1c0df683dbdcc299b69 from Fortio PR lazy-fortran/fortio#68. No system HDF5 link is introduced.

@krystophny

Copy link
Copy Markdown
Member Author

Correction: the compatible main-line libneo bridge is PR itpplasma/libneo#414 at commit 7262c32bf0406594a8ef27e84654d249aef2cb86. This replaces the FortFEM-specific bridge reference; it consumes standalone Fortio PR lazy-fortran/fortio#68. The consumer branch has been updated and CI restarted.

)

target_link_libraries(${PROJECT_EXE_NAME}
LIBNEO::hdf5_tools

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

general question: once itpplasma/libneo#413 lands, libneos hdf5 tooling is just a glorified wrapper, no? That is the intended design i.e. libneo acts as the hdf_5/netcdf facade where we decide what the backend is? Cause if not, this is just more complicated then directly linking fortio. Fine with the first approach, just wanna know if this is the intend.

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.

2 participants