[cekees]

Review and dependency summary:

• CI no longer blocked by pumi: add 4.1.0, 4.2.1 #6116 as the new pumi versions have been moved into this PR.
• The dependencies xtensor-python: add 0.28.0 #5826 and libevent: declare pkgconfig as a build dep when +openssl #5829 have already been merged.
• @cekees is the upstream maintainer who is releasing new versions of Proteus.

---

Proteus (proteustoolkit.org): PDE/multiphysics toolkit built around PETSc for parallel linear/nonlinear solves.

This first pass deliberately omittted Chrono (rigid-body/FSI coupling) and SCOREC/PUMI (parallel mesh adaptation) support: both are optional at the proteus code level. Now PUMI has an upstream package with a PR that will work for this package. Chrono has a new package added to this PR.

Sources from cekees/proteus's main branch rather than erdc/proteus's tagged releases: the Chrono/SCOREC-optional support this "no chrono" package design depends on (the PROTEUS_SKIP_PUMI_CHRONO or PROTEUS_SKIP_PUMI and PROTEUS_SKIP_CHRONO env vars, and a try/except around an otherwise-unconditional Chrono config-file lookup) hasn't been released upstream yet. Revisit tracking erdc/proteus tags once that support lands there.

Built and verified working end-to-end (spack install + import proteus + petsc4py Comm.init()) against a from-scratch toolchain, including building openmpi/petsc/etc. from source rather than relying on any external MPI.

Obsolete comment (those fixes have been merged):

Depends on two other fixes proposed alongside this PR (now merged): xtensor-python@0.28.0 (this branch/PR) and xtensor's xsimd upper bound for 0.26.0:0.27 (this branch/PR) -- xtensor-python was previously stuck at 0.23.1, and xtensor's own xsimd constraint was too permissive for the 0.26/0.27 line.

[bernhardkaindl]

@cekees. I just wanted to test how Copilot review does on this PR.

If it asks for change, check carefully, don't apply blindly, it might hallucinate still :)

[Copilot]

Pull request overview

Adds a new Spack py-proteus package (tracking Proteus’ torino_narwhal branch) and includes supporting packaging fixes to enable successful builds on more toolchains/platforms (notably Darwin) and to provide required linkable artifacts for downstream consumers.

Changes:

• Add a new py-proteus Python package with a +scorec variant and extensive dependency/env wiring for Proteus’ build system.
• Update triangle to handle Darwin builds and install a linkable triangle.h + libtri.a for consumers.
• Update zoltan to improve shared-library linking on macOS toolchains (libgfortran discovery and dylib install-name fixups).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
repos/spack_repo/builtin/packages/zoltan/package.py	Improve macOS/shared-library robustness (libgfortran search path + dylib install_name).
repos/spack_repo/builtin/packages/triangle/package.py	Add Darwin-specific build flag handling and install triangle headers/library for link-time consumers.
repos/spack_repo/builtin/packages/py_proteus/package.py	New Proteus Python package with variants, dependencies, and environment configuration for builds.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sethrj]

Leaving this comment to remind that it's under discussion in #6278 @haampie

[bernhardkaindl]

I'd like to summarize the discussion in #6278

• In January, to work around build races, and to work around tests not building in some cases, we split the build into libs, netlib and shared with openblas: skip tests during build #2936
• That was in force until April, when openblas: use default make target #4569 removed the workaround for the build races and the test issues in openblas: skip tests during build #2936 in favour of a faster parallel build that also doesn't run ar multiple times.
• While setting CROSS=1 would not run the tests, it would still build them, but as it was said, building and running the tests should best be part of the run_tests phase when requesed.

I also think I saw some sporadic build errors in GitLab CI that could have been caused by the parallel build races, and CROSS=1 would not fix them.

The parallel build race mentioned was

"ar: libopenblas-r0.3.34.a: error reading zlatm5.o: file truncated"

If libopenblas-r0.3.34.a is build by "libs", then libs could be built first and then "netlib" and "shared" chould be built in parallel.

The preferred fix would be to fix the build races, but that might take some time.
However, as the focus should be on successful builds, with build speed only a goal as long as successful builds are guaranteed, I'd be inclined to approve this revert to #2936, which was known to work.

My gripe is that this change to openblas would not belone to a PR that says "py-proteus: New package" in the title. Two gripes here:

• Reverting commit causing a build issue of openblas would not appear to be part of a commit adding a new package. These are two orthogonal issues and should have been submitted separately for more clarity and a more focussed review.
• This want me to say that I'm also not sure about the other changes in this PR: Are some of them likewise not required for the new package?

