NCrystal becomes runtime rather than buildtime dependency

[tkittel]

Description

As discussed on #3304, this PR makes NCrystal a run-time rather than a build-time dependency, also for the C++ layer of openmc (it was always a run-time only dependency in the Python layer).

This has a lot of advantages, primarily:

• No hassle trying to matrix in NCrystal=yes and NCrystal=no in binary distributions like at conda and the incoming python wheels by @ahnaf-tahmid-chowdhury, and the ensuing headache of wondering about ABI stability and when a new NCrystal release will require a rebuild of OpenMC packages.
• People using NCrystal will no longer be forced to perform a custom rebuild of OpenMC, they simply have to also install the ncrystal package. So any build of OpenMC (conda, python wheels, manual builds) will work with any build of NCrystal (conda, python wheels, manual builds), assuming just a simple platform compatibility (i.e. you still can't be silly and try to mix openmc-aarch64 packages with ncrystal-x86 of course).

And note that for users not adding NCrystal materials to their OpenMC geometries, there should be ~0 overhead. And people adding NCrystal materials to their geometries will get a nice pedagogical error message if NCrystal is not already installed, telling them to install it. Once they have done so, they can simply rerun and it should just work.

So on the NCrystal / ESS side we will be super happy with this, since it means we can use OpenMC+NCrystal out of the box, and hopefully other OpenMC developments will also become easier going forward, since there will be less of a need to worry about configuration and build issues related to NCrystal.

The technology behind all of this is simply to load NCrystal as a dynamic plugin with dlopen, like I can see is already done for compiled sources in OpenMC. Thus, the -ldl (from CMAKE_DL_LIBS) is already used for OpenMC builds, and there is no new dependency introduced there.

The ability to actually load and use NCrystal via an abstract plugin interface was introduced in the recent NCrystal 4.1.0 release, and has been already tested to work in NCrystal's CI on macOS/Ubuntu/Windows/aarch64/intel, so I am pretty confident that this is solid :-)

Checklist

• I have performed a self-review of my own code
• I have run clang-format (version 15) on any C++ source files (if applicable)
• I have followed the style guidelines for Python source files (if applicable)
• I have made corresponding changes to the documentation (if applicable)
• I have added tests that prove my fix is effective or that my feature works (if applicable)

[tkittel]

@shimwell @ahnaf-tahmid-chowdhury @paulromano FYI.

Note that I converted the PR to draft for now, since there might be a few issues related to docs/source/usersguide/install.rst and doxygen comments. However, feel free to comment already :-)

Cheers,
Thomas

[tkittel]

I forgot to mention, the infrastructure for discovering NCrystal (via the ncrystal-config command) and calling out to dlopen+dlsym or LoadLibrary+GetProcAdress, and wrapping the abstract NCrystal interface into a small helper class NCrystalScatProc is all encapsulated in two new files ncrystal_load.h and ncrystal_load.cpp.

[marquezj]

Thanks @tkittel. This will simplify a lot the deployment of OpenMC with NCrystal materials.

From what I see you need to remove the CMake option for NCrystal in tools/ci/gha-install.py.

[tkittel]

Thanks @marquezj .

I just wanted to verify that all CI passed with the current commits, which it actually did. I removed ncrystal lines in that file + I spotted a few that I missed in src/output.cpp (basically ncrystal is no longer a build option, so there is no need to accept or print flags related to whether or not ncrystal is enabled).

[tkittel]

I still need to look into doxygen strings in the changed C++ code, but apart from that I feel this is ready for review (unless my last two commits broke the tests, which they should hopefully not).

[tkittel]

So I have tried to add some doxygen comments now, hope all is OK :-)

[tkittel]

Could someone let me know:

1. Is the CODEOWNERS file in a correct format now? (ncrystal_interface.cpp is no longer the only ncrystal file, there is also ncrystal_load.h/.cpp).
2. How can I actually view what doxygen does with the comments? I could not find any reference to the new code in the readthedocs output.

[marquezj]

From my side the only remaining issue is the CODEOWNERS file.

It might be useful in the future to check that the interoperability also works in other platforms, but currently CI is only set up for Ubuntu.

[tkittel]

Thanks @marquezj @paulromano for the feedback and invite. I have updated CODEOWNERS to also have src/ncrystal_load.cpp, and did a rebase for good measure.

