Update world builder to version 0.5.0. #5164

MFraters · 2023-06-01T17:02:54Z

This is a work in progress pull request for updating the world builder. It does two things:

Update the world builder to the latest pre-release version (release candidate?).
Change the way the world builder is build by ASPECT. It is now builds it as a separate project.

Do not merge! Merging needs to wait until the release is done, but I would like approval of this pull request before I make the release. I have made a todo here: GeodynamicWorldBuilder/WorldBuilder#493

Reviews are very welcome!

gassmoeller

My comments so far. Now testing to compile against different worldbuilder versions on some of my systems.

gassmoeller · 2023-06-02T17:49:03Z

CMakeLists.txt

@@ -135,66 +135,71 @@ if(ASPECT_WITH_WORLD_BUILDER)
    MESSAGE(STATUS "World Builder not found. Using internal version.")
    SET(WORLD_BUILDER_SOURCE_DIR "${CMAKE_SOURCE_DIR}/contrib/world_builder/" CACHE PATH "" FORCE)
  ENDIF()
+
+  # add source and include dirs:
+  MESSAGE(STATUS "Using World Builder version ${WORLD_BUILDER_VERSION} found at ${WORLD_BUILDER_SOURCE_DIR}.")


This line needs to be below 142 and 143 to give the correct WORLD_BUILDER_VERSION.

gassmoeller · 2023-06-02T17:59:30Z

CMakeLists.txt

-    SET(UNITY_WB_LAST_FILES
-    "${WORLD_BUILDER_SOURCE_DIR}/source/parameters.cc")
+  IF(WORLD_BUILDER_VERSION VERSION_GREATER_EQUAL 0.5.0 AND NOT WORLD_BUILDER_VERSION_LABEL MATCHES "pre")
+    ADD_SUBDIRECTORY("${WORLD_BUILDER_SOURCE_DIR}" ${CMAKE_BINARY_DIR}/world_builder/ SYSTEM)


SYSTEM is a keyword that was only added in cmake 3.26.1. It works fine without the keyword for me (cmake 3.22).

CMakeLists.txt

gassmoeller · 2023-06-02T18:03:21Z

contrib/world_builder/CMakeLists.txt

@@ -16,35 +16,72 @@ if(POLICY CMP0058)
 endif()

 # load in version info and export it
-SET(WORLD_BUILDER_SOURCE_DIR ${CMAKE_SOURCE_DIR})
-INCLUDE("${CMAKE_SOURCE_DIR}/cmake/version.cmake")
+SET(WORLD_BUILDER_SOURCE_DIR ${PROJECT_SOURCE_DIR})


Line 1 triggers a warning from my cmake (Compatibility with CMake < 2.8.12 will be removed from a future version of CMake.). Requiring cmake 2.8.12 removes the warning and 2.8.12 was release in 2013, so I would guess there is no harm in increasing WorldBuilder's minimum cmake version to 2.8.12? ASPECT already requires cmake 3.1.0 so we could also just require a higher version for the worldbuilder version inside ASPECT.

Currently the minimum cmake version in the world builder is 2.8.4, but that has been the case since probably version 0.3.0, so almost 3 year (but at least 2). I see no harm in, and I think it is a good idea to increase it to 2.8.12. I will make the changes.

tjhei · 2023-06-02T19:18:09Z

CMakeLists.txt

-    SET(UNITY_WB_LAST_FILES
-    "${WORLD_BUILDER_SOURCE_DIR}/source/world_builder/parameters.cc")
-  ENDIF()
+    IF(WORLD_BUILDER_VERSION VERSION_LESS 0.5.0)


If we bundle 0.5, we might as well set the minimum supported version to 0.5.

Because there may be a lot of users out there with custom world builder files and versions below 0.5 release (us for example). World builder files for version 0.4 do not work with 0.5, and requiring 0.5 forces a lot of people to upgrade their workflow immediately after release of world builder 0.5. I would rather wait until the next ASPECT release before we do that.

Are you sure it's worth it for us to be forward and backward compatible? If a user updates to a new aspect release we can also expect that they are willing to upgrade WB. There is value in bundling the oldest supported version to improve test coverage.

