ENH: Minimal changes for side-by-side CMake build #358

Patol75 · 2022-07-01T07:26:36Z

See discussions in #115 and #356.

The present PR offers a minimal changeset to incorporate a CMake build within Fluidity.

The branch passes both building and unit-testing using the CMake approach (Actions run). However, I have not made sure I did not break the Autotools CI, but this is being checked by GitHub.

-> Autotools builds are failing, but I am afraid I do not understand why. From what I see, the relevant part of the log is:

#13 318.7     Congratulations, make makefiles succeeded!
#13 DONE 318.7s
#14 [ 9/13] RUN test -z "$(git status --porcelain */Makefile.dependencies)"
#14 ERROR: executor failed running [/bin/sh -c test -z "$(git status --porcelain */Makefile.dependencies)"]: exit code: 1
------
 > [ 9/13] RUN test -z "$(git status --porcelain */Makefile.dependencies)":
------
error: failed to solve: executor failed running [/bin/sh -c test -z "$(git status --porcelain */Makefile.dependencies)"]: exit code: 1
Error: buildx failed with: error: failed to solve: executor failed running [/bin/sh -c test -z "$(git status --porcelain */Makefile.dependencies)"]: exit code: 1

angus-g · 2022-07-04T01:35:47Z

Autotools builds are failing, but I am afraid I do not understand why

I think this is because the changes to the imports in:

femtools/Diagnostic_variables.F90
femtools/Sparse_Matrices_Fields.F90
assemble/Zoltan_integration.F90
are changing Make.dependencies (generated with make makefiles).

Patol75 · 2022-07-04T03:29:34Z

Thanks, @angus-g, that did the trick! From what I now see, Fluidity builds on Bionic, Focal and Impish using Autotools, and it should also build on Jammy using CMake. That is a good start!

angus-g

Nice work on the whole! A few minor tweaks needed in some places, but there are also a few major things that need addressing.

Also, are the .F90 changes from #349 strictly required? Otherwise it would be nice to keep them separated and avoid potential merge conflicts. Additionally, it would be good to rebase this onto main to drop the merge commit from the PR.

My "not sure" comments regarding the Python installs are that I don't think that a build command should install into the user's Python environment.

angus-g · 2022-07-04T07:04:38Z

libspud/diamond/CMakeLists.txt

+add_custom_target(spud_diamond ALL
+  COMMAND ${Python3_EXECUTABLE} -m pip install --force-reinstall .
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  VERBATIM
+)


Not sure about this...

angus-g · 2022-07-04T07:05:10Z

libspud/dxdiff/CMakeLists.txt

+add_custom_target(spud_dxdiff ALL
+  COMMAND ${Python3_EXECUTABLE} -m pip install --user --force-reinstall .
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  VERBATIM
+)


Not sure about this...

angus-g · 2022-07-04T07:05:25Z

libspud/python/CMakeLists.txt

+add_custom_target(spud_python ALL
+  COMMAND ${Python3_EXECUTABLE} -m pip install --user --force-reinstall .
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  VERBATIM
+)


Not sure about this...

angus-g · 2022-07-04T07:18:24Z

python/CMakeLists.txt

+add_custom_target(fluidity_python ALL
+  COMMAND ${Python3_EXECUTABLE} -m pip install --force-reinstall .
+  WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}
+  VERBATIM
+)


Not sure about this...

assemble/tests/CMakeLists.txt

CMakeLists.txt

angus-g · 2022-07-04T08:24:58Z

CMakeLists.txt

+FetchContent_Declare(libspatialindex
+  GIT_REPOSITORY https://github.com/libspatialindex/libspatialindex
+  GIT_TAG origin/master
+  LOG_DOWNLOAD ON
+  LOG_CONFIGURE ON
+  LOG_BUILD ON
+  LOG_INSTALL ON
+)
+FetchContent_MakeAvailable(libspatialindex)
+
+# Add Judy
+ExternalProject_Add(libjudy
+  PREFIX libjudy
+  URL https://sourceforge.net/projects/judy/files/judy/Judy-1.0.5/Judy-1.0.5.tar.gz/download
+  CONFIGURE_COMMAND ./configure --enable-64-bit --prefix=<SOURCE_DIR>
+  BUILD_COMMAND make
+  BUILD_IN_SOURCE TRUE
+  INSTALL_COMMAND make install exec_prefix=<SOURCE_DIR>
+  TEST_COMMAND make check
+  TEST_BEFORE_INSTALL TRUE
+  LOG_DOWNLOAD ON
+  LOG_CONFIGURE ON
+  LOG_BUILD ON
+  LOG_INSTALL ON
+  LOG_TEST ON
+)
+ExternalProject_Add_Step(libjudy byproducts
+  DEPENDEES install
+  BYPRODUCTS <SOURCE_DIR>/lib/libJudy.a
+  LOG ON
+)
+ExternalProject_Get_property(libjudy SOURCE_DIR)
+set(libjudy_SOURCE_DIR ${SOURCE_DIR})
+add_library(Judy STATIC IMPORTED)
+set_target_properties(Judy
+  PROPERTIES IMPORTED_LOCATION ${libjudy_SOURCE_DIR}/lib/libJudy.a
+)
+
+# Add Zoltan
+if(${CMAKE_C_COMPILER_VERSION} GREATER_EQUAL 10)
+  set(ZOLTAN_CONFIGURE ../libzoltan/configure FCFLAGS=-fallow-argument-mismatch --enable-f90interface --with-gnumake --with-scotch --with-scotch-incdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/include --with-scotch-libdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/lib --with-parmetis --with-parmetis-incdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/include --with-parmetis-libdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/lib)
+else()
+  set(ZOLTAN_CONFIGURE ../libzoltan/configure --enable-f90interface --with-gnumake --with-scotch --with-scotch-incdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/include --with-scotch-libdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/lib --with-parmetis --with-parmetis-incdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/include --with-parmetis-libdir=$ENV{PETSC_DIR}/$ENV{PETSC_ARCH}/lib)
+endif()
+ExternalProject_Add(libzoltan
+  PREFIX Zoltan
+  GIT_REPOSITORY https://github.com/sandialabs/Zoltan
+  GIT_TAG origin/main
+  CONFIGURE_COMMAND ${ZOLTAN_CONFIGURE}
+  BUILD_COMMAND make everything
+  BUILD_BYPRODUCTS <BINARY_DIR>/src/libzoltan.a
+  INSTALL_COMMAND ""
+  LOG_DOWNLOAD ON
+  LOG_CONFIGURE ON
+  LOG_BUILD ON
+)
+ExternalProject_Get_property(libzoltan BINARY_DIR)
+set(Zoltan_BINARY_DIR ${BINARY_DIR})
+add_library(Zoltan STATIC IMPORTED)
+add_dependencies(Zoltan libzoltan)
+set_target_properties(Zoltan
+  PROPERTIES IMPORTED_LOCATION ${Zoltan_BINARY_DIR}/src/libzoltan.a
+)


I understand the appeal of this, but I don't like forced dependency downloads at build/configure time. Both libjudy and libspatialindex are already in the repository and used for the autotools build, so there should be an option to build using these already-distributed methods (at least until they're eventually removed). That would probably come down to a cache variable for each of them, perhaps USE_BUNDLED_LIBJUDY or something like that.

For all three of the libraries (libjudy, libspatialindex, Zoltan), I think there should be the option to use a custom/system-distributed library before building something as part of fluidity. This probably means providing FindJudy, FindSpatialindex and FindZoltan, then converting the three builds to use the FetchContent mechanism, and using FIND_PACKAGE_ARGS in FetchContent_Declare (or, only building if the finds fail).

I am leaving this one out for now, I will come back to it later.

I am starting to look at this now. The following changes should be enough to use the included libjudy:

ExternalProject_Add(libjudy # PREFIX libjudy # URL https://sourceforge.net/projects/judy/files/judy/Judy-1.0.5/Judy-1.0.5.tar.gz/download SOURCE_DIR ${CMAKE_SOURCE_DIR}/libjudy CONFIGURE_COMMAND ./configure --enable-64-bit --prefix=<SOURCE_DIR> BUILD_COMMAND make BUILD_IN_SOURCE TRUE INSTALL_COMMAND make install exec_prefix=<SOURCE_DIR> TEST_COMMAND make check TEST_BEFORE_INSTALL TRUE # LOG_DOWNLOAD ON LOG_CONFIGURE ON LOG_BUILD ON LOG_INSTALL ON LOG_TEST ON )

However, running configure does not actually generate a Makefile. Does anyone know what I am missing?

If you look at fluidity's main configure there's some comments where it calls libjudy's configure. So it appears indeed libjudy's ./configure has been neutered and doesn't actually produce any useful output. Where the magic happens is at the end of fluidity's main configure: there's a big list of files in AC_CONFIG_FILES where it substitutes @variable@ with environment variables if they've been marked as output variables using AC_SUBST - that list includes the Makefiles in the libjudy (sub)directories. So it appears the only reason we run libjudy's configure is to run its configure tests.

Thank you @stephankramer, I get it now. So it is not an easy fit. Nevertheless, it looks like it is possible.

CMakeLists.txt

Patol75 · 2022-07-05T09:24:47Z

Thanks for the review, @angus-g! I have addressed quite a few things, but they are still some left. Have a look at the discussions in the unresolved comments.

Regarding the Python packages, I just found it extremely convenient to have these targets included in the main build. What would be the alternative? Maybe @stephankramer has a suggestion?

About the F90 changes, please have a look below: I have gathered all the error messages I get if I keep the original files.

/home/thomas/Softwares/fluidity_minimal/femtools/Sparse_Matrices_Fields.F90:34:5:

   34 | use c_interfaces
      |     1
Fatal Error: Cannot open module file ‘c_interfaces.mod’ for reading at (1): No such file or directory
compilation terminated.

/home/thomas/Softwares/fluidity_minimal/femtools/Sparse_Matrices_Fields.F90:83:8:

   83 |     if (compare_pointers(c_loc(x), c_loc(b))) then
      |        1
Error: Function ‘compare_pointers’ at (1) has no IMPLICIT type

/home/thomas/Softwares/fluidity_minimal/femtools/Diagnostic_variables.F90:1185:80:

 1185 |     call get_environment_variable("HOSTNAME", value_buffer, default = "Unknown")
      |                                                                                1
Error: Cannot find keyword named ‘default’ in call to ‘get_environment_variable’ at (1)

/usr/bin/ld: lib/libfluidity.a(Diagnostic_variables.F90.o): in function `__diagnostic_variables_MOD_write_detectors':
/home/thomas/Softwares/fluidity_minimal/femtools/Diagnostic_variables.F90:2648: undefined reference to `cget_nan_'
/usr/bin/ld: /home/thomas/Softwares/fluidity_minimal/femtools/Diagnostic_variables.F90:2684: undefined reference to `cget_nan_'
collect2: error: ld returned 1 exit status

…d libjudy

…packages

for more information, see https://pre-commit.ci

Patol75 requested a review from angus-g July 1, 2022 07:26

angus-g requested changes Jul 4, 2022

View reviewed changes

Patol75 force-pushed the cmake_build_2022_minimal branch from 0e5a4d9 to 5e9529b Compare July 5, 2022 09:26

Patol75 added 5 commits August 22, 2022 18:35

ENH: Minimal changes for side-by-side CMake build.

db0815e

BUG: Update Makefile dependencies

2bf8d74

ENH: Address part of @angus-g review

8fd2950

ENH: Finer control over Zoltan configure; mark UDUNITS as optional

df09291

ENH: Address comments on external libraries and Python packages

3c87987

Patol75 force-pushed the cmake_build_2022_minimal branch from 2dc3e82 to 3c87987 Compare August 22, 2022 08:35

Patol75 added 5 commits August 23, 2022 17:43

MAINT: Minor formatting changes; Remove incorrect approach for bundle…

c325031

…d libjudy

ENH: Try to speed up the building process; Try to fix install_python_…

4c56ef3

…packages

BUG: Fix incorrect branch in Dockerfile

a00b002

BUG: Limit number of Ninja jobs

05fb7a2

BUG: Fix manual build when invoked through CMake

f52701e

Patol75 mentioned this pull request Feb 6, 2024

Installtion issues around VTK and BLAS libraries #384

Open

Patol75 and others added 2 commits May 6, 2024 09:26

Merge branch 'main' into cmake_build_2022_minimal

12f30fb

[pre-commit.ci] auto fixes from pre-commit.com hooks

bc011e3

for more information, see https://pre-commit.ci

Patol75 mentioned this pull request May 6, 2024

Compilation changes #385

Open

Patol75 mentioned this pull request Aug 24, 2024

Any plans for noble ppa release? #387

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Minimal changes for side-by-side CMake build #358

ENH: Minimal changes for side-by-side CMake build #358

Patol75 commented Jul 1, 2022 •

edited

Loading

angus-g commented Jul 4, 2022 •

edited

Loading

Patol75 commented Jul 4, 2022

angus-g left a comment •

edited

Loading

angus-g Jul 4, 2022

angus-g Jul 4, 2022

angus-g Jul 4, 2022

angus-g Jul 4, 2022

angus-g Jul 4, 2022

Patol75 Jul 5, 2022

Patol75 Aug 22, 2022

stephankramer Aug 22, 2022 •

edited

Loading

Patol75 Aug 23, 2022

Patol75 commented Jul 5, 2022

ENH: Minimal changes for side-by-side CMake build #358

Are you sure you want to change the base?

ENH: Minimal changes for side-by-side CMake build #358

Conversation

Patol75 commented Jul 1, 2022 • edited Loading

angus-g commented Jul 4, 2022 • edited Loading

Patol75 commented Jul 4, 2022

angus-g left a comment • edited Loading

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

stephankramer Aug 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Patol75 commented Jul 5, 2022

Patol75 commented Jul 1, 2022 •

edited

Loading

angus-g commented Jul 4, 2022 •

edited

Loading

angus-g left a comment •

edited

Loading

stephankramer Aug 22, 2022 •

edited

Loading