Concerning other platforms than ubuntu, I actually have verified that the mechanism works on all platforms (Windows, macOS, Linux, Intel, Arm, ...), since before releasing NCrystal 4.1.0 with the new APIs, I added a standalone example here, which essentially exercises the ncrystal_load.h/.cpp files (they are basically the same files as those submitted to OpenMC in this PR, apart from modifications for OpenMC coding style).

You can see a workflow exercising this at https://github.com/mctools/ncrystal/blob/main/.github/workflows/plugindb.yml (recent run here), in which the small app is built without any build-time dependency on NCrystal, and then it is used with an NCrystal installation only at run-time. For fun I even build NCrystal wheels and the example app on separate machines, before running them together on a third machine.

So I fully expect that when/if OpenMC adds more CI platforms in the future, that this will all just work :-)

[tkittel]

Ok, so now all tests passed again. Any more suggestions or comments, or is this ready to merge?

[ahnaf-tahmid-chowdhury]

Thank you very much for the update! It is truly very streamlined.

[marquezj]

Good work. I think it is ready to merge.

[paulromano]

@tkittel Thanks a lot for this PR! The simplifications to the build process and distribution are very compelling. The only part here that makes me a bit uneasy is the call to popen, which I'd prefer to avoid if it were at all possible. If the shared library path were provided by some other means, would it be possible to have an API call to NCrystal provide the remaining pieces of information (intversion, symbol_namespace)?

[tkittel]

@paulromano after several years and iterations of considering this, I honestly do not think there is any better alternative than querying ncrystal-config for the library path. Any other solution would inevitably be specific to each deployment mechanism (e.g. potentially different for Python wheels, conda builds, manual builds of NCrystal, debian packages, freebsd ports, etc.). Or it would require users to mess around with fiddly bits like environment variables, adding stuff to xml, or flags to openmc or whatnot. On the NCrystal side, we are devoted to always ensuring that any installation of NCrystal will have a correct functioning ncrystal-config, since that is how we now have chosen we want to support it (and it is how the NCrystal python modules look for the shared library).

Is your concern that this would break non-NCrystal users' usage of OpenMC, or that it would not work for people using NCrystal in OpenMC? For the former, they would not trigger the relevant code-paths at all, so it should be a non-issue (and popen is posix + in windows std libs). For the latter, I would say let us go ahead and let us see if any unexpected problems would surface down the line. If it would become necessary for some very specific use-case to provide the information via a compile time flag to OpenMC or perhaps an environment variable, we could always add it then. However, I would prefer not to add more complicated options until there is more than a theoretical need for them (after all, there would have to be some CI tests of these options or we could not rely on them anyway... and we would have to maintain docs as well, etc.).

As I mentioned, the mechanism has been tested elsewhere on a lot of platforms and seems to work without issue so far.

Cheers,
Thomas

[paulromano]

Thanks for the quick response and explanation @tkittel. My reluctance is more about the philosophical idea of OpenMC spawning some other executable, and of using non-standard headers in the C++ code, although I fully understand that popen, dlopen, etc. are practically guaranteed to be available on any conceivable system. And of course we already use dlopen for custom source routines. So, I may just have to accept that practicality beats purity here 😄

If it's OK with you, I might do a little refactoring on this branch to separate out the dynamic linking calls to be reusable for this and custom source routines (which would have the benefit of enabling them to be used on windows too).

[tkittel]

@paulromano I fully understand your uneasiness, but I hope I can convince you nonetheless since I believe this will be the most head-ache free and convenient path forward :-)

And of course, feel free to further refactor on this branch, I will keep my hands off it in the meantime :-)

[paulromano]

Alright, I think I'm satisfied with where this is. Thanks for working through my comments @tkittel. Can you take a quick look through my last changes in ncrystal_load.cpp simplifying the ifdefs and make sure you're OK with it?

[tkittel]

Thanks @paulromano ! Your changes look very OK, you even found another unused variable.

Which is surprising since I thought I had actually tried to compile with -pedantic -Wall -Wextra, but apparently not. I did so now and found another unused variable left over. So I fixed that as well, and now there really should be no more! Geez.

Btw., can I ask for the comments you left next to each include concerning what they bring in, are you using some code inspection tool, or do you simply have all the header files and the C++ standard in your head? ;-)

Anyway, all looks OK for me!

[paulromano]

can I ask for the comments you left next to each include concerning what they bring in, are you using some code inspection tool, or do you simply have all the header files and the C++ standard in your head?

I do it manually, usually consulting online documentation like https://devdocs.io/cpp. I wouldn't be surprised if there are automated ways to do this though! I don't personally use it but maybe something like include-what-you-use can do this?