mctools / ncrystal

NCrystal : a library for thermal neutron transport in crystals and other materials
https://mctools.github.io/ncrystal/
Other
38 stars 17 forks source link

The python extension installs the C++ library #151

Closed yurivict closed 8 months ago

yurivict commented 8 months ago

The Python extension should use the pre-installed ncrystal library and headers and build and install the extension.

Currently pyproject.toml builds and installs the C++ library, headers and utilities. This way the python extension conflicts with the ncrystal C++ package.

Since multiple Python versions can be installed concurrently, python extensions should only contain the extension, and it should be linked with the C++ library.

tkittel commented 8 months ago

Thanks for this report as well! So I might be confused here about what you are trying to do, but as far as I know we don't have an "NCrystal C++ package" and an "NCrystal Python package", but merely a single NCrystal package which can be installed either by Python tools (pip) or cmake - but not both at the same time.

It is not a perfect world though, and I would be happy to get advice on how to improve this. Currently, if installing via pip one gets an installation which will not be detected by downstream CMake code invoking find_package(NCrystal) unless the downstream code first invokes ncrystal-config --show cmakedir and adds the resulting path to the CMAKE_PREFIX_PATH. Conversely, if installing via CMake, the NCrystal Python modules do not get added to the correct site-packages directory or PYTHONPATH, unless one uses the activation scripts discussed in #150.

These issues can of course be addressed if packaging for a particular system (e.g. our conda-forge package is fully functional in all respects out of the box), but I have been unable to find any online resources about how this would be solved in general.

tkittel commented 8 months ago

You might be interested in how our conda-forge recipe handles this currently: https://github.com/tkittel/ncrystal-feedstock/blob/main/recipe/build.sh

Admittedly it is not pretty.

yurivict commented 8 months ago

It would be an easy solution to add a build argument that would control whether the python project would install everything, or only the Pythin part.

The way how it is now is python centric, but very packaging unfriendly.

tkittel commented 8 months ago

I am not sure I really understand exactly how that should be done. If you think it would be easy, could you please open a PR and show me?

tkittel commented 8 months ago

But there is separate issue: It sounds to me like you want to add two distinct packages for NCrystal on freeBSD? I.e. ncrystal-cpp and ncrystal-python? That doesn't sound right to me, on all other channels we have a single ncrystal package which installs everything.

tkittel commented 8 months ago

I am wondering if part of the confusion is that you see the pyproject.toml file and think you therefore have to install the python modules with that file. However, that is not necessarily the case - the pyproject.toml file is a very recent addition, and merely provides an alternative to invoking CMake directly.

yurivict commented 8 months ago

It sounds to me like you want to add two distinct packages for NCrystal on freeBSD? I.e. ncrystal-cpp and ncrystal-python? That doesn't sound right to me, on all other channels we have a single ncrystal package which installs everything.

Having separate packages for C++ and Python is considered to be the correct solution in general.

For example, when some C++ app would use ncrystal, the combined C++/Python package would bring in all the Python infrastructure when this is completely unnecessary. This causes people to complain that everything is bundled with everything, and dependencies are out of control.

The above is true for all libs that have Python bindings, not only for ncrystal.

I do packaging for FreeBSD for many years, and encountered this issue many times.

tkittel commented 8 months ago

I see your point, although for a project with limited manpower like NCrystal we also have to consider the implication on complexity and increased testing and maintenance of yet another installation option. And note that our python modules don't necessarily come with any non-optional dependencies. Still, I am of course more than happy to consider any contribution to the project along the lines you suggest, preferably in the form of a pull request (which you indicated should be easy).

Cheers, Thomas

yurivict commented 8 months ago

After the second look I see that only some scripts are installed by the python installed besides the python extension. The C++ library and headers aren't installed separately, which is good.

So we would just delete the scripts in bin/* and this would be ok for the python extension port.

Thank you for your help!

tkittel commented 7 months ago

That's great @yurivict. I have opened #152 nonetheless to ensure I get back to this eventually - for now such a split is not actually really supported, although if you found a way to make it work, great.

Just make sure as a check that the following commands work in your installation to check that you got the interplay between the two packages correct:

nctool --test
python3 -c 'import NCrystal; NCrystal.test()`

And ncrystal-config -s should show something reasonable. In particular ncrystal-config --show libdir should indicate the directory of libNCrystal.so, and ncrystal-config --show includedir should be the correct include path (i.e. containing NCrystal/NCrystal.hh).

It is also a good check that find_package(NCrystal) works in CMake code.

If all of those checks work, you can be pretty confident that you are providing a fully functioning NCrystal installation.

yurivict commented 7 months ago

Thank you, @tkittel.

I tried all commands that you've mentioned and all of them succeeds and produce reasonable results on FreeBSD. So it looks like the NCrystal package is installed correctly.

Cheers, Yuri