Details

Reviewers

Brecht Van Lommel (brecht)
Joerg Mueller (nexyon)
Sybren A. Stüvel (sybren)

Group Reviewers

Platforms, Builds, Tests & Devices

Commits

rB010f44b855ca: Fix several issues with handling of numpy in CMake.
rB5d570c875eda: Fix several issues with handling of numpy in CMake.

Summary

Issues were:

Abusing of WITH_PYTHON_INSTALL_NUMPY by both Audaspace and Mantaflow.
- PYTHON_INSTALL options only decide whether we copy python (and some extra modules) in our Blender installation. On linux it makes much more sense to use global python installation.
- Now we have instead a proper WITH_PYTHON_NUMPY
Bad assumptions regarding path of headers relative to path of python module.
- In current Debian testing, modules are under python3.9 directory, while headers are under python3 directory.
- Now we properly find_path for headers as well, modifying find_python_package to take an optional argument for headers.

Note that some of the required changes are done to extern libraries
and should therefore be propagated upstream too.

Diff Detail

Repository: rB Blender

Event Timeline

Bastien Montagne (mont29) requested review of this revision.Dec 7 2020, 3:38 PM

Bastien Montagne (mont29) created this revision.

Harbormaster completed remote builds in B11589: Diff 31689.Dec 7 2020, 3:38 PM

Brecht Van Lommel (brecht) added inline comments.Dec 7 2020, 3:49 PM

CMakeLists.txt
1626	`WITH_AUDASPACE AND NOT WITH_SYSTEM_AUDASPACE` -> `WITH_PYTHON_NUMPY`? To ensure it finds the package when `WITH_PYTHON_NUMPY=ON` and `WITH_PYTHON_INSTALL_NUMPY=OFF`.

I would expect these extern files that were modified do exist upstream, but I could be wrong.

This revision now requires changes to proceed.Dec 7 2020, 3:50 PM

Update from review.

Harbormaster completed remote builds in B11597: Diff 31701.Dec 7 2020, 5:36 PM

In D9773#242898, @Brecht Van Lommel (brecht) wrote:

I would expect these extern files that were modified do exist upstream, but I could be wrong.

To be clear, I do not intend to commit changes to code in extern, those were mainly to ensure it works as expected, and to notify @Joerg Mueller (nexyon) and @Sebastián Barschkis (sebbas) about the need to update their side of the code too.

Sorry I mistyped, I meant these files do not exist upstream.

This revision is now accepted and ready to land.Dec 7 2020, 5:45 PM

Right, the numpy files from mantaflow are missing in /extern/plugin. While numpy support has been disabled in the fluid modifier, those files should still be added. Just for completeness.

Correct, the blender_config.cmake, does not exist in upstream audaspace.

Sybren A. Stüvel (sybren) added a subscriber: Sybren A. Stüvel (sybren).Dec 8 2020, 10:50 AM

Sybren A. Stüvel (sybren) added inline comments.

CMakeLists.txt
353	NumPy is not used during the build, so this sentence is a bit weird. I think "Copy the NumPy library into the blender install folder" is clearer and consistent with the `WITH_PYTHON_INSTALL` description. Also note the capitalisation of NumPy.
build_files/cmake/macros.cmake
1218–1223	This list is incomplete, `${PYTHON_LIBPATH}` is missing (the first path in `HINTS`).

In D9773#242963, @Brecht Van Lommel (brecht) wrote:

Sorry I mistyped, I meant these files do not exist upstream.

Ah OK, then will have to commit them as well.

Cleanup NumPy casing in messages.

Harbormaster completed remote builds in B11611: Diff 31728.Dec 8 2020, 11:38 AM

Bastien Montagne (mont29) added inline comments.Dec 8 2020, 11:39 AM

CMakeLists.txt
353	Actually, the whole point of this new option is because numpy is used during the build, since we need its headers... We need those even if we do not install python and its extra modules.
build_files/cmake/macros.cmake
1218–1223	Same as above existing message for `PYTHON_NUMPY_PATH`... TBH I would just remove all those lists of tried paths (or if we really want to keep them generate them with some loops), don't see much of the point of having them in the error message anyway, But that would be outside of this patch in any case.

Sybren A. Stüvel (sybren) added inline comments.Dec 8 2020, 12:42 PM

CMakeLists.txt
353	Ah, I see where my confusion is coming from: I thought of "using numpy" as doing `import numpy` and then using it to compute stuff. How about making it less specific, so that it basically covers all? "Include NumPy in Blender (used by Audaspace and Mantaflow)" removes the details on where & how exactly it's used, and to me covers enough information to understand the option.

Sybren A. Stüvel (sybren) added inline comments.Dec 8 2020, 12:43 PM

build_files/cmake/macros.cmake
1218–1223	Yes, having them in some form that allows passing to `find_path()` and generating the errors would be nice. I think it's a nice touch to list which directories were searched, though, as that can really help in debugging build issues. I agree this is outside the scope of this patch.