I understand that wanting to "getting things done" lents itself to submit all of the work done and needed for a reliable build across all tested platforms is a logical thing to do, but this PR changes 7 recipes that do not appear to be closely related to each other, so I must say that I don't like that from the viewpoint of somebody who's asked to assist in the merge of such PRs.

[cekees]

@bernhardkaindl could we go ahead and re-enable checks despite #6116 not yet being merged? The changes on it are small and shouldn't affect this PR. I've verified the spack package inpendently against the platforms I have access to and would like to wrap this up.

[Copilot]

🟡 Changes recommended

Unresolved critical dependency and SuperLU configuration issues, along with additional moderate build issues, remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

repos/spack_repo/builtin/packages/openblas/package.py:677

• The OpenBLAS shared target itself has libs and netlib as prerequisites, and those goals are phony. Since this loop already invokes libs and netlib, the third invocation rebuilds/rearchives both targets in a new make process, roughly doubling the library build work for every spec. Keep the targets serialized without invoking a phony-prerequisite target again (or otherwise make the shared step reuse the completed prerequisites).

            for target in ("libs", "netlib", "shared"):
                make("-s", target, *self.make_defs)

repos/spack_repo/builtin/packages/py_proteus/package.py:100

• Eigen installs its headers under include/eigen3, not directly under include (see eigen/package.py:108 and :147), and Proteus's setup includes that eigen3 subdirectory for its extensions. Adding only prefix.include to CPATH therefore leaves #include <Eigen/...> unresolved; add prefix.include.eigen3 for this dependency.

        for dep in ("eigen", "xtensor", "xtensor-python", "xtl"):
            env.prepend_path("CPATH", self.spec[dep].prefix.include)

repos/spack_repo/builtin/packages/py_proteus/package.py:58

• This dependency is unconditional even though parmetis is only consumed below when +pumi (and the Proteus PUMI extension is skipped for ~pumi). As a result, spack install py-proteus~pumi still concretizes and builds the expensive ParMETIS stack, defeating the optional PUMI variant. Make this dependency conditional on +pumi.

    depends_on("parmetis")

repos/spack_repo/builtin/packages/py_proteus/package.py:73

