Support for $METHOD other than opt

[roystgnr]

Using METHOD=oprof (to build with performance monitoring instrumentation), devel (to build with assertions and debugging symbols enabled), and/or dbg (to use more expensive libMesh and libstdc++ assertions, and to disable optimization to make stepping through a debugger easier) are extremely useful, but we can't get a usable Cardinal build with them right now.

The culprit seems to be contrib/openmc/cmake/Modules/FindLIBMESH.cmake - it's looking for libmesh-opt.pc, specifically, with no way to override that at the moment, but that ought to be easy enough to fix and send upstream. If I need more debugging support (which I probably will, shortly) then I'll do so.

Building with more than one method at a time would be harder - use multiple out-of-source builds for dependency libraries, maybe - so that's not going to be a near-term goal.

[pshriwise]

Hey @roystgnr, I wanted to make you aware of the PR containing the version of FindLIBMESH.cmake that's going to go into OpenMC (hopefully really soon). It looks for libmesh.pc instead of the "opt" version specifically.

https://github.com/openmc-dev/openmc/blob/434c8308cff80a7fb131426779ce62ad96f2040e/cmake/Modules/FindLIBMESH.cmake

With the update from this libMesh PR that selects the content in libmesh.pc based on the METHOD/METHODS variable, I think we should be able to rely on the METHOD variable to build Cardinal in any of these modes.

As @RonRahaman has mentioned to me before, it's probably even better to have FindLIBMESH.cmake select a package config file to look for based on the METHOD variable and fall back onto libmesh.pc in its absence.

Mainly wanted you to know there may be a fix on the way to avoid duplicated effort :)

[roystgnr]

It looks for libmesh.pc instead of the "opt" version specifically.

This seems to have worked for me as a short-term workaround; thanks!

As @RonRahaman has mentioned to me before, it's probably even better to have FindLIBMESH.cmake select a package config file to look for based on the METHOD variable and fall back onto libmesh.pc in its absence.

This is exactly the right thing to do in the medium term. Right now to keep things as simple as possible I've got each cardinal build in a separate directory, each pointed to its own separate libMesh build, each of which is only built for one METHOD, but that's not something people can do if they want to use an external libMesh.

Mainly wanted you to know there may be a fix on the way to avoid duplicated effort :)

I appreciate it. I've only worked a little with cmake, long in the past, and trying to do so again this morning has been dismaying (Everyone's using global variables instead of function arguments and return values??) and frustrating (I see people using $ENV, but I don't see why it's not picking up the environment variables I'm querying when I try to use it...)

[RonRahaman]

Right now to keep things as simple as possible I've got each cardinal build in a separate directory, each pointed to its own separate libMesh build, each of which is only built for one METHOD, but that's not something people can do if they want to use an external libMesh.

@roystgnr I think having the OpenMC and nekRS build directories selected based on METHOD would take care of a lot of this nastiness. I think I mentioned this on Slack and then I didn't follow up. Shall I put in a PR for this today?

[RonRahaman]

As @RonRahaman has mentioned to me before, it's probably even better to have FindLIBMESH.cmake select a package config file to look for based on the METHOD variable and fall back onto libmesh.pc in its absence.

I actually think that CMAKE_BUILD_TYPE would be a better way for OpenMC to infer which build of libmesh to use. CMAKE_BUILD_TYPE is used to determine the optimization and debug flags for OpenMC itself, so you'd probably get a more consistent build if you're linking a "Debug" build of OpenMC to a "dbg" build of libmesh (and likewise, a "Release" build of OpenMC to a "opt" build of libmesh).

[RonRahaman]

It's definitely a good idea to use METHOD in the Cardinal makefiles, since that's the expected mechanism in the MOOSE-style makefiles. From that, we can set CMAKE_BUILD_TYPE when we configure OpenMC (as we're doing now).

However, it's a little odd to use METHOD in OpenMC itself, since CMAKE_BUILD_TYPE is the expected mechanism to control OpenMC builds.

[roystgnr]

I think having the OpenMC and nekRS build directories selected based on METHOD would take care of a lot of this nastiness. I think I mentioned this on Slack and then I didn't follow up. Shall I put in a PR for this today?

As far as I'm concerned, I've now got both a working opt build and a working dbg build (and what should lead to a working devel build), so no need for you to rush on my account. I do think that's a great long-term step though so I'm not going to object if you want to do it now.

I actually think that CMAKE_BUILD_TYPE would be a better way for OpenMC to infer which build of libmesh to use. CMAKE_BUILD_TYPE is used to determine the optimization and debug flags for OpenMC itself, so you'd probably get a more consistent build if you're linking a "Debug" build of OpenMC to a "dbg" build of libmesh

I only wish. If dbg libMesh was just about turning off optimization and the NDEBUG macro and turning on the DEBUG macro and debugging symbols, then sure, that'd be close enough.

But, depending on --enable-glibcxx-debugging settings, two different dbg libMesh builds can be ABI incompatible with each other! There's no way to ensure a compatible application build here without actually scarfing the specific compiler settings from e.g. libmesh-config.

[RonRahaman]

There's no way to ensure a compatible application build here without actually scarfing the specific compiler settings from e.g. libmesh-config.

That's totally workable. In libmesh builds of OpenMC, we can use libmesh-config to initialize the relevant CMAKE flags, and that can be done in OpenMC's CMakeLists. Regardless, it would use CMAKE_BUILD_TYPE from OpenMC.

I guess my point is that OpenMC user could accidentally set inconsistent CMAKE_BUILD_TYPE and METHOD flags. In standalone OpenMC builds, the CMakeLists would need to check for consistency or just rely on one. Most OpenMC and CMake project users are accustomed to using CMAKE_BUILD_TYPE, so it would be more natural for them.

[pshriwise]

I guess my point is that OpenMC user could accidentally set inconsistent CMAKE_BUILD_TYPE and METHOD flags. In standalone OpenMC builds, the CMakeLists would need to check for consistency or just rely on one. Most OpenMC and CMake project users are accustomed to using CMAKE_BUILD_TYPE, so it would be more natural for them.

Yeah, that's a good point about avoiding direct adoption of the METHOD variable in OpenMC @RonRahaman. Right now we use a bunch of CMake options to control compiler flags, so I think we'd want to keep using those.

https://github.com/openmc-dev/openmc/blob/e2842cfef3e842ab62820ae4786feddf907aaae9/CMakeLists.txt#L28-L33

@roystgnr is the information in the package config files for a given libMesh installation the same as what one would get from libmesh-config? If so, would it suffice to use those to determine what libMesh .pc file to search for and fall back on libmesh.pc with a warning that the package config file doesn't match OpenMC's settings?

The nice thing about relying on CMake's PkgConfig module is that it then imports libMesh as a CMake target and takes care of these flags for us. Maybe I'm off track here though.

[RonRahaman]

Right now we use a bunch of CMake options to control compiler flags, so I think we'd want to keep using those.

The nice thing about relying on CMake's PkgConfig module is that it then imports libMesh as a CMake target and takes care of these flags for us. Maybe I'm off track here though.

Yeah, if that's the case with PkgConfig (and I do believe you're right), then using PkgConfig would be better than using libmesh-config, as I proposed. So in OpenMC's CMakeLists, I'm in favor of using the OpenMC flags (--profile, --optimize) as the user-facing options; and based on those flags, getting the appropriate libmesh config flags from PkgConfig

[roystgnr]

I guess my point is that OpenMC user could accidentally set inconsistent CMAKE_BUILD_TYPE and METHOD flags.

IMHO that'd actually be something good to support! I work in a similar manner all the time, combining an optimized PETSc build with a debug libMesh build, because I'm typically looking for errors in newly-added libMesh code but I'm not expecting to trigger any errors in PETSc (except via bad inputs that ought to be caught by the dbg-mode sanity checks in our own PETSc shim classes).

One could use METHOD to figure out which libMesh to link against and what flags it wants, use CMAKE_BUILD_TYPE to determine what flags OpenMC could get, and simply combine flags, with OpenMC flags coming second. This would partly work just like we want: compilers are pretty good about allowing inconsistent flags and letting later flags override earlier ones, so if libMesh flags included -D_GLIBCXX_DEBUG -O0 and OpenMC flags added afterward included -O3 then in the end we'd get the effect of -D_GLIBCXX_DEBUG -O3, which would be weird but would be just what we'd want for an optimized OpenMC build used with a debug and --enable-glibcxx-debugging libMesh+Cardinal. What wouldn't work so well would be the NDEBUG and DEBUG macro settings ... but it looks like OpenMC doesn't currently use those (or the assert() macro that depends on NDEBUG) so it wouldn't be a near-term problem at least.

@roystgnr is the information in the package config files for a given libMesh installation the same as what one would get from libmesh-config?

I haven't really been looking at those, but it looks like the answer is "it's not the same, but it's sufficient"? In libmesh-*.pc we get the important macros defined via flags on the Cflags: line, and we get our compiler warning+optimization+debugging+profiling flags defined on the cxxflags_extra line. If the cmake pkgconfig module is using the Cflags: flags then it should be giving us something ABI compatible. So maybe the solution is to use METHOD to find the necessary Cflags:, but then ignore cxxflags_extra entirely in favor of whatever CMAKE_BUILD_TYPE recommends?

[roystgnr]

Hmm... short-term and medium-term solution, anyway. Long-term, if OpenMC or Cardinal or other sources here are going to start using assert() or other compile-time conditionals which depend on NDEBUG and/or DEBUG, it'll make sense to manually follow up libMesh macro definitions with your own -D and -U settings, so you can build Cardinal-with-asserts against libMesh-without or vice-versa.

At the moment (and so far as we intend for the future, though we don't have coverage in CI for it and it has broken once in the past...) libMesh doesn't let those macro settings affect the ABI, so you can use one setting for libMesh along with a different setting for higher level code.

[pshriwise]

I think it's safe to assume we won't be using any of those types of constructs in OpenMC anytime soon, so linking OpenMC and libMesh with different "methods" should be alright.

To keep it simple I think what I would recommend is attempted detection of a METHOD environment variable in OpenMC's FindLIBMESH.cmake file. If that variable is present, then we'll fail if we can't find the package config file for that method. If that variable isn't present, then we'll use the libmesh.pc which should always be present and its content will follow the rules outlined in the libMesh configuration files.

[roystgnr]

I think it's safe to assume we won't be using any of those types of constructs in OpenMC anytime soon

Don't say that. Okay, I don't put enough assertions into my code either, but I at least have the good taste to be sad about it and to intend future improvement.

To keep it simple I think what I would recommend is attempted detection of a METHOD environment variable in OpenMC's FindLIBMESH.cmake file. If that variable is present, then we'll fail if we can't find the package config file for that method. If that variable isn't present, then we'll use the libmesh.pc which should always be present and its content will follow the rules outlined in the libMesh configuration files.

That works, assuming you're only incorporating the part of the content that's necessary. Which looks like it's probably what's happening, in the list(APPEND cxxflags ${LIBMESH_CFLAGS}) line, but I'm not familiar enough with cmake to say for sure.

[pshriwise]

@roystgnr thanks for the feedback! I'll make sure that what we've discussed here (the detection of METHOD with a fallback onto libmesh.pc) gets added to the version of the FindLIBMESH.cmake file in my current PR for this capability.

[pshriwise]

Realizing I didn't really respond to this first part.

I think it's safe to assume we won't be using any of those types of constructs in OpenMC anytime soon

Don't say that. Okay, I don't put enough assertions into my code either, but I at least have the good taste to be sad about it and to intend future improvement.

Haha fair enough. What I meant here really is that we won't be using those constructs directly as we use the gsl-lite guidelines instead. Turns out there are in fact some static_assert's in there though, so I may need to look more carefully at this...

[roystgnr]

What I meant here really is that we won't be using those constructs directly as we use the gsl-lite guidelines instead.

With Expects()/Ensures(), controlled by gsl_CONFIG_CONTRACT_CHECKING* macros? Okay, that makes sense.

Turns out there are in fact some static_assert's in there though, so I may need to look more carefully at this...

No, static_assert is actually fine! Since it gets checked at compile time rather than run time (and so only needs to be checked once, or at worst once per level of recursion in a template metaprogram, never once per iteration through an inner loop), there's no serious cost to compilers simply always checking them; IIRC there's not even a standard way to turn them off.

[pshriwise]

With Expects()/Ensures(), controlled by gsl_CONFIG_CONTRACT_CHECKING* macros? Okay, that makes sense.

Yep.

No, static_assert is actually fine! Since it gets checked at compile time rather than run time (and so only needs to be checked once, or at worst once per level of recursion in a template metaprogram, never once per iteration through an inner loop), there's no serious cost to compilers simply always checking them; IIRC there's not even a standard way to turn them off.

Ah, I see. Well then we'll proceed as planned! Thanks for the discussion around this.