Anyways, not a big deal if you feel confident this will still work..

Well we bundled 0.4.0 up to now, so we are sure it works at the moment. I am ok dropping support for World Builder 0.4.0 right after ASPECT's release, but I would like to keep it for the release. Mostly, to not surprise users who are currently using ASPECT's development environment and WB 0.4.0, then update the ASPECT version by a few days and suddenly need to use the bundled WB 0.5.0 immediately at the time of the WB release.

gassmoeller · 2023-06-02T21:18:35Z

So we found an issue with including the world builder cmake system into ASPECT's system that currently makes me think we should hold off on integrating the cmake changes immediately for this version (at least for the combination of world builder version and aspect release that we plan to ship for aspect 2.5). On clusters with compiler modules (at least on Frontera) world builder finds a different C/CXX/Fortran compiler than the one that is used by the MPI system (in my case it defaulted to the compilers in /usr/bin). This of course causes havoc when compiling. The system seems to work nicely on PC systems where the found compiler is usually the only one on the system.

I can still compile on Frontera with the following additional lines in the ASPECT CMakeLists.txt before the call to ADD_SUBDIRECTORY:

    SET (ENV{CXX} ${CMAKE_CXX_COMPILER})
    SET (ENV{CC} ${CMAKE_C_COMPILER})
    SET (ENV{FC} ${CMAKE_Fortran_COMPILER}) # This does not help the fortran wrapper, I just include for completeness
    #SET (WB_BUILD_DOCUMENTATION OFF CACHE BOOL "")
    #SET (WB_ENABLE_PYTHON OFF CACHE BOOL "")
    #SET (WB_INCLUDE_UNIT_TEST OFF CACHE BOOL "")
    SET (WB_MAKE_FORTRAN_WRAPPER OFF CACHE BOOL "") # this is required
    #SET (WB_RUN_APP_TESTS OFF CACHE BOOL "")

@tjhei: Do you have an idea for how to prevent a cmake project from finding the wrong compiler? the MPI_CXX_COMPILER variable is set correctly by the call to FindMPI, but CMAKE_CXX_COMPILER points to the wrong compiler (and this one is used). The solution I used above is to point the environment variable CXX to the correct compiler, but ideally the world builder should figure this out itself.

Menno: As discussed, I would like to still integrate the changes we made in this PR, we are much closer to using the cmake system now, just dont activate them yet for the combination of WB and ASPECT that we are going to release (we can activate it afterwards in aspects developer version).

MFraters · 2023-06-02T21:24:17Z

@gassmoeller, I have changed the check from 0.5.0-pre to 0.6.0(-pre) as discussed for now. If we can find a satisfactory solution before the release, we can change it back, otherwise we keep it.

gassmoeller

Ok, this looks good to me then. Let's keep this PR open for a few days before merging to give others the chance to comment. But at least from ASPECT's side there should be no barrier left for the world builder release.

gassmoeller · 2023-06-02T21:25:52Z

CMakeLists.txt

@@ -135,66 +135,71 @@ if(ASPECT_WITH_WORLD_BUILDER)
    MESSAGE(STATUS "World Builder not found. Using internal version.")
    SET(WORLD_BUILDER_SOURCE_DIR "${CMAKE_SOURCE_DIR}/contrib/world_builder/" CACHE PATH "" FORCE)
  ENDIF()
-
-  # Always include the header files.
+


Remove whitespace

MFraters · 2023-06-02T22:46:33Z

found a relevant quote on the issue we are having: https://cmake.org/pipermail/cmake/2011-June/045037.html

You should *NEVER* set CMAKE_<LANG>_COMPILER, this completely breaks
things. Also, with CMake you shouldn't use the compiler wrappers (mpicxx
etc.), because CMake figures out how to compile MPI programs without it.

So that would mean we need to focus on getting the right c++ into the world builder and not the right mpi compiler.

MFraters · 2023-06-02T23:27:44Z