• The fallback returned here does not handle the standard external Debian/Ubuntu MPICH layout (<prefix>/include/<multiarch>/mpich/mpi.h). For such an MPI prefix, neither lib/*/mpich loop matches and the fallback makes Proteus look for <prefix>/include/mpi.h, so a valid external MPI installation fails during compilation. Detect this include layout (or pass the discovered include directory through the build configuration) before falling back to mpi_prefix.

        for candidate in glob.glob(os.path.join(mpi_prefix, "lib", "*", "mpich")):
            if os.path.isfile(os.path.join(candidate, "include", "mpi.h")):
                return candidate
        for candidate in glob.glob(os.path.join(mpi_prefix, "lib", "*", "openmpi")):
            if os.path.isfile(os.path.join(candidate, "include", "mpi.h")):

repos/spack_repo/builtin/packages/zoltan/package.py:127

• config_ldflags already contains -lgfortran above, so appending this -L option leaves the library search path after the -l lookup. On systems where GCC's Fortran runtime directory is not a default linker path—the case this change is meant to handle—configure can still fail to find libgfortran. Insert the directory before -lgfortran (or otherwise place -L before the library option).

                    config_ldflags.append("-L" + os.path.dirname(gfortran_lib))

• Files reviewed: 7/7 changed files
• Comments generated: 2
• Review effort level: Lite

[bernhardkaindl]

@cekees, as this refers to headers and libraries, it looks like this comment would be related to a potential build issue. but the build worked, so I'm not sure if this is an real issue. Please add a comment here and set it to resolved if you say that this comment does not apply.

[cekees]

Yes, in this current build configuration in proteus, it is setting the superlu include to the PETSc include dir and the lib dir to sys.exec_prefix/lib, ignoring SUPERLU_DIR. I'd rather fix the next proteus patch release, however, than drop the env.set here, meaning we should just leave this as is.

For a PETSc configured with --download-superlu, which is how proteus is most often built but deliberately not how spack builds it, the 1.9.0 proteus release is correct. For spack, petsc+superlu links spack's own superlu and doesn't vendor the headers (petsc-3.25.4-/include/slu_ddefs.h is absent), but the build succeeds anyway because depends_on("superlu") is direct and the compiler wrapper injects -I/-L implicitly: no -I...superlu appears in the build log, yet SuperLU_timer_ is linked into superluWrappers.so. The env var is the explicit statement of something that currently works through a side effect. Pointing SUPERLU_DIR at an arbitrary superlu is the behaviour we want to keep in proteus (and the spack recipe).

It's a future one-line change on proteus — get_flags(package) already reads _DIR. For example proteus/config/thunder.py:23 uses it as get_flags('superlu'). It used to be this way for other specific HPC machines that have been retired. It's just that default.py doesn't call it.

I filed it as erdc/proteus#1283 (with the dead lib64 assignment on line 299), keeping the current behaviour as the fallback when SUPERLU_DIR is unset. Put differently, once the upstream change applies, the recipe here will be correct, but it should also currently work anyway, it's just misleading until I repair proteus to honor SUPERLU_DIR.

[bernhardkaindl]

@cekees some questions found during review.

[bernhardkaindl]

Suggested change

	# Chrono compiles C as well as C++ (e.g. chrono_thirdparty/libstl/stlfile.c),
	# so both must be declared -- omitting "c" fails at cmake time with
	# "[spack cc]: Error: SPACK_CC_* variables not set".

Based on looking at other PRs, it looks like a comment only Claude Code would add,
but we usually do not explain that C/C++ packages need the C/C++ compilers, it's a given.

[bernhardkaindl]

Suggested change

	with open(header) as f:
	if "#include <type_traits>" in f.read():
	return
	filter_file(r"^#include <cstdio>$", "#include <cstdio>\n#include <type_traits>", header)
	filter_file(r"^#include <cstdio>$", "#include <cstdio>\n#include <type_traits>", header)

The worst that can happen is that type_traits is included twice, but its header guard should make that a non-issue.

[bernhardkaindl]

Suggested change

depends_on("fortran", type="build") # remove in future not a direct proteus dep

Remove it now if Proteus does not need a fortran compiler.

[bernhardkaindl]

Suggested change

	depends_on("py-cython@3:", type="build")
	depends_on("py-cython@3", type="build")

In https://github.com/erdc/proteus/blob/1.9.0/pyproject.toml#L2 it says Cython >=3.0,<4:

requires = ["setuptools>=61.0","cython>=3.0,<4","pybind11>=2.11,<3","numpy>=1.25,<3","mpi4py"]

[bernhardkaindl]

Suggested change

	depends_on("py-numpy@1.25:", type=("build", "run"))
	depends_on("py-numpy@1.25:2", type=("build", "run"))

In https://github.com/erdc/proteus/blob/1.9.0/pyproject.toml#L2 it says "numpy>=1.25,<3"

requires = ["setuptools>=61.0","cython>=3.0,<4","pybind11>=2.11,<3","numpy>=1.25,<3","mpi4py"]

[bernhardkaindl]

Suggested change

	depends_on("tetgen") # shells out
	depends_on("tetgen", type="run")

Same question.

[bernhardkaindl]

Suggested change

	depends_on("gmsh~fltk~med") # shells out to the gmsh, no fltk/med needed
	depends_on("gmsh~fltk~med", type="run")

Same question.

[bernhardkaindl]

Suggested change

depends_on("ncurses") # Fenton waves as text gui, generally not used

The comment is unclear to me. Is Ncurses directly used or not?
The search does not show me a place, only yml files specifying some build environments?
https://github.com/search?q=repo%3Aerdc%2Fproteus+ncurses&type=code

[bernhardkaindl]

Suggested change

depends_on("eigen@3.4") # xtensor dep

There should be no need to specify all indirect dependencies, that's the job of the concetizer. My test build on Linux without depends_on("eigen@3.4") was successful.

[bernhardkaindl]

Suggested change

	depends_on("libx11", type=("build", "link"))
	depends_on("libx11")

Build and Link are the default types, so the agument can be optimized out, AFAIK.

[bernhardkaindl]

@cekees CI checks have passed for the last push, but several questions are open after more review, so I've enabled draft status to not run CI for every small change until all questions are resolved.

Feel free to click the "Ready for review" button after all questions are answered and the PR is ready for another GitLab CI run and should be ready for merge.

[cekees]

I think all we're waiting on to move out of draft mode for #6116 to be merged into develop and then one more merge of develop into this branch.

[balay]

Just a note: petsc+superlu changes are merged in (develop) via #6917, so can be removed from this PR (befor a future merge or rebase with latest develop)