Skip to content

Move MyMPILib into NEO-2#66

Open
krystophny wants to merge 3 commits intomainfrom
dep-audit/wave-01-remove-mympilib
Open

Move MyMPILib into NEO-2#66
krystophny wants to merge 3 commits intomainfrom
dep-audit/wave-01-remove-mympilib

Conversation

@krystophny
Copy link
Copy Markdown
Member

@krystophny krystophny commented Mar 27, 2026

Summary

Move MyMPILib into NEO-2 and keep the PR scoped to that change.

What Changed

  • add COMMON/MyMPILib as the in-repo home of the library
  • move the MyMPILib user doc into DOC/UserDoc/MyMPILib.tex
  • move the dedicated regression test into TEST/mympilib/
  • wire COMMON against the local NEO2MyMPILib target instead of LIBNEO::MyMPILib
  • restore HTTPS-based dependency fetches in cmake/Util.cmake and TEST/cmake/test_libneo_tag.sh
  • remove the extra LIBNEO_SOURCE_DIR and FGSL_PATH wrapper/config plumbing from this PR
  • add make test to the active PR workflow so the ported TEST/mympilib path runs in CI
  • keep a narrow top-level Python interpreter hint so fetched libneo configures reliably with the system Python it still requires today

Why

MyMPILib is NEO-2-specific code. Keeping it in libneo made libneo carry an unnecessary MPI build surface and made ownership unclear.

Impact

  • NEO-2 now owns its MPI abstraction directly
  • the CI path keeps using HTTPS for dependency fetches
  • the ported TEST/mympilib test is now exercised by the active PR workflow

Verification

$ make clean
rm -rf build

$ make
...
[438/438] Linking Fortran executable NEO-2-QL/neo_2_ql.x

$ make test
cmake --build --preset default
[1/2] cd /home/ert/code/NEO-2/COMMON/MyMPILib && /home/ert/code/NEO-2/COMMON/MyMPILib/Scripts/do_versioning.sh
Versioning MyMPILib...
[2/2] cd /home/ert/code/NEO-2/build/COMMON && /home/ert/code/NEO-2/COMMON/../ShellScripts/tag_version.sh
...
Test project /home/ert/code/NEO-2/build/TEST
    Start 1: nrutil_test
1/2 Test #1: nrutil_test ......................   Passed    0.07 sec
    Start 2: test_mympilib
2/2 Test #2: test_mympilib ....................   Passed    0.05 sec

100% tests passed, 0 tests failed out of 2

Related

  • Companion libneo PR removes MyMPILib from libneo entirely.
  • Related issue: #258

@GeorgGrassler
Copy link
Copy Markdown
Contributor

@krystophny I took a view at it: The breaking of the golden record run comes most likely from the changed find_or_fetch function workflow. Now it requires ssh authentification in the ci workflow, whereas previously it was find with https. In general, I recommend not fixing everything in one single pr now and just focus on the main objective: moving MPILib into NEO-2. So my review:

  • avoid drive-by fixes for now if not needed to do the pr
  • only move MPILib to NEO-2 and fix the local build process to as was already done in the pr with add_subdirectory etc.
  • add a test: the golden record test only runs in serial mode. It offers therefore no protection of the correct working of MPILib.
  • add the ported over tests in TEST to the CI pipeline

I looked at MPILib in libneo myself again and it is already quite self contained. There should be no changes necessary at all. Ran a simple recursive diff and got the following back

differences.txt

@krystophny krystophny changed the title [codex] Move MyMPILib into NEO-2 Move MyMPILib into NEO-2 Mar 30, 2026
@krystophny
Copy link
Copy Markdown
Member Author

Addressed the review points in b53dc8c.

  • Restored HTTPS-based fetch/clone paths in cmake/Util.cmake and TEST/cmake/test_libneo_tag.sh, so this PR no longer switches CI over to SSH auth.
  • Removed the extra LIBNEO_SOURCE_DIR and FGSL_PATH wrapper/config plumbing again to keep the PR focused on moving MyMPILib into NEO-2.
  • Kept only one narrow build fix: a top-level Python interpreter hint in CMakeLists.txt, because fetched libneo still configures Python today and CMake was otherwise selecting a broken user-local interpreter here.
  • The ported TEST/mympilib test stays under TEST/, and I added make test to the active PR workflow so it is exercised in CI instead of only existing on disk.

Local verification:

make clean
make
make test

Result:

100% tests passed, 0 tests failed out of 2

@GeorgGrassler
Copy link
Copy Markdown
Contributor

GeorgGrassler commented Mar 30, 2026

* Kept only one narrow build fix: a top-level Python interpreter hint in `CMakeLists.txt`, because fetched `libneo` still configures Python today and CMake was otherwise selecting a broken user-local interpreter here.

Where was it selecting a broken user-local interpreter? I could not find anything about that in the CMakeConfigs? Or was it doing that in libneo configure/build instructions? Cause then this should belong to the libneo fix and not on the consumer side.

@krystophny added also some more comments/questions directly add the changed code lines. That would need clarification first, before approval.

The /usr/bin/python3 hint was a workaround for a stale uv-managed
Python 3.12 in ~/.local/bin that lacked dev headers. Fixed the local
system instead: removed the broken symlink so CMake find_package
discovers the system Python correctly.
@krystophny
Copy link
Copy Markdown
Member Author

Removed the hardcoded Python interpreter path in 10d7499. The root cause was a stale uv-managed Python 3.12 symlink in ~/.local/bin/ that lacked dev headers and NumPy — CMake picked it up before the system Python 3.14. Fixed on the local system by removing the broken symlink; the find_package(Python ...) call finds the correct interpreter on its own now.

Regarding the inline comments: I can see the reference to source-level comments but the GitHub API returns none — they may still be in pending/draft state. Could you submit the review so I can see and address them?

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