I think I may have found a solution, but I am currently unable to test it on stampede2. The idea is that since the world builder needs to select a good enough compiler (c++14 or higher), we can enforce that. My hope if that if it sees that the compiler it normally found isn't good enough, it will hopefully select the better one (and hopefully the same as aspect found). I have updated this pull request with the relevant cmake statement, let me know if it solve the issue for you.

tjhei · 2023-06-03T03:09:13Z

Yes, do not specify MPI compilers directly.

I am a bit worried that other settings could also break this setup: compiler flags are handled correctly for example?

What is the advantage of using the GWB cmake file? I am not aware of any advantages over including the files manually. If there are no big advantages, let's wait until after the release...

MFraters · 2023-06-03T16:12:09Z

I am a bit worried that other settings could also break this setup: compiler flags are handled correctly for example?

It is build as a completely separate project, so it has its own compiler flags and then aspect links to the build library.

What is the advantage of using the GWB cmake file? I am not aware of any advantages over including the files manually. If there are no big advantages, let's wait until after the release...

There are a few adantages. It simplifies the aspect cmake, it really separates out the world builder and enables unity build (although that could also be done within aspect), it provides users with the tools to quickly view initial conditions so that they don't have to run it aspect every time. Non of them are requires, or exclusive to this approach, but I thought this would be more clean within aspect.

I agree that it would be good for this to have more time for testing. The current code does the previous approach for the bundled world builder, but will use the new approach for the next development version which users would have to point aspect to themselfs. Does this approach work for you Timo or do you want the new cmake code removed?

tjhei · 2023-06-04T15:09:58Z

Ok, interesting. What does GWB produce in this setup: a shared library, a static library, or an object file?

MFraters · 2023-06-04T18:17:00Z

hmm, good question. I think it is static. It was a bit of a hunt in the cmake manual, but here are the revelant links:

aspect uses target link libraries: https://cmake.org/cmake/help/latest/command/target_link_libraries.html
The library is created by the add_library function: https://cmake.org/cmake/help/latest/command/add_library.html
https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html#variable:BUILD_SHARED_LIBS -> If present and true, this will cause all libraries to be built shared unless the library was explicitly added as a static library. -> Not present in the world builder so a static library is build.

gassmoeller · 2023-06-05T18:38:06Z

What does GWB produce in this setup: a shared library, a static library, or an object file?

It is build as a static library in $CMAKE_BINARY_DIR/world_builder/lib that is then linked into the ASPECT binary and also installed alongside aspect into the bin/ directory (the installation works).

Not sure if this is the best option, or if it matters. I find it a bit weird that is is installed into bin/ and not lib/, @MFraters I think this is caused by world_builder CMakelists:544.

@tjhei: Would you prefer Menno split this into two PRs, one that includes the new WB version but the current cmake system, and then another PR that includes the cmake changes? We would like to release the new WB version as soon as possible to do some testing before the release, but the cmake changes are not critical right now.

tjhei · 2023-06-05T18:42:47Z

If it is a static library, it doesn't need to be installed at all because it will be linked into the aspect binary.

We do need to be careful to have identical compiler flags. Imagine ASPECT and GWB having different c++ standard flags for example.

tjhei · 2023-06-05T18:56:12Z

Up to you guys. I am ok merging both if you think it's ready. I would assume most users of ASPECT releases use the bundled GWB anyways.

gassmoeller · 2023-06-05T20:03:39Z

I think this PR is ready. In the current form the new cmake system is disabled until either a user compiles against a new non-bundled worldbuilder or until we bundle the next world builder release or decide to manually activate it.

The new system already works fine on regular systems (only clusters can make problems). I agree we need to find a solution to ensure the same compiler and flags, but I think we can handle that later. Maybe we can let the bundled worldbuilder cmakelists modify to use the deal.II macros?

@MFraters: Want to go ahead with the world builder release? When that is done we can merge this PR.

MFraters · 2023-06-06T12:55:16Z

ok, sounds good. I will make the release and update this pull request.

MFraters · 2023-06-06T18:15:40Z

The world builder has been released and I have added a changelog entry for aspect. I think this pull request is now ready :)

gassmoeller

Good to go then. Thanks

MFraters force-pushed the update_gwb_to_0.5.0 branch from 3de97d4 to c974d11 Compare June 1, 2023 20:53

MFraters mentioned this pull request Jun 1, 2023

Todo: release 0.5.0 GeodynamicWorldBuilder/WorldBuilder#493

Closed

7 tasks

MFraters force-pushed the update_gwb_to_0.5.0 branch 2 times, most recently from 8e3ec34 to f959c14 Compare June 2, 2023 04:14

MFraters requested review from tjhei and gassmoeller June 2, 2023 13:04

MFraters force-pushed the update_gwb_to_0.5.0 branch from f959c14 to dc0fdfe Compare June 2, 2023 14:27

gassmoeller reviewed Jun 2, 2023

View reviewed changes

tjhei reviewed Jun 2, 2023

View reviewed changes

MFraters force-pushed the update_gwb_to_0.5.0 branch from ec5488e to 51d669d Compare June 2, 2023 21:18

gassmoeller approved these changes Jun 2, 2023

View reviewed changes

MFraters force-pushed the update_gwb_to_0.5.0 branch from 51d669d to 92c9869 Compare June 2, 2023 21:43

MFraters force-pushed the update_gwb_to_0.5.0 branch from 92c9869 to 46dd9b5 Compare June 2, 2023 23:02

MFraters added 3 commits June 6, 2023 20:01

Update world builder to version 0.5.0.

ba798d3

Change the way the world builder is build within ASPECT.

7d31e56

Update world builder tests.

5c745d1

MFraters force-pushed the update_gwb_to_0.5.0 branch from 46dd9b5 to 5c745d1 Compare June 6, 2023 18:01

MFraters changed the title ~~[WIP] Update world builder to version 0.5.0.~~ Update world builder to version 0.5.0. Jun 6, 2023

Add changelog entry for updating the world builder.

5294773

gassmoeller approved these changes Jun 6, 2023

View reviewed changes

gassmoeller merged commit c794185 into geodynamics:main Jun 6, 2023
6 checks passed

MFraters mentioned this pull request Jul 8, 2023

World builder separate cmake project #3771

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update world builder to version 0.5.0. #5164

Update world builder to version 0.5.0. #5164

MFraters commented Jun 1, 2023 •

edited

gassmoeller left a comment

gassmoeller Jun 2, 2023

gassmoeller Jun 2, 2023

gassmoeller Jun 2, 2023

MFraters Jun 2, 2023

tjhei Jun 2, 2023

gassmoeller Jun 2, 2023

tjhei Jun 3, 2023

gassmoeller Jun 5, 2023

gassmoeller commented Jun 2, 2023

MFraters commented Jun 2, 2023

gassmoeller left a comment

gassmoeller Jun 2, 2023

MFraters commented Jun 2, 2023

MFraters commented Jun 2, 2023

tjhei commented Jun 3, 2023

MFraters commented Jun 3, 2023

tjhei commented Jun 4, 2023

MFraters commented Jun 4, 2023

gassmoeller commented Jun 5, 2023

tjhei commented Jun 5, 2023 •

edited

tjhei commented Jun 5, 2023

gassmoeller commented Jun 5, 2023

MFraters commented Jun 6, 2023

MFraters commented Jun 6, 2023

gassmoeller left a comment

Update world builder to version 0.5.0. #5164

Update world builder to version 0.5.0. #5164

Conversation

MFraters commented Jun 1, 2023 • edited

gassmoeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gassmoeller commented Jun 2, 2023

MFraters commented Jun 2, 2023

gassmoeller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MFraters commented Jun 2, 2023

MFraters commented Jun 2, 2023

tjhei commented Jun 3, 2023

MFraters commented Jun 3, 2023

tjhei commented Jun 4, 2023

MFraters commented Jun 4, 2023

gassmoeller commented Jun 5, 2023

tjhei commented Jun 5, 2023 • edited

tjhei commented Jun 5, 2023

gassmoeller commented Jun 5, 2023

MFraters commented Jun 6, 2023

MFraters commented Jun 6, 2023

gassmoeller left a comment

Choose a reason for hiding this comment

MFraters commented Jun 1, 2023 •

edited

tjhei commented Jun 5, 2023 •

edited