Use fortio through libneo for scientific I/O - #173
Conversation
There was a problem hiding this comment.
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:
-
[blocker] COMMON/neo_sub.f90:2593 —
neo_read_boozmn()still doesuse netcdf, only : nf90_open, ...(and subsequentnf90_*calls). This is reachable from the main code path:neo_init→neo_read→neo_read_boozmnwheninp_swi == INP_SWI_BOOZMN, invoked fromCOMMON/neo_magfie.f90:341(both QL and PAR),tools/create_surfaces/src/create_surfaces.f90, and the unit test. The PR removesfind_package(HDF5)(CMakeLists.txt), thenf-configinclude/link setup and${NETCDF_FLIBS}(COMMON/ProjectConfig.cmake.in), and${NETCDF_LIBS}(COMMON/CMakeLists.txt:105) — so thenetcdfFortran module is neither on the include path (netcdf.modno longer found) nor linked (-lnetcdffgone). Thecommonstatic library can no longer compile or link. Required fix: migrateneo_read_boozmnand its callers to libneo's Fortio/booz-xform reader and updateTEST/test_boozmn_read.f90accordingly, or keep the NetCDF configuration/linkage for this path. -
[blocker] TEST/test_boozmn_read.f90:39-41 and 449+ — this committed unit test links
commonand uses the system NetCDF module extensively (nf90_create,nf90_def_var,nf90_put_var, etc.) to build its fixtures, and exercises exactly theneo_read_boozmnpath that finding #1 removes. With the NetCDF packaging dropped in.github/workflows/unit-tests.yml:44and.github/workflows/unit-tests-coverage.yml:38and the link flags removed,make testcan 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. -
[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_boozmnagainst 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.bcinputs and never executeneo_read_boozmn, so they cannot detect this regression — onlymake testwould, and that workflow had the NetCDF packages removed. Fix the comment and add CI/unit coverage for the.ncBooz-xform input path. -
[minor] Readme.md:14 and Build.md:27-28 — the prerequisites still instruct users to
apt install ... libhdf5-dev libnetcdf-dev libnetcdff-devand listhdf5_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.
|
Dependency follow-up: this branch now pins libneo bridge |
|
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 |
There was a problem hiding this comment.
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.
What changed
LIBNEO_REFoverrides.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
boozmn_read_test: passes against the Fortio-backed stack.foreaches the pre-existing missing FGSL archive (fgsl/src/FGSL/.libs/libfgsl.a) after static analysis; that external build prerequisite is unchanged by this migration.