Reconcile CONTRIBUTING.md and developer_guide.rst on GFE/pFUnit setup - #134
Merged
Merged
Conversation
…#165) * Reconcile CONTRIBUTING.md and developer_guide.rst on GFE/pFUnit setup (#53) Both docs described a different, non-working procedure for getting pFUnit available; standardize on the extern/gfe (lowercase) path that actually matches .gitignore and scripts/develop.sh, add the missing build+install+CMAKE_PREFIX_PATH steps, and cross-link the two docs instead of duplicating divergent instructions. Also notes the current Windows pFUnit build limitation (#163). * Drop stale Windows-fails caveat now that #163 has a fix in progress See #163 (fix landing in a separate PR). (cherry picked from commit 1f7ea1f)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
CONTRIBUTING.md and docs/source/developer_guide.rst gave two different, non-reconciled procedures for getting pFUnit/GFE available so the pFUnit-based
cea_core_testsuite can build and run. Neither matched what actually works (theextern/gfepath used byscripts/develop.sh).djkees#53
Changes
extern/gfe(lowercase), matching.gitignoreandscripts/develop.sh— the old instructions cloned intoextern/GFE, which isn't the path.gitignoreactually ignores. Added the missing build+install step for the vendored GFE/pFUnit copy and the-DCMAKE_PREFIX_PATH=build-dev/installflag CEA's owncmake --preset devneeds to find it — cloning alone (the old instructions) was never enough forfind_package(PFUNIT)to succeed. Documentedscripts/develop.shas the automated equivalent, andPFUNIT_DIRas the alternative for an already-installed PFUnit.extern/GFEcapitalization to match, and replaced the Testing section's contradictory "PFUnit is a maintainer-only dependency, install it yourself" text (which conflicted with the Prerequisites section's own claim that it's vendored) with a cross-link to CONTRIBUTING.md's now-accurate procedure, so the two docs describe one consistent story instead of two different ones.Testing
.gitignore(onlyextern/gfeandextern/installare ignored, notextern/GFE) andscripts/develop.sh(which clones intoextern/gfe, builds it with the same-DSKIP_*flags now documented, and passes-DCMAKE_PREFIX_PATHto the CEA configure step) — the documented procedure is scripts/develop.sh's steps written out manually, not a new invented procedure. Also confirmedCMakeLists.txtonly calls barefind_package(PFUNIT)with no default search path, which is whyCMAKE_PREFIX_PATH/PFUNIT_DIRhas to be set explicitly.Compatibility / Numerical behavior
Update
This PR originally added a note that the vendored pFUnit build "currently fails" on Windows and is effectively Linux/macOS-only. That turned out to have a real fix (the
#ifndef _WIN32guard in pFUnit'sFUnit.F90is already there, but gfortran doesn't predefine_WIN32the way a C/C++ compiler does, so the guard picks the wrong branch) — see #135 in this same batch. That caveat has been dropped from CONTRIBUTING.md/developer_guide.rst here since it's no longer accurate.Merge-order dependency: this PR should merge at the same time as or after #135, not before — merging this first (dropping the caveat) while
mainstill lacks #135's fix would leave CONTRIBUTING.md briefly overclaiming Windows support.Drafted with Claude's assistance
extern/GFEvsextern/gfemismatch and the missing build/install/CMAKE_PREFIX_PATH steps were found by direct comparison of CONTRIBUTING.md's old instructions against.gitignoreandscripts/develop.sh, not assumed from the issue text.CMakeLists.txthas no automatic path to a vendored GFE install (plainfind_package(PFUNIT)), which is why the old CONTRIBUTING.md steps (clone, thencmake --preset dev) could never have worked as written.