Plugin improvements

[rafaelab]

This PR improves the plugin examples, offering more information and flexibility for users developing their own plugins.

• Provide an advanced CMake finder for CRPropa.
• Expose and reorganise external libraries shipped with CRPropa (e.g., gtest, zstream-cpp, etc), allowing developers to directly link their plugins to them.
• Rename USE_CRPROPA_BUILTIN in plugin's CMakeLists to USE_OWN_CRPROPA to avoid confusion with SWIG's "builtin" feature.
• Minor bug fix related to the handling of variants in SWIG (this problem appears in the context of plugins).

[JanNiklasB]

Hey,

those are very nice improvements, I like especially the improved FindCRPropa.cmake.

I just noticed some minor things plus an issue in the python stubs generation, for which libcrpropa.so can not be found on first install since it is installed after the stubs command is called (this was caused by one of my PRs), this can easily be fixed by moving the code block (see comments) after the other installs.

With Regards,
Jan-Niklas

[rafaelab]

Thanks, @JanNiklasB.
I've implemented all your fixes, I guess
I was just a bit unsure about the PYBIND_STUBGEN part because this type of customisation should probably be optional, not the default. But you are right that it is harmless, so I brought it back.

Everything worked on my laptop and on the cluster. Since you seem to have a different setup, can you double check that you can install it?

[rafaelab]

• About the ./ for tests, I don't think it is an acceptable idea for an installation. It fixes conda and breaks custom installations. However, claude suggested me something that seems to be working: $<TARGET_FILE:${testName}>. Make sure it also work with conda.

• About gtest, I now replaced only the testing / gtest block according to a preference list: prefer an existing CMake target; try CMake’s find_package(GTest); then try CRPropa’s installed/bundled libraries, and only then fetch googletest.
  (This was a bit tricky to do in a consistent way. If you have a better suggestion, let me know).

[JanNiklasB]

$<TARGET_FILE:${testName}> would just find the path to the targeted binary (s. cmake-generator-expressions ).
So in our case it should be no different from using ${CMAKE_CURRENT_BINARY_DIR}.

My fix in mind would be something like:

Make the path dependent on a variable which defaults to ${CMAKE_CURRENT_BINARY_DIR}:

set(CRPROPA_TESTS_PATH ${CMAKE_CURRENT_BINARY_DIR} CACHE PATH "")  # rename to anything more appropriate
mark_as_advanced(CRPROPA_TESTS_PATH)
add_test(NAME ${testName} COMMAND ${CRPROPA_TESTS_PATH}/${testName})
# and for python
add_test(NAME ${testName} COMMAND ${Python_EXECUTABLE} ${CRPROPA_TESTS_PATH}/${testName}.py)

then change the variable manually in conda-bld/build.sh (last line):

cmake .. -G Ninja \
	-DCMAKE_PREFIX_PATH="${PREFIX}" \
	-DPython_EXECUTABLE="${PYTHON}" \
	-DPython_NumPy_INCLUDE_DIR="${NUMPY_INCLUDE_DIR}" \
	-DPython_INCLUDE_DIR="${PYTHON_INCLUDE_DIR}" \
	-DPython_INSTALL_PACKAGE_DIR="${SP_DIR}" \
	-DCMAKE_INSTALL_PREFIX="${PREFIX}" \
	-DBUILD_DOC=OFF \
	-DDOWNLOAD_DATA=OFF \
	-DENABLE_COVERAGE=OFF \
	-DENABLE_GIT=ON \
	-DENABLE_HDF5=ON \
	-DENABLE_OPENMP=ON \
	-DENABLE_PYTHON=ON \
	-DENABLE_QUIMBY=OFF \
	-DENABLE_SWIG_BUILTIN=ON \
	-DENABLE_TESTING=ON \
	-DFAST_WAVES="${FAST_WAVES}" \
	-DOMP_SCHEDULE=dynamic \
	-DSIMD_EXTENSIONS="${SIMD_EXTENSIONS}" \
	-DUSE_ABSOLUTE_RPATH=ON \
	-DCRPROPA_TESTS_PATH="${PREFIX}/share/crpropa/test/"

[rafaelab]

I think this version is cleaner, @JanNiklasB .
The conda build seems to work.

The only thing I'm unsure about is the plugin's CMakeLists correctly finding gtests for older CRPropa versions.
I tried a few different things, some worked, some didn't, and the one that hasn't given me problems is the current one.

[JanNiklasB]

I think your way of finding gtest should in principle work in any case, only the includes might not be set every time (s. comment).

Furthermore, the dependencies of the python tests on the test that copies the crpropa python files should probably be placed inside the addtestPy function so that the dependency is set for all potential tests.

[JanNiklasB]

The tests are only failing because of typos in line 302 and 303 (see suggestions)
Other then that it now looks good to me.

[rafaelab]

I'm still working on my last attempt, @JanNiklasB .
If it doesn't work, I won't try to make gtest work with older CRPropa versions.
I will let you know when it's done

[rafaelab]

This version (current) is almost my last attempt, @JanNiklasB. I will just fix one more thing based on the outcome of our discussion.

As you can see, one of the tests fail because of the pybind11-stubgen (in OSX only). I had originally removed all references to pybind from CMakeLists.txt because it does not pertain to CRPropa (at the moment). In my opinion, it should not even have been included in the cmake file.
It is one more thing to support and it makes no difference for the science output of most users.

However, since it is already there, it can be offered as an option. Naturally, it should only be activated by the users who really want it and be kept off by default. That can be done with a cmake flag ENABLE_PYBIND11_STUBGEN.
It keeps the code clean from a feature that is not needed by most users and developers.

The question then becomes: should it be part of the conda release? To me this is a clear no, especially because it is making a test fail. It will just increase the size of the package and increase the runtime of the workflows.
If it is removed from meta.yaml, the conda package will likely work just fine.

The way it currently is, it is acting as a strong dependence and I really don't see the practical advantage of having it in the default installation and conda release.

I am adding this to the PR, since it is needed to make the tests pass, but I'd like to hear from you first.

[JanNiklasB]

It is not failing because of pybind11-stubgen, what is failing is the python import. This is caused by your removal of

  if(APPLE)
    set_target_properties(crpropa-swig PROPERTIES
      INSTALL_RPATH "@loader_path;@loader_path/../../${PYTHON_PROJECT}/.libs"
    )
    set_property(TARGET crpropa-swig APPEND PROPERTY
      LINK_FLAGS "-flat_namespace -undefined suppress"
    )
  endif()

(Compare #548 )

pybind11-stubgen is not a requirement we do but an option since we do not built any stubs when it is not available.
In the conda package the stubs take only minimal space (~ 600KiB in raw form, much less in compressed package form, compare that to 6.3 MiB for our required python files and the 3MiB total size of our package (header and libraries not yet included)).

It enables the linting which is typically done by pylance and makes our documentation available without looking it up per help() function or by searching through the manual, so even if it is not scientific it is still very useful for most users.

[rafaelab]

Hm... Then I misunderstood the intent.
I thought the goal was more to prepare for your own Pybind extensions rather than for improved documentation.
In this case, you are right, it is can be very useful.

In any case, I will add it as optional, but keep the default to On.

[JanNiklasB]

I see why it seemed that it is needed for pybind11 extensions (it's in the name after all). Since swig does not generate proper modern stub files I searched for a third party alternative but it seems pybind11-stubgen is also the best option for swig...

I like adding a option, I was hesitant with that since I did not want to create the implication that we would require pybind11-stubgen for CRPropa to work, but we also do not require fftw and hdf5 for CRPropa to work and still have the option.

Was that the last thing you wanted to add? If so I would approve it from my side.

[rafaelab]

Yes, I got confused because I know you were working on pybind wrappers, so I thought you wanted to add something in the current version to prepare for that. Sorry about that.
I think an option set to true by default is harmless and gives more control to the users.

From my side, I'm done with this PR if all tests pass.

Thanks for the review, @JanNiklasB!