BUG: Depend upon Python / pybind11 only if requested

[Leengit]

When the cmake option ENABLE_PYEXP is OFF, we don't need Python or its pybind11.h include file. However, currently, a build environment that has Python will try to include pybind11.h even when ENABLE_PYEXP is OFF and pybind11.h is not available.

The code change checks the value of ENABLE_PYEXP and, when it is OFF, the build does not try to use any part of Python, including pybind11.h.

[The9Cat]

The n-body side also uses DiskDensityFunc in exputils to allow for user-defined Python functors.

The organization here is that exputils hosts code used both by the n-body code and pyEXP.

If we want no Python support in a minimal build, we'll want to exclude Python support based on ENABLE_MINIMAL not ENABLE_PYEXP.

[The9Cat]

Thinking more about this: ENABLE_MINIMAL was requested by the Gala developers. They may or may not want Python support so I'm worried that this might break their builds. Perhaps it would be better to have a DISABLE_PYTHON CMake option to exclude Python is not wanted and ENABLE_PYEXP is no?

[Leengit]

Thank you for the quick response. In case I am not the only one that needs to learn more ...

• Beyond the files I've already touched, it looks like I should take a look at src/Cylinder.cc, utils/ICs/initial.cc, include/DiskModels.H, expui/BiorthBasis.H, and expui/CMakeLists.txt, and any files that depend upon those .H files.
• If the "user defined Python functors" is not the same as pyEXP, we could go with two distinct cmake flags. Would it be better to match existing flags and have them both be enablers, such as ENABLE_PYEXP and ENABLE_PYTHON (or ENABLE_PYTHON_FUNCTORS or some such name), with appropriate defaults?
• Are these two flags independent or would enabling one of them require the enabling of the other to be sensible? We could have cmake output an error for a nonsensical combination.
• We could have ENABLE_MINIMAL, which currently affects 5 enable flags, also affect the ENABLE_PYTHON_FUNCTORS flag in whatever way Gala is hoping for. Whom do we ask at Gala?
• We might want to also determine a default for the ENABLE_PYTHON_FUNCTORS flag when ENABLE_PYEXP_ONLY is set; it too currently affects the same 5 enable flags as ENABLE_MINIMAL. Whom do we ask about this?

[The9Cat]

Are we sure that this really needs fixing? Or perhaps I should ask directly: what is your goal in eliminating Python support from the code base?

[The9Cat]

I'll also point out: DiskDensityFunc already has a fallback for the case that ENABLE_PYTHON3 is no: the functor wrapper will throw an exception when called if there is no Python development environment. So it should be sufficient to suppress the CMake find_package(Python3...) call in the top-level CMake.

[The9Cat]

My suggestion here is:

1. Add a configure option like

option(DISABLE_PYTHON "Disable the Python development environment" OFF)

at line 69 in CMakeLists.txt.
2. Put line 301 in an if block like:

if (NOT DISABLE_PYTHON)
  find_package(Python3 COMPONENTS Interpreter Development)
endif()

Revert all of the other changes and that should suffice. That's very minimal and unlikely to break anything else. We should fix the typos that you found in DiskDensityFunc also.

In principle, Gala is intended to have the entire basis construction stack which includes Python functors. Gala itself wouldn't care, but since it's a Python library, it has the Python environment anyway, so I'm reluctant to pull that out by default. Rather, I think it's better to add a separate exclusion for Python. So vitiating the intent of ENABLE_MINIMAL perhaps but DISABLE_PYTHON is really removing a feature.

One final comment. I think this is really a new feature, not a bug fix, so I'd like to rebase against devel as a PR, following our usual policy.

[Leengit]

Okay, I'll do that. Thank you.