Build dynamic library by default

Using a statically linked dune-common together with the Python bindings leads to hard-to-diagnose crashes and other bugs (specifically, a double 'free' in the destructor of DebugStream). This patch therefore enables shared libraries by default, and adds an explanation of why this is necessary.

The same for dune-uggrid: staging/dune-uggrid!242

mentioned in merge request staging/dune-uggrid!242

I think this should be in 2.10. The crashes that result from using a statically linked dune-common with Python are completely unsurmountable for regular users.

This implementation always builds dynamic libraries. Static libraries also have its use cases. IMO we have to instead design default CMake presets for the python bindings rather hard coding this into de build system.

I think this should be in 2.10.

• We have a modular design, this means that either every module also has to set this, or the user may end up with mixed libraries. This can be even more confusing: each kind has its own set of challenges. If we want to have a consistent behavior across modules, this is too late for the release.
• This sounds like trying to masquerade bugs that we could not solve (or we were not aware of). Shared libraries might solve the immediate bug in dune-common, but why to ship something where we have even less experience with?

In the cmake docu they write that there are two types of presets, the one the could be checked-in to the repository, the other one a private preset. Maybe we can ship a preset for the python bindings.

I agree, that changing the default library type to "shared" in just one module looks surprising to me and it is not very well tested so short before a release. We could instead add a proper error/warning message in case python bindings are enabled and static libraries are beeing used.

An error/warning would be much better IMO than the status quo. Note though that an error will mean that a default Dune build will always fail: Python bindings are on by default (rightfully so, IMO), but so are static-only libraries.

Implementing an error messages exceeds my CMake guru level, unfortunately.

I will be more than happy to replace this by something better after 2.10. However, right now I recommend this new "easy" Python interface to people, and it crashes without helpful error message as soon as they do something nontrivial with UGGrid. Yes, maybe it says "for Python you need to enable shared libraries when building", but honest, who reads that?

And I don't see the downside to my proposal. What's so bad about mixing shared libraries with static ones?

Mixing static and dynamic library also need to be done with some care. I guess, one needs to enable fPIC and maby some other flags.

A downside of dynamic libraries, also pointed out by @andreas.dedner, is that we need to make sure all the paths to the dynamically loaded libraries and dependencies are correctly set in the packages/executables, even when copied around. This is not trivial. Actually, CMake should help with that and we can probably investigate this again, but it would take some time and the outcome is open. It probably works nicely in a local setup, if you just run dunecontrol, but in combination with installation and packaging we need to be more careful.

in my opinion, this long rationale text should not be in the main CMakeLists file, like an excuse that we have not developed code that works correctly with all library types. Instead, it should be in the changelog or in the MR description.

I put it there deliberately. People who change this setting need to be made aware of the consequences. And they will look neither in MR descriptions nor in old change logs.

This is a developer note. The main CMakeLists file is for users. If we add a proper CMake error message that says: we need to build shard libraries for the python bindings, then we could add a comment above like "see #1234 for details".

I agree with that.

Using a regular variable will not allow users to change this. Suggestion:

option(BUILD_SHARED_LIBS "Build using shared libraries" ON)

later on in the CMakeLists file, one could also check for properly set BUILD_SHARED_LIBS flag in case python bindings are enabled: Maybe something like

if(DUNE_ENABLE_PYTHONBINDINGS)
  if (NOT BUILD_SHARED_LIBS)
    message(FATAL_ERROR "The option BUILD_SHARED_LIBS is required for the Python bindings to work correctly. Set this option on the command line or the Dune .opts file.")
  endif()
  add_subdirectory(python)
endif()

This works nicely -- I'll update the merge request.

But see my comment above: Unless the default for BUILD_SHARED_LIBS really is ON, default builds will fail.

changed this line in version 2 of the diff

I agree. The message shall be a warning, and the CMake option should be avoided if we can give a preset, or postponed for the next release.

The lengthy explanation is still there, because I don't really know where to point the reader to.

All current testing setups will also stop working. The nightly tests / system tests do not use shared libs. The pip packages build libdunecommon.a as well so pypi packages will fail as well as all the github tests.

In fact this MR makes the tests on github fail already. Switching to shared libs might be a problem for the packaging, e.g., https://github.com/dune-project/dune-testpypi/actions/runs/10680388545/job/29601736577

At the moment package building does not work with shared libraries. The issue is that they are build in an isolated environment and then need to be copied into the venv. Shared libraries are not easy to move - at least we didn't get it to work. Samuel and myself did some work on this end of 2020/2021 (see log messages) playing around with setting rpaths etc. but couldn't get it to work. So decided to turn off shared libs for the packaging. So if we enable shared by default this has to be turned off during build from package index, i.e., if(not SKBUILD)

That's not good, of course. So close to the release we should then turn shared libs off again by default, and live with warning-only. Is that okay?

We could check while loading any python module from dune-ug if libduneug was shared. If not a meaningful error message could be printed. That does not change anything for current setup as long as I don't use dune.grid.uggrid in my code. If I do then an error message is probably useful. Just not sure how to check that libduneug is available as so file...

Unfortunately, it is not only dune-uggrid, also dune-common shows a bug with global variables if debugstreams (I think) are used somewhere. It just showed up in a dune-uggrid test.

Would it work to allowed static libs for packaging and using dynamic libs when building directly?

The lengthy explanation is still there, because I don't really know where to point the reader to.

This is easy. Once we have clarified what is the actual problem, we create an issue in dune-common gitlab explaining this and the possible solutions and point the user to that issue.

Would it work to allowed static libs for packaging and using dynamic libs when building directly?

The SKBUILD cmake variable is used for that also in other places. But I wouldn't want to do this for 2.10

I should perhaps add that this is what I do now - in my config.opts file I've always had -DBUILD_SHARED_LIBS=ON while anything with the pypi packages will be without shared since that is the default.

So, BUILD_SHARED_LIBS seems to work in our system tests. I will run the testing-scenarios again to see the effect of the latest changes. If this does not work (if fails), we cannot (yet) activate BUILD_SHARED_LIBS by default. Adding a warning (or even an error) if shared libraries are not activated (which is not the current default) seems to be a strange behavior as well. We claim that it is not working without that flag but the flag is not set by default but the python bindings are enabled by default (leading to a CMake warning by default) - this is surprising.

Not sure whether we find an acceptable solution for 2.10 so quickly. Maybe we can backport a solution once we have decided on a behavior.

Not sure whether we find an acceptable solution for 2.10 so quickly.

I think is fine to change to shared libraries (although I would prefer the warning TBH). But making this change on the same day of the release is not acceptable. We simply cannot foresee all the possible consequences of this change in few hours.

added 1 commit

• 76172bd3 - Build dynamic library by default

Compare with previous version

Independent of whether this will be the default or not, we should investigate why the CI for the core modules fails when dune-common has dynamic libraries. As far as I can see, this is not even related to the python bindings.

added 91 commits

• 76172bd3...b4c8be7f - 90 commits from branch master
• e13bc94a - Build dynamic library by default

Compare with previous version

mentioned in issue infrastructure/dune-nightly-test#2

I think it is a good time to continue with this topic. We have no release soon and once we merge a solution, we can think about back-porting it to 